390614 - Split up the "Older than 6 days" history folder

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement, P4)

Description

[Oliver Saier]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007073104 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007073104 Minefield/3.0a7pre

Bug 332748 made me think of filing this bug. There may be several usability issues with the "Older than 6 days" folder:
1. it looks weird to have one folder per day, then one folder for 174 days.
2. expanding it takes a long time (bug 385245)
3. it contains thousands of entries so it's nearly useless to scroll down in the hope to find /the/ page you visited x months ago.

It'd be cool if history folders were dynamically created on the basis of browser.history_expire_days, or if a, say, 3-month timeframe was used:
+ 1 week ago
+ ...
+ 1 month ago
+ ...
+ 3 months ago

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Oliver Saier]

Well, the correct captions would be:
+ Last week
+ 2 weeks ago
...
+ Last month
...

Comment 2

[(not reading, please use [e-mail] instead)]

given that we are now doing 180 days by default, this seems like a good idea.  (see bug #332748)

it also looks like faaborg has some ideas about this, see bug #387730

Comment 3

[Ria Klaassen (not reading all bugmail)]

The "calendar" worked very well in Places' early days; you could see the history of every day you wished. It was really an impressing feature but it was taken out.  

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

We need to fix this, but it's going to be tricky to get the fidelity right. I tend to think that beyond a week, it's going to be hard to remember what date range you're interested in, so we might just want to have a large bucket for anything older than 3 weeks. Maybe:

Today
Yesterday
Past Week
Past Month
Older

(In reply to comment #3)
> The "calendar" worked very well in Places' early days; you could see the
> history of every day you wished. It was really an impressing feature but it was
> taken out.  

It was also pretty buggy and took up a lot of room. I'd rather do richer things with calendars (like showing page visit density and allowing to zoom in, much like calendar-based UI in photo album apps) but that's extension fodder for now.

Comment 6

[Marco Bonardo [:mak]]

this could be easily fixable with Ondrej's patch in bug 385245, cc-ing him

Mike, is that your final thought in comment 4?

a better thing could be make that dynamically based on current user prefs, using an exponential grow, so if a user chooses 90 days of history should have a different view from a user having 300 days of history or one having 10 days of history. we should have high density near today, and low density near the end of the history.

Comment 7

[Ondrej Brablc]

I just want to confirm, that it is more than easy to do this using bug 385245. We can even have multiple levels like:

By Day
  January
    Monday 1st
    Tuesday 2nd
    ...
  February

You would only see months and days, where you had some visits.

Comment 9

[Marco Bonardo [:mak]]

I'd like to take a look at this as a late-l10n for 3.1.

Ideally i'd like to split the container as:
- Today
- Yesterday
- This week
- This month
- January
- December 2008
- November 2008

and so on, till history is complete.
After splitting history i'd fix also bug 422163, that should give Library History more usefulness.

Mike, any thought on this?

Comment 10

[Marco Bonardo [:mak]]

Attachment: wip 0.1

works, but i need to fix labels and finish fixing test_history_sidebar.
I added This Week and This Month to locale.
I'll need to add a new constant to nsIDateTimeFormat, since i need to get the month name from a PRTime, while actually i can only get YYYY/MM or the full date, will probably be KDateFormatMonthLong.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.0

New separation is:
- Today: from start of today to now
- Yesterday: from start of yesterday to start of today
- 7 days ago: from start of 7 days ago to start of yesterday
- This month: from start of this month to start of 7 days ago
- January: all January visits (being this year, no year in the title)
- December 2008: all December visits (being a past year, year in the title)

Effectively "This week" was a non sense on Sunday and Monday, 7 days ago catches more.
I'm waiting for more feedback.

Problems i need assistance on:
- I only need 1 additional string for "This month", if that's an issue i could use month's name, but from what i got, some string change will be allowed in first week after b3 release, for b4.
- I need to add a constant to nsIDateTimeFormat to get the month's name, so i need to know:
 1. who can review those changes?
 2. is that a problem? IIRC we already broke binary components compatibility in other patches, but want to be sure it's feasible (i can't think another clean way to do that)

Comment 12

[Marco Bonardo [:mak]]

Pike, could you elaborate in previous points, and on best way to show months containers for foreign languages? i'm using "monthname" or "monthname YYYY", could that have bad intercation with eastern languages?

Comment 13

[Axel Hecht [:Pike]]

I don't think this is 3.1 material, just to make that statement first.

Here are a few other thoughts:

- avoid clear buckets. It has been mentioned before that people probably don't remember whether it was June or July last year.
- exponential growth might make sense, something like 'yesterday', 'a few days', 'a few weeks', 'a few months', 'older', where "a few" could be 3 or 4. Going back to point 1, I'd make the queries overlap by 12 hours, a day, a week, a month.
- I'd stay away from calendars. Those suck, might not feel natural in all locales, and are generally painful to localize.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> I don't think this is 3.1 material, just to make that statement first.

well i think it would be a really appreciated change, and quite visible to users. Due to slushy 3.1 release we could take this in early b4 (the decision is up to drivers though).

I couldn't tell what could the user really remember, i see that thinking "was in July" is hard, but also thinking "was 3 months ago" is. I would not be able to tell "i looked at it 3 months ago", but i remember i looked at a site just before christmas, so i can look at December. Do you think this is going too much toward a calendar UI?

Notice the purpose of this bug is not completely rewrite interaction with the sidebar, but only to split history better then actual "older than 6 days ago"
Potentially with actual code i could adapt this to many possibilities, so we only need to find a reasoneable one.

Comment 15

[Marco Bonardo [:mak]]

trybuilds to test the interaction:
https://build.mozilla.org/tryserver-builds/2009-02-27_17:[e-mail]-try-5df62683408/
include this and bug 422163

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 17

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to comment #11)

> - 7 days ago: from start of 7 days ago to start of yesterday
i'd rather expect here "to now" too

> - This month: from start of this month to start of 7 days ago
same here

else looks good & is definetly a progress to current design :)

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> (In reply to comment #11)
> > - 7 days ago: from start of 7 days ago to start of yesterday
> i'd rather expect here "to now" too

well yes could be, the idea here (and in current implementation that i tried not change) is that visits are grouped, so they don't overlap through containers. But really that needs UX team evaluation.

Personally, as a general vision toward making the current implementation better, i would also vote to remove the "by last visited" from the sidebar, because it's damn slow (we are trying to show more than 100 000 entries in the tree), by date can replace it (but we could also replace that with the same query as By Date, but always expanding the Today container, so to avoid the additional click).
And i would replace by most visited with visit count grouped view, where visits are grouped based on biggest number of visits, so something like "More than X visits", "More than X/2 visits", "More than X/4 visits", "visited once pages", "unvisited pages".

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 364655 [details] [diff] [review]
patch v1.1

Hey Marco, I think this is the right idea, but users won't understand our desire to not overlap containers. If we're expressing these containers as queries or filters, they need to match the semantic concept that the user will carry with them.

I think we should be doing:

  Today: from 00h00 until present time
  Yesterday: from 00h00 of previous day until 23:59 of previous day
  Past Week: from 00h00 of 7 days ago until present time
  This Month: from 00h00 of 1st of the month until present time
  <previous month's name>: that month's history
  .
  .
  .

One thing I've noticed is that it takes a long time to open the past week's worth of history. We might want to retain "2 days ago" in order to keep performance up.

Comment 20

[Marco Bonardo [:mak]]

(In reply to comment #19)
> (From update of attachment 364655 [details] [diff] [review])
>   Today: from 00h00 until present time
>   Yesterday: from 00h00 of previous day until 23:59 of previous day
>   Past Week: from 00h00 of 7 days ago until present time

Is "7 days ago" ok, or do we want a new string for "Past Week", i suppose that could have some issue for localization, in italian if i translate past week i think to the past week (sunday to saturday), not the past 7 days. and if "7 days ago" is ok, should it overlap to present time or stop at 2 days ago?

> One thing I've noticed is that it takes a long time to open the past week's
> worth of history. We might want to retain "2 days ago" in order to keep
> performance up.

performances could get better with async favicons patch (bug 481227), btw is "2 days ago" going from 2 days ago 00:00 to 2 days ago 23:59 or to present time?

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #20)
> Is "7 days ago" ok, or do we want a new string for "Past Week", i suppose that
> could have some issue for localization, in italian if i translate past week i
> think to the past week (sunday to saturday), not the past 7 days. and if "7
> days ago" is ok, should it overlap to present time or stop at 2 days ago?

How about "Last 7 days"? And yes, it should overlap. The goal that I have here is to let the user match against these sort of mental constructs:

 - hm, that thing I was at just today ...
 - I was just at that page yesterday ...
 - someone sent me something a few days ago ...
 - it was N months ago ...

The month split up accomplishes the last bit, so I'm trying to figure ways of hitting the first bits.

So, yes, overlap. :)

> performances could get better with async favicons patch (bug 481227), btw is "2
> days ago" going from 2 days ago 00:00 to 2 days ago 23:59 or to present time?

2 days ago isn't in the list. Let's say it's 14:30h on Tuesday. The list should show:

  Today: 0h00 today - 14:30 today
  Yesterday: 0h00 Monday - 23:59 Monday
  Past Week: 0h00 last Tuesday - 14:30 today
  This Month: 0h00 first of the month - 14:30 today
  <previous month's name>: that month's history
  .
  .
  .

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v1.2

fixed beltzner comments, added an "older than 6 months" container per IRC conversation with beltzner.
A trybuild is coming for UI-review, but i'd like to start with reviews on the code.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 367059 [details] [diff] [review]
patch v1.2

asking additional review to Pike for intl and l10n changes

Comment 24

[Marco Bonardo [:mak]]

auto review reminder:
+# This are used to generate history containers when history is grouped by date
nit: These

Comment 25

[Marco Bonardo [:mak]]

a side node to myself: i should check containers deletion, since it now relies on not overlapping containers, most likely i need to special case overlapping ones.

Comment 26

[Marco Bonardo [:mak]]

new trybuilds:
https://build.mozilla.org/tryserver-builds/2009-03-12_09:[e-mail]-try-69d31316094/

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v1.3

i had to change some code, mostly removal is not better since gets limits directly from the query, and query update is simply refreshing the root query, since more containers do overlap. This should fix all of my above comments.

Comment 28

[Marco Bonardo [:mak]]

s/is not better/is NOW better/

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 367154 [details] [diff] [review]
patch v1.3

>+        var queries = {};
>+        PlacesUtils.history.queryStringToQueries(node.uri, queries, {}, {});
>+        var query = queries.value[0];
>+        var beginTime = query.beginTime;
>+        var endTime = query.endTime;
>+        NS_ASSERT(query && beginTime && endTime,
>+                  "A valid date container query should exist!");
>+        // We want to exclude beginTime from the removal
>+        bhist.removePagesByTimeframe(beginTime+1, endTime);
>       }

