Thanks for the comments.

(In reply to Jim Porter (:squib) from comment #20)
> Comment on attachment 8337301
> fix bucket groups to be based off of a calendar week concept
> 
> Review of attachment 8337301:
> -----------------------------------------------------------------
> 
> This isn't a full review; just a quick pass. David Bienvenu is unlikely to
> be super-response, since I believe he works at Google now. I've redirected
> the review to someone who might be a better pick (sorry if I redirected
> wrong!).

Thanks for pointing it to anyone who'll respond.

> 
> ::: mail/locales/en-US/chrome/messenger/messenger.properties
> @@ +178,3 @@
> >  lastWeek=Last Week
> >  twoWeeksAgo=Two Weeks Ago
> >  older=Older
> 
> We might need to change the names of these strings to force localizers to
> update them; after this patch, lastWeek doesn't refer to the same thing. A
> localizer might have translated that to something like "Within the last 7
> days", which is accurate for how the code works pre-patch, but would be
> wrong post-patch.

noted. suggestions ?

> 
> ::: mailnews/base/src/nsMsgGroupView.cpp
> @@ -135,5 @@
> > -    int64_t GMTLocalTimeShift = currentExplodedTime.tm_params.tp_gmt_offset +
> > -      currentExplodedTime.tm_params.tp_dst_offset;
> > -    GMTLocalTimeShift *= PR_USEC_PER_SEC;
> > -    currentTime += GMTLocalTimeShift;
> > -    dateOfMsg += GMTLocalTimeShift;
> 
> Why'd you remove the time-shifting? Isn't that important, since we want to
> figure out when midnight is in local time, not GMT?

It didn't seem to work correctly.
With it, I had messages that were very near to the bucket boundaries, that sometimes, were on the wrong side of the bucket boundary.

Commented out, all my messages seemed to align correctly at the boundaries.

> 
> @@ +135,5 @@
> > +    // the localization for first day of calendar
> > +    int64_t todayMidnight = currentTime - currentTime % PR_USEC_PER_DAY;
> > +    int64_t yesterday = todayMidnight - PR_USEC_PER_DAY;
> > +    int64_t thisWeek = todayMidnight - (PR_USEC_PER_DAY *
> > +                                       (currentExplodedTime.tm_wday + 0));
> 
> Why the "+ 0" here?

You missed the line above. It's a place holder for the start of calendar week localization offset.

> 
> @@ +137,5 @@
> > +    int64_t yesterday = todayMidnight - PR_USEC_PER_DAY;
> > +    int64_t thisWeek = todayMidnight - (PR_USEC_PER_DAY *
> > +                                       (currentExplodedTime.tm_wday + 0));
> > +    int64_t lastWeek = thisWeek - (PR_USEC_PER_DAY * 7);
> > +    int64_t twoWeeks = lastWeek - (PR_USEC_PER_DAY * 7);
> 
> Nit: I'd call this lastTwoWeeks to be clearer.

Noted.

> 
> @@ +778,5 @@
> >              if (m_kOldMailString.IsEmpty())
> >                m_kOldMailString.Adopt(GetString(NS_LITERAL_STRING("older").get()));
> >              aValue.Assign(m_kOldMailString);
> >              break;
> > +          case Invalid:
> 
> I don't think we really need to add this here; the default case will catch
> it.

Yeah, it was a Nit on my part, default aside, I recalled that some compilers complain if every instance of an enum is not handled, so I was being pro-active. :)

> 
> ::: mailnews/base/src/nsMsgGroupView.h
> @@ +46,5 @@
> >    virtual void InternalClose();
> >    nsMsgGroupThread *AddHdrToThread(nsIMsgDBHdr *msgHdr, bool *pNewThread);
> >    virtual nsresult HashHdr(nsIMsgDBHdr *msgHdr, nsString& aHashKey);
> > +
> > +  enum AgeBucket_t { Invalid, Today, Yesterday, ThisWeek, LastWeek, TwoWeeksAgo, Older };
> 
> I think it would make more sense to put this at the top of the protected:
> list, and also to put each value on its own line.
> 
> I'm not sure what Mozilla's standard for naming enums is; I don't think it's
> Foo_t though.

Noted, I briefly looked for coding standard for enums, and couldn't find anything.

I'll wait for more comments before taking any more action.
I don't want to litter this bug with a bunch of back and forth.