As far as I can tell, places does not already have a way to group results by date. need GROUP_BY_DATE and GROUP_BY_DATE_AND_SITE for history sidebar, for View | By Date and View | By Date and Site (to achieve parity with Fx 2) see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#934

This is correct. It used to be in there but I removed it in a big refactor when I broke it and realized we didn't need it. This should be easy to add to the grouper since other grouping is more complicated. Once you write a day grouper, you can magically do group by day the by host, for example.

I should clarify, don't write a GROUP_BY_DAY_AND_SITE flag. The grouping mode is an array of grouping flags. Specify [GROUP_BY_DAY, GROUP_BY_DOMAIN] to get this. HOST would be exactly what the old system uses, but DOMAIN is much more useful, modulo the incomplete TLD list which has to be finished anyway.

brettw, thanks for the infomation. Perhaps for backwards compatibility, we should use GROUP_BY_HOST for Fx 2 style group "by date and site" and "by site", but add two more: "by date and domain" and "by domain" to take advantage of the more useful GROUP_BY_DOMAIN. Or is that just bloat? I'll spin off a new bug about that, and cc you. > modulo the incomplete TLD list which has to be finished anyway are you referring to bug #319643?

I think the current dropdown already has too many things and I have to think for a surprisingly long time about what each one does before I select it. For example, what's the difference between "by date" and "by last visited" without trying it? I don't see how people are supposed to decide they want host or domain grouping. If anything, grouping modes should be removed, not added.

whoops, instead of this hackery: +// XXX todo, add this for winstripe +// XXX todo, what about open / closed state for winstripe (Fx 2.0 on pinstripe doesn't do it.) +#define FOLDER_ICON_URI "chrome://browser/skin/places/folder.png" + what I want to do is add: <?xml-stylesheet href="chrome://browser/skin/places/places.css"?> to history-panel.xul, which has the css to style containers, and fix spacing issues, etc. new patch coming.

it turns out, I don't need to pass around the time, because nsNavHistoryContainerNode::FillStats() will populate the container (ex: "Today") with the time of the node, if the node's time is > than the container time. I'll remove that part of my patch, and then add a comment to nsNavHistoryContainerNode::FillStats() to explain that.

note, once the fix for bug #356487 lands, the additional patch needed for this will be to remove the comments around "group" line of code in history-panel.js + else if (gHistoryGrouping == "dayandsite") { + /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */ + placeURI += "&group=" + NHQO.GROUP_BY_HOST; + placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING; + } + else { + /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */ + placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING; + }

from another bug, brettw says: I realized you should be doing this grouping differently than you are, as it will give poor performance grouping every single item in history (though possibly not worse than the Mork one). You should have some root thing (probably this is hardcoded somewhere for now, in the original design it would be a bookmark folder) that contains queries. These queries are for "Today", "Yesterday", etc... Querying by time is *very* fast. This prevents you from having to schlep *all* of history into memory and sort it and group it. Only those folder that the user has opened will be populated and kept up-to-date, which will happen on demand. Initial population will be instant.

Comment on attachment 244544 [details] [diff] [review] add comment about bug #359346 clearing r= request, due to brettw's comment.

note, the last patch really implements GROUP_BY_DAY, not GROUP_BY_DATE (although if we want to do IE 7 style grouping, or even generic date grouping, GROUP_BY_DAY won't be accurate). additionally, see brettw's comment #16

this just the old patch (updated to the trunk), and is not brettw's suggestion.

brett also writes: > Your group by date implementation is probably fine for now, since it gives > parity. You can worry about performance later. Just realize that this really > needs to be changed before shipping. based on that comment, I'll seek reviews on my implementation, and log a spin off bug about his suggested optimization.

Can you please rewrite GetAgeInDays using "regular" math. I am pretty sure it is legal to use 64-bit math nowadays because all our compilers support it, so we don't have to have these difficult to read LL_ macros. Also, this code is very confusing (I actually rewrote it once, but I can't find it). There are errors in it that happen to cancel each other out to give the correct answer. The author was confused and thought that PRTime was in msecs when it is in usecs. For example, LL_DIV(ageInDays, ageInSeconds, msecPerDay); but days != seconds / (msecs/day)! In GetAgeInDays, I think the hashtable stuff is unnecessary. You first compute the age in days, clamp it to your range of 0-8, then convert it to a string which you look up in a hashtable. Also, NUM_DAYS should be a const int which is scoped and typed correctly, and will be optimized the same. Then you have: const int numDays = 8; nsNavHistoryContainerResultNode* dates[numDays]; for (int i = 0; i < numDays; i++) dates[i] = NULL; Then you can do the same thing you always did with lazily creating the categories without all those string copies and hash computations.