couldn't you just QI the node to nsINavHistoryQueryResultNode and call getQueries() ?

>diff --git a/intl/locale/idl/nsIScriptableDateFormat.idl b/intl/locale/idl/nsIScriptableDateFormat.idl
>--- a/intl/locale/idl/nsIScriptableDateFormat.idl
>+++ b/intl/locale/idl/nsIScriptableDateFormat.idl
>@@ -39,18 +39,19 @@
> 
> typedef long nsDateFormatSelector;
> %{ C++
> enum
> {
>     kDateFormatNone = 0,            // do not include the date  in the format string
>     kDateFormatLong,                // provides the long date format for the given locale
>     kDateFormatShort,               // provides the short date format for the given locale
>-    kDateFormatYearMonth,           // formats using only the year and month 
>-    kDateFormatWeekday              // week day (e.g. Mon, Tue)
>+    kDateFormatYearMonth,           // formats using only the year and month (e.g. YYYY/MM)
>+    kDateFormatWeekday,             // week day (e.g. Mon, Tue)
>+    kDateFormatMonth                // formats using only the month's name

please replicate the comment change to the scriptable version as well

> nsresult
> PlacesSQLQueryBuilder::SelectAsDay()
> {
>   mSkipOrderBy = PR_TRUE;
>-  PRBool asDayQuery = 
>-    mResultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY;
>-
>-  mQueryString = nsPrintfCString(255,
>+
>+  PRUint16 type =
>+    mResultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ?
>+    nsINavHistoryQueryOptions::RESULTS_AS_URI :
>+    nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY;

nit: resultType would be clearer

>+  nsCOMPtr<nsIDateTimeFormat> dateFormatter;
>+  dateFormatter = do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID);
>+
>+  // These are day containers and catch-all final container.
>+  PRInt32 additionalContainers = 3;

nit: "These are the day containers" for clarity

>+  // We use a guess of the number of months considering all of them 30 days
>+  // long, but we split only the last 6 months.
>+  PRInt32 monthContainers = PR_MIN(6, (history->mExpireDaysMax/30));

handle case of mExpireDaysMax = 0?

>-    PRInt32 fromDayAgo = -i;
>-    PRInt32 toDayAgo = -i + 1;
>+        if (i == additionalContainers + 6) {
>+          // Older than 6 months
>+          history->GetAgeInDaysString(6,
>+            NS_LITERAL_STRING("finduri-AgeInMonths-isgreater").get(), dateName);
>+          // From start of epoch
>+          sqlFragmentBeginTime = NS_LITERAL_CSTRING(
>+            "(datetime(0, 'unixepoch')*1000000)");
>+          // To start of 6 months ago
>+          sqlFragmentEndTime = NS_LITERAL_CSTRING(
>+            "(strftime('%s','now','start of day','-6 months','utc')*1000000)");
>+          break;
>+        }
>+        PRInt32 MonthIndex = i - additionalContainers;
>+        // Previous months title is only month's name if inside this year,
>+        // month's name and year for previous years.

s/months/month's/

>@@ -6136,22 +6164,22 @@ nsNavHistory::FilterResultSet(nsNavHisto
>         nodeIndex > 0 && aSet[nodeIndex]->mURI == aSet[nodeIndex-1]->mURI)
>       continue;
> 
>     if (appendNode)
>       aFiltered->AppendObject(aSet[nodeIndex]);
>       
>     // stop once we've seen max results
>     if (aOptions->MaxResults() > 0 && 
>-        aFiltered->Count() >= aOptions->MaxResults())
>+        (PRUint32)aFiltered->Count() >= aOptions->MaxResults())
>       break;
>   }
> 
>   // de-allocate the matrixes
>-  for (PRUint32 i=0; i < aQueries.Count(); i++) {
>+  for (PRUint32 i=0; i < (PRUint32)aQueries.Count(); i++) {
>     delete terms[i];
>     delete includeFolders[i];
>     delete excludeFolders[i];
>   }

why PRUint32? could be PRInt32 right? (also goes for the other instances in this patch)

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -4278,117 +4278,23 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI

>-      nsRefPtr<nsNavHistoryQueryResultNode> siteNode;
>-      siteNode = new nsNavHistoryQueryResultNode(host, EmptyCString(), queryUri);
>-      rv = siteRoot->GetAsContainer()->InsertSortedChild(
>-               siteNode, PR_FALSE, PR_TRUE/*Ignore duplicates*/);
>-      NS_ENSURE_SUCCESS(rv, rv);
>-    }
>+        resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
>+        resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY)
>+      mRootNode->GetAsQuery()->Refresh();

