Comment 30 for bug 237341

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!).

::: 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.

::: 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?

@@ +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?

@@ +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.

@@ +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.

::: 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.