thanks for the review, brett. I'll work on a new patch and seek another review.

> Can you please rewrite GetAgeInDays using "regular" math. the new version (in my last patch) is based on the simplified version of code from mozilla/xpfe/components/history/src/nsGlobalHistory.cpp > In GetAgeInDays, I think the hashtable stuff is unnecessary. I've re-written it per your suggestion. My previous attempt was based on how GroupByHost() works, but with a fixed number of days, it was overkill.

Comment on attachment 244966 [details] [diff] [review] updated patch, per brettw's comments Looks good.

Comment on attachment 244966 [details] [diff] [review] updated patch, per brettw's comments seeking browser / toolkit peer review from gavin.

from irc, gavin spots two issues: 1) can't you just use PRInt64 types directly? (following up on brettw's original comment, about removing the LL_ macros) and, 2) int -> PRInt32 and attach a new patch.

still need LL_INIT(), but other LL_ removed, and GetAgeInDays() now returns PRInt64

to be specific, if I do: static const PRInt64 USECS_PER_DAY = PR_USEC_PER_SEC * 60 * 60 * 24; we'd get the following warning on windows: c:/builds/trunk/mozilla/toolkit/components/places/src/nsNavHistory.cpp(3302) : w arning C4307: '*' : integral constant overflow this is why I'm using the LL_INIT() macro

Comment on attachment 245179 [details] [diff] [review] updated patch, per gavin's comments Just a few little nits, feel free to ignore them as you deem necessary. >+ for (PRInt32 i = 0; i < numDays; i++) >+ dates[i] = NULL; s/NULL/nsnull/ ? >+ GetAgeInDaysString(numDays-2, >+ NS_LITERAL_STRING("finduri-AgeInDays-isgreater").get(), >+ dateNames[numDays-1]); nit: fix indent? >+ if (ageInDays >= (numDays - 1)) >+ ageInDays = numDays - 1; Looks like that should be just > and not >=. >@@ -693,16 +696,26 @@ PRInt32 PR_CALLBACK nsNavHistoryContaine > PRUint32 aType, bType; > a->GetType(&aType); > b->GetType(&bType); > > if (aType != bType) { > return bType - aType; > } > >+ if (aType == nsINavHistoryResultNode::RESULT_TYPE_DAY) { >+ // for the history sidebar, when we do "View | By Date" or >+ // "View | By Date and Site" we sort by SORT_BY_TITLE_ASCENDING, >+ // so to make the day container show up in >+ // desire order, we need to compare by time, instead of title >+ // hard coding this isn't ideal, but we can't currently have >+ // one sort per grouping. see bug #359332 on that issue. >+ return -ComparePRTime(a->mTime, b->mTime); This comment is a little hard to read. I think it needs a period after "title", and "desire" needs a "d".

> s/NULL/nsnull/ ? fixed, thanks g#. > nit: fix indent? fixed, thanks g#. > Looks like that should be just > and not >=. good point, since if ageInDays == numDays - 1, then assigning ageInDays to numDays - 1 is a noop. fixed, thanks g#. > This comment is a little hard to read. I think it needs a period after "title", and "desire" needs a "d". fixed the typos, and tried to make it a little more readable. thanks again, g#. I'll attach the final patch, and check in.

carrying over r=g#,brettw

fixed. thanks again gavin and brettw for the reviews and suggestions. Checking in browser/components/places/content/history-panel.js; /cvsroot/mozilla/browser/components/places/content/history-panel.js,v <-- history-panel.js new revision: 1.2; previous revision: 1.1 done Checking in toolkit/components/places/public/nsINavHistoryService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v <-- nsINavHistoryService.idl new revision: 1.48; previous revision: 1.47 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.102; previous revision: 1.101 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.68; previous revision: 1.67 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- nsNavHistoryResult.cpp new revision: 1.78; previous revision: 1.77 done Checking in toolkit/components/places/src/nsNavHistoryResult.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v <-- nsNavHistoryResult.h new revision: 1.27; previous revision: 1.26 done

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