will this maintain expansion state?

>+/**
>+ * Fills history and checks constainers' labels.
>+ */

typo: containers

and elsewhere in this file

>diff --git a/toolkit/locales/en-US/chrome/places/places.properties b/toolkit/locales/en-US/chrome/places/places.properties
>--- a/toolkit/locales/en-US/chrome/places/places.properties
>+++ b/toolkit/locales/en-US/chrome/places/places.properties
>@@ -1,18 +1,25 @@ BookmarksMenuFolderTitle=Bookmarks Menu
> BookmarksMenuFolderTitle=Bookmarks Menu
> BookmarksToolbarFolderTitle=Bookmarks Toolbar
> UnsortedBookmarksFolderTitle=Unsorted Bookmarks
> TagsFolderTitle=Tags
> 
>+# LOCALIZATION NOTE
>+# These are used to generate history containers when history is grouped by date

the usage pattern seems to be to put the string names in parentheses after LOCALIZATION NOTE. however, i don't know if this is for a tool, or just for clarity. ask Axel?

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v1.4

a side note, mac date formatting has just changed (Bug 360018) i don't know if that code will make 3.5, btw i've updated my code so i can create 2 diff versions for trunk and branch. Also while there i noticed they changed the short date from YYYY/MM to YY/MM that looks wrong to me, i've changed that and made them aware of the change.
But on the other side i can't compile on Mac so i'm not sure my Mac changes do compile fine, Dietrich could you make a compile of intl with my patch please?

(In reply to comment #29)
> (From update of attachment 367154 [details] [diff] [review])
> >+  // We use a guess of the number of months considering all of them 30 days
> >+  // long, but we split only the last 6 months.
> >+  PRInt32 monthContainers = PR_MIN(6, (history->mExpireDaysMax/30));
> 
> handle case of mExpireDaysMax = 0?

I don't think there's need to, in such a case we won't create month containers, and alsi expireDaysMin will be 0 so history is disabled (if instead min > max we set max to min value)

> >diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
> >--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
> >+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
> >@@ -4278,117 +4278,23 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI
> >+        resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY)
> >+      mRootNode->GetAsQuery()->Refresh();
> 
> will this maintain expansion state?

yes

> >diff --git a/toolkit/locales/en-US/chrome/places/places.properties b/toolkit/locales/en-US/chrome/places/places.properties
> >--- a/toolkit/locales/en-US/chrome/places/places.properties
> >+++ b/toolkit/locales/en-US/chrome/places/places.properties
> >+# LOCALIZATION NOTE
> >+# These are used to generate history containers when history is grouped by date
> 
> the usage pattern seems to be to put the string names in parentheses after
> LOCALIZATION NOTE. however, i don't know if this is for a tool, or just for
> clarity. ask Axel?

He can directly reply here since i'm asking a review to him.

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

you need review from someone else for the /intl changes. r=me otherwise.

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

hope jtd is the right person to ask review on intl changes.

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

r=me with nits.

Some tools use the () stuff in localization notes, so please follow the scheme as documented on https://developer.mozilla.org/En/Localization_notes.

I'm forwarding the other concern of mine to beltzner for UI review, we're mixing OS-locale-month-names together with app-locale "This month". mak said on irc that we're doing this elsewhere, though I'm not sure if this close. Your call if that's OK or if we should just add the month names to some .properties in toolkit and use those.

Comment 34

[Marco Bonardo [:mak]]

exactly we are using scriptableDateFormat in other places to format the date, and it takes months' names from the OS, clearly those depend on OS locale.
We can force localizers to provide 12 strings for all months clearly (that would also avoid me an additional intl review), but i suppose a user understands the locale his OS is setup to.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

uir=beltzner, me wrikey!

Comment 36

[Simon Montagu :smontagu]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

Stealing intl review request...

Comment 37

[Simon Montagu :smontagu]

Comment on attachment 367698 [details] [diff] [review]
patch v1.4

(In reply to comment #34)
> exactly we are using scriptableDateFormat in other places to format the date,
> and it takes months' names from the OS, clearly those depend on OS locale.
> We can force localizers to provide 12 strings for all months clearly (that
> would also avoid me an additional intl review), but i suppose a user
> understands the locale his OS is setup to.

No, I think in this case we should use app-localized month names and not OS date formatting. I don't think that the argument for using the OS format in dates that appear *as dates* applies here where the month name is appearing as text on its own (or even with the year).

FWIW, l10ns that have localized calendar already have localized month names in dateFormat.properties.

Comment 38

[Axel Hecht [:Pike]]

http://mxr.mozilla.org/l10n-central/find?text=&kind=text&string=dateFormat.properties would be the mxr search for dateFormat.properties.

Comment 39

[Axel Hecht [:Pike]]

... to clarify, which we can't use in Firefox, unless we copy/move dateFormat.properties to toolkit. I don't think it'd make sense to create a second copy in browser.

Comment 40

[Marco Bonardo [:mak]]

Attachment: patch v2.0

based on suggestion from smontagu and Pike i've copied dateFormat.properties to toolkit, if this gets approved from a toolkit peer and l10n, i'll then file a bug against calendar to drop their file.

So now i need, a quick check from dietrich for small changes i've done to get months' names, a tookit peer review and approval for the additional locale file, and a l10n review.

Comment 41

[Dietrich Ayala (:dietrich)]

Comment on attachment 367987 [details] [diff] [review]
patch v2.0

>+void
>+nsNavHistory::GetMonthName(PRInt32 aIndex, nsACString& aResult)
>+{
>+  nsIStringBundle *bundle = GetDateFormatBundle();
>+  if (!bundle)
>+    aResult.Truncate(0);
>+
>+  nsCString name = nsPrintfCString("month.%d.name", aIndex);
>+  nsXPIDLString value;
>+  nsresult rv = bundle->GetStringFromName(NS_ConvertUTF8toUTF16(name).get(),
>+                                          getter_Copies(value));
>+  if (NS_SUCCEEDED(rv))
>+    CopyUTF16toUTF8(value, aResult);
>+  else
>+    aResult.Truncate(0);
>+}

if !bundle, shouldn't there be an early return or an |else| block after that? (else preferred)

>+nsIStringBundle *
>+nsNavHistory::GetDateFormatBundle()
>+{
>+  if (mDateFormatBundle)
>+    return mDateFormatBundle;
>+
>+  nsCOMPtr<nsIStringBundleService> bundleService =
>+    do_GetService(NS_STRINGBUNDLE_CONTRACTID);
>+  NS_ENSURE_TRUE(bundleService, nsnull);
>+  nsresult rv = bundleService->CreateBundle(
>+      "chrome://global/locale/dateFormat.properties",
>+      getter_AddRefs(mDateFormatBundle));
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>+
>+  return mDateFormatBundle;
> }

nit: instead of two returns, please just wrap work in if(!mDateFormatBundle) { ... }

r=me with these changes

Comment 42

[Simon Paquet [:sipaq]]

(In reply to comment #40)
> Created an attachment (id=367987) [details]
> patch v2.0
> based on suggestion from smontagu and Pike i've copied dateFormat.properties 
> to toolkit, if this gets approved from a toolkit peer and l10n, I'll then 
> file a bug against calendar to drop their file.

CC'ing Philipp (Calendar lead dev) here. Philipp, Firefox devs plan to copy dateFormat.properties to toolkit. Do we have a problem with that? 

An advantage I see is that we would share the maintenance of that file. 

A potential downside would be that this file would underlie a different/longer release cycle and that we couldn't change strings in this file at our leisure. On the other hand, this file has been pretty static in the past (last real content change in August 2007), so this might not be an issue at all.

Comment 43

[Marco Bonardo [:mak]]

Attachment: patch v2.1

fixed dietrich's comments

I got a sort of approval from calendar guys and philipp in #calendar.
At this point i practically need a toolkit peer approval.

Comment 44

[Mike Connor [:mconnor]]

a=me on moving this into toolkit.  Really really not sure about throwing this at localizers now, but on the other hand it doesn't look hard to do at one fell swoop.  Please keep string freeze in mind though.

(Remove the "why aren't we counting from zero" comment though, it seems pretty obvious that for localizer-facing bits, zero-based counting is likely to cause confusion.)

Comment 45

[Marco Bonardo [:mak]]

Comment on attachment 368070 [details] [diff] [review]
patch v2.1

clearing reviews per approval, i discussed with Galdalf and Enn and situation was in a quite good shape.

Comment 46

[Marco Bonardo [:mak]]

Attachment: patch v2.1 - 1.9.1 friendly

this is an unbitrot against 1.9.1 branch.

Comment 47

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5413fb8a2a43

I won't be here tomorrow, so please Dietrich or Shawn or anybody, take care of pushing this to branch before the freeze if everything is ok on trunk.

Comment 48

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/4e1a4c23041c

backed out due to unit test failures on all platforms:

*** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell/test_places/unit/test_history_sidebar.js | Yesterday == Today
JS frame :: /Users/dietrich/moz/trunk/mozilla/testing/xpcshell/head.js :: do_throw :: line 129
JS frame :: /Users/dietrich/moz/trunk/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 148
JS frame :: ../../../../_tests/xpcshell/test_places/unit/test_history_sidebar.js :: fill_history :: line 123
JS frame :: ../../../../_tests/xpcshell/test_places/unit/test_history_sidebar.js :: run_test :: line 266

Comment 49

[Dietrich Ayala (:dietrich)]

i think there are problems with the calls to sqlite's strftime function:

1. 'now' is already UTC, so no need to pass the 'utc' modifier. doing so does modify the time even further (which seems like a footgun, imo).
2. using 'utc' does the opposite of what we need. it assumes the value passed to it is non-utc and needs to be converted. we want the value in local time for comparison with history visit times, so need to pass the 'localtime' modifier instead of 'utc'.

doing s/utc/localtime/ in the strftime() calls got the sidebar working again, but the test still failed.. but then suddenly started passing after midnight rolled around! (that really messed with my head, btw)

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we want to split out a strings-only patch here to ensure we hit string freeze, or can someone take ownership of the entire patch and try to see it through?

Comment 51

[Dietrich Ayala (:dietrich)]

Attachment: strings only (trunk)

Comment 52

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #42 and #43)
> An advantage I see is that we would share the maintenance of that file. 
> 
> A potential downside would be that this file would underlie a different/longer
> release cycle and that we couldn't change strings in this file at our leisure.
> On the other hand, this file has been pretty static in the past (last real
> content change in August 2007), so this might not be an issue at all.

The same reasoning came to my mind. I agree that the file has been pretty static. What just came to my mind, we might need changes to this file with bug 482460, but I think we can get away with putting the resources in a different file. The problem described in the mentioned bug is also worth looking at for toolkit, but I think that can wait until we've made experience.

To make things short, I agree we should move this file to toolkit.

Comment 53

[Alexander L. Slovesnik]

(In reply to comment #51)
> Created an attachment (id=368275) [details]
> strings only (trunk)
finduri-AgeInDays-is=%S days ago
finduri-AgeInDays-last-is=Last %S days
finduri-AgeInDays-isgreater=Older than %S days
finduri-AgeInMonths-isgreater=Older than %S months

Are there any reasons that these strings don't use plural forms ( https://developer.mozilla.org/En/Localization_and_Plurals )? Without plural forms it's hard to translate them properly.

Comment 54

[Dietrich Ayala (:dietrich)]

(In reply to comment #53)
> Are there any reasons that these strings don't use plural forms (
> https://developer.mozilla.org/En/Localization_and_Plurals )? Without plural
> forms it's hard to translate them properly.

hrm, yes they should be. however, the existing strings aren't either and have been there since 3.0 (and before?), so i don't think we should block on it.

i'll file a new bug for this.

Comment 55

[Dietrich Ayala (:dietrich)]

filed bug 484252 for that

Comment 56

[Dietrich Ayala (:dietrich)]

landed strings on trunk: http://hg.mozilla.org/mozilla-central/rev/15c21cc6ef8b

Comment 57

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368275 [details] [diff] [review]
strings only (trunk)

a191=beltzner

Comment 58

[Dietrich Ayala (:dietrich)]

landed strings on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ad1ec18f2ff0

Comment 60

[Marco Bonardo [:mak]]

(In reply to comment #53)
> (In reply to comment #51)
> > Created an attachment (id=368275) [details] [details]
> > strings only (trunk)
> finduri-AgeInDays-is=%S days ago
> finduri-AgeInDays-last-is=Last %S days
> finduri-AgeInDays-isgreater=Older than %S days
> finduri-AgeInMonths-isgreater=Older than %S months
> 
> Are there any reasons that these strings don't use plural forms (
> https://developer.mozilla.org/En/Localization_and_Plurals )? Without plural
> forms it's hard to translate them properly.

because them are intended to always be plural. We never use them with 0 or 1, those are special cased if needed (eg. today, yesterday)

Thanks for landed strings

Comment 61

[Axel Hecht [:Pike]]

Marco, please read the mentioned page on MDC to learn that plural is a slightly more complex thing.

The real reason why we haven beaten Marco to it is that he needs the strings from C++, and the plural logic is only exposed as a js module, aka js-only. That's why we can't do a whole lot better than doing the follow-up bug 484252 (see comment 55).

Comment 62

[Marco Bonardo [:mak]]

Attachment: patch v2.2

what we really need to to is calculate midnight based on localtime, but then convert it to an utc timestamp (what we put in the db), the change is quite trivial so not asking a further review. Tested on all timezones and with DST changes, i'm going to push on central for baking.

Comment 63

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/40f19b310ca5

Comment 64

[Vlado Valastiak [:wladow]]

Based on previous comments and landing dateFormat.properties file, shouldn't we get month names from dateFormat.properties? Currently they are taken most likely from OS, in other words not from dateFormat.properties

Comment 65

[Marco Bonardo [:mak]]

what makes you think they're taken from OS? they are taken from dateFormat.properties in ::GetMonthName

Comment 66

[Vlado Valastiak [:wladow]]

Forgot to mention UA: Mozilla/5.0 (Windows; U; Windows NT 6.0; sk; rv:1.9.2a1pre) Gecko/20090324 Minefield/3.6a1pre

I noticed that while testing my localization: month names (in the History sidebar) are all in lowercase, although they are defined with capitals in dateFormat.properties file. To be sure I changed month.2.name to 'XXX', but the month name was still 'february' (in slovak).

Comment 67

[Marco Bonardo [:mak]]

there's no other place we could take months' names since initial intl changes have not landed, so those can only come from dateFormat.properties.

Comment 68

[Vlado Valastiak [:wladow]]

Uhm, this is really strange, but with a fresh new profile it works as expected. Ok then, sorry for the noise.

Comment 69

[Marco Bonardo [:mak]]

Comment on attachment 368877 [details] [diff] [review]
patch v2.2

Tests are still correctly passing and users on trunk seem to like the change, asking approval.
Risk is medium but comes with automatic test.

Comment 71

[Mike Connor [:mconnor]]

Comment on attachment 368877 [details] [diff] [review]
patch v2.2

awesome.

Comment 72

[Zibi Braniecki [:zbraniecki][:gandalf]]

We'd love to have a litmus test for folks to actually test their localization of those pre-landed strings in action.

Comment 73

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/560358d258af

Comment 74

[Bill Coleman]

I have mixed feelings about this fix. I think splitting the older than 6 days folder is great, but I do not like the fact that the previous week has combined into one folder. I preferred having a separate folder for 3 days ago, 4 days ago, etc. It was especially useful when I was checking to see if Weave synch on history was working correctly from computer to computer.

Comment 75

[Alex Faaborg [:faaborg] (Firefox UX)]

Ideally the answer to that question would so consistently be yes that you wouldn't need to check.

Comment 76

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre

Comment 77

[David Dahl :ddahl]

Just a note about the approach taken here. I know we may be stuck with the existing schema through Firefox 3.5.x, but this is CLASSIC "data mart" / data warehouse star schema territory. A problem that is very much solved. 

A star schema will allow for very simple (read: fast) queries to generate reports like this. As we proceed to Firefox.next we should create a new schema that actually makes any kind of "drill down" into the history and bookmarks easy.

Comment 78

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv