385245 - history sidebar very slow (way slower than fx2)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

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

history sidebar very slow for last visited (slower than fx2 ?)

ria wrote:  "history By Last Visited takes 35 seconds."

I'm seeing perf problems as well.

a couple of things:

1)  we might be loading the tree again twice now, see bug #373944 and bug #385208.  I'll check into that and back out the fix for bug #373944 until we have a fix that doesn't regress performance.

2)  besides that, I know we spend a lot of time trying to figure out if we can collapse duplicates.  I think we can do this work ahead of time, before we return results to the view.

Comment 1

[Ria Klaassen (not reading all bugmail)]

On branch the same action takes only 1 second. Maybe a solution would be to display in the "By Last Visited" view only the visits of the last 3 days? Then there could by a link on the bottom that points to the next 3 days etc.

Comment 2

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

> On branch the same action takes only 1 second.

Thanks Ria.  Updating summary.

> Maybe a solution would be to display in the "By Last Visited" view only the 
> visits of the last 3 days?

While we could work around the performance issues by changing the UI, I don't think we should do that.  I think we have some real performance problems to tackle.

There are definitely some performance gains to be made in 
_buildVisibleSection().  For example, we shouldn't bother attempting collapse duplicates if none of the results are visits.

But even if _buildVisibleSection() did as little work as possible, we are still going to be slower than fx 2.  I claim we are paying a price for how often we
are crossing the xpconnect boundary.

I'm going to attach a small patch that I have in my local tree, but save the
performance work for after A6.

Comment 3

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

Attachment: wip patch (not ready for review)

Comment 4

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

it is slow for other history sidebar views (like most visited) when you have lots of data.

right now, all our history sidebar views as for the results as visits.  but perhaps we can ask for results for view like "By Site" by URI (and not visits), and eliminate are need to collapse duplicates

I hope to come back to this later, but for now, assign to nobody.

Comment 5

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

another comment, there are optimizations we can make when searching.

for example, searching could as for results by URI (and not visits), and then we could skip checking separators and skip collapsing duplicates and skip checking for containers)

we could do the same (skip checking separators and and skip checking containers) for bookmark searches as well.

I might get to land this stuff for A6, for bug #333965.

Comment 6

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

Attachment: updated patch, wip

The goal of this patch is to spend as little time as possible in _buildVisibleSection(), as it is expensive.

todo items:

1) not sure if for last visited we should be using type=0 (RESULTS_AS_URI), see change to controller.js.  we may need to override the resultType when last visited view (to be RESULTS_AS_VISIT), unless we are searching.  I think the other views are ok with RESULTS_AS_URI.  Need to check fx2 parity.

2)  For last visited and most visited, when not searching, I'm not getting updated when an item changes (and should move positions in the view).  for when searching or other views, I get those updates.

Comment 7

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

Attachment: updated patch (last patch showed separators in the bm window when sorted)

Comment 8

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

Attachment: updated patch

Comment 9

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

some notes:

in addition to being slow, some of our queries are not correct w.r.t. fx 2 parity.

group by (date and site) and (date) should be RESULT_AS_VISIT, but site, most visited and last visited to be RESULT_AS_URI.  (note, our history menu already does RESULT_AS_URI, which should match the same as the top 10 in last visited)

Here's the query for last visited (we might be able to optimize this)

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, visit_date,
f.url, null, null
FROM moz_places h
LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id
LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id
WHERE h.hidden <> 1 and v.visit_type <> 4
GROUP BY h.id ORDER BY 6 DESC

when filtering, we should also do RESULT_AS_URI and sort by title.

additionally, I think we should remove the collapse duplicate code from treeView.js and do it in C++ (in nsNavHistory.cpp) for both performance reasons and to fix bugs like #386399

I'll attach my update work in progress patch.

Comment 10

[Marco Bonardo [:mak]]

i get better performance with this:

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, 
MAX(visit_date) as visit_date, f.url, null, null
FROM moz_historyvisits v
JOIN moz_places h ON h.id = v.place_id
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE h.hidden <> 1 and v.visit_type <> 4
GROUP BY v.place_id 
ORDER BY visit_date DESC

Comment 11

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

as we've been discussing in bug #381898, we should be doing:

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date),
f.url, null, null
FROM moz_places h
LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id
LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id
WHERE h.hidden <> 1 and v.visit_type <> 4
GROUP BY h.id 
ORDER BY 6 DESC

or

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, 
MAX(visit_date) as visit_date, f.url, null, null
FROM moz_historyvisits v
JOIN moz_places h ON h.id = v.place_id
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE h.hidden <> 1 and v.visit_type <> 4
GROUP BY v.place_id 
ORDER BY visit_date DESC

Comment 12

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

By "we should be doing", I mean instead of what I had in comment #9 or what you have in comment #10.  (I was missing the MAX(visit_date) and as seen in bug #381898 we can't order by visit_date, we need to order by the column number or do "MAX(visit_date) as foo" and then do "ORDER BY foo DESC".

We may be able to come up with an even faster query, using CREATE VIEW as you propose in bug #381898 comment #10

Comment 13

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

Attachment: patch

Comment 14

[Brett Wilson]

I don't see how you can get any kind of reasonable performance out of the current UI.

First, I don't see why you need parity with Fx2 here. The old history sidebar was terrible. While I suggested getting approximate parity first before working on new features, it looks to me like this is currently being done at the expense of any new features. Unless you are providing a better experience with places, why risk ripping everything out?

Second, the old sidebar was designed for having a small history that fits in memory where it is reasonable to do everything brute-force. With the longer histories you are attempting to enable, this is no longer possible. The view you have is all your history period. How can you get any kind of performance making a copy of the entire history database and sorting in the view, especially when you have 180 days of history and also want to update it in real time?

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

It really seems like you need to page your data here. The original Places UI did this by restricting queries to date ranges. You could do this for rows ordered by date (or whatever the natural sort order is) by receiving notifications from the treeview as the user scrolls. 

However for sorted data it's difficult, because if your history has 150k entries, you need to sort that, right?

How useful is a scrolling row-based list over 2 years of data anyway? It seems like you want to keep the working set relatively small, and give the user the ability to jump from chunk to chunk.

Comment 16

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

brett and ben, thanks for jumping in here.  I think you guys are right on the money.

With a huge history, the current ui will not have reasonable perf (brett) and will not be usable (ben)

See bug #387730 (and http://wiki.mozilla.org/Places:User_Interface/Viewing_Tagged_Pages) for a tracking bug for fixing the history sidebar UI.

Instead of working on squeezing perf out of the fx 2 style history sidebar, I'll wait for (or help with) the desired fx 3 style sidebar.

Comment 17

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

myk ran into this recently, and did a systrace on the firefox process and noticed a ton of this:

access("/var/tmp/etilqs_42Uja56RNhEkNfX", F_OK) = -1 ENOENT (No such file or directory)
stat64("/var/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=4096, ...}) = 0
access("/var/tmp", R_OK|W_OK|X_OK)      = 0

looking at the sqlite source, these etilqs_* files are temp files, and we might be creating them as we create page caches.

Comment 18

[Denis Saintilma]

this bug was fixed recently https://bugzilla.mozilla.org/show_bug.cgi?id=398896

Comment 19

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

> this bug was fixed recently https://bugzilla.mozilla.org/show_bug.cgi?id=398896

Denis, the original bug was logged before we had tagging.

Comment 20

[Dietrich Ayala (:dietrich)]

pushing to M10.

Comment 21

[Mike Schroepfer]

So this is set for M10 but blocks bug 333965 which is blocking M9.   Which are we planning on doing?

Comment 22

[Jesse Ruderman]

It takes 1-2 seconds to open the history sidebar in my recent trunk debug build of Firefox.

Comment 23

[coth]

29 seconds here between clicking on Show All History and window appearance. It's not on fresh start, but after several hours of extensive usage, so large portion of database was cached. Places window lags a lot, when history is open.

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b2pre) Gecko/2007112805 Minefield/3.0b2pre

Comment 24

[Ondrej Brablc]

Is there any reason why the history in the **sidebar** is build different way than the one in the "Show All History" **window**? The sidebar version gets all the places and all their visits. The window version gets just the places and their last visit date. My understanding is that this should be fine for the sidebar too whatever we want to do with it. 

With the bug #392399 we are about to speed up the window query (by factor of 20). Should we change the sidebar version too?

BTW, there is something wrong with the window version. It takes 2 seconds to reorder my 2500+ places, although it does not look into database. In Thunderbird reordering my 5500+ sent mail is done immediately.

And another point, I saw, that favicons are loaded one by one. Now the original query just populates URLs and data is populated later. If we are displaying entire history, we should be either loading the data and mime type with the original query (this would slow down the query even more) or I we should first display the bookmarks, then preload all favicon data with one cursor and update the view. Did anyone check what is the difference in performance without favicons?

Comment 25

[Marco Bonardo [:mak]]

in the sidebar the user can choose how to group visits (by date, by site, by site and dates, order by visit_date, etc)
while in the organizer there is an entry for every place, ordered by last visit date

i don't think that the sidebar can be optimized at all in bug #392399 since it uses simple visit_date and not grouping

Comment 26

[Brett Wilson]

The optimization for the sidebar is to make the default group by date and have the sidebar folder a folder of queries, one for each time range. I discussed this on IRC like 6 months ago, but it seems to have gotten lost. There is then no penalty for having date ranges that are not open.

This will not help some modes like global ordering by date, but I argue those should be removed or reworked so they can be expressed efficiently as smaller queries rather than all of history.

Comment 27

[Ondrej Brablc]

I have been doing some tests and the performance of the sidebar is really very slow even for mid size database. For big database it is unusable (ffx in debug mode freezes for tens of seconds or couple of minutes).

I have put together some draft analysis based on my tests and initial suggestions for the changes: 
http://wiki.mozilla.org/User:Ondrej/Bugs/385245
 

Comment 28

[Michael Adams]

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2007122815 Minefield/3.0b3pre

Using history data collected from 3.0b2 and my current installed version dating back more than 6 days: performance doesn't seem to be bad: then again I'm running an AMD X2-4200+ based system.

Comment 29

[Ondrej Brablc]

Attachment: Grouping replaces with multiple queries

This is a draft patch, it has some TODOs to be done, but is fully functional and works very fast on a big profile (22MB - 2500+ places, 250000+ visits).

What has been done
------------------
Before this patch, visits were read all from the database and then processed according to the desired grouping "By Date", "By Site", ... On big database, this could last long seconds or even minutes. This patch returns (whenever possible) "place:" queries instead. They are executed only when user clicks on them, what results in much less data reading operations and much better response times.


What is still missing
---------------------
1) The icon for all queries shows "query.png". This was previously used for smart queries only and should be changed to the normal folder icon.
2) There seems to be a sorting issue for "By Site", somehow the first and last item get swapped.
3) I have checked all the functionality of the history sidebar, but not any possible regression with bookmark queries (I will do this ASAP).

Comment 30

[coth]

Ondrej Brablc, 22MB is very small profile. Big it's when 100MB+.

Comment 31

[Ondrej Brablc]

22MB is just the size of my places.sqlite (basically just moz_places and moz_historyvisits), not of the whole profile. Here are some numbers I have just taken for the comparison:

"By Date and Site"  28 secs -> less than 1 secs
"By Site"           22 secs -> 2 secs
"By Date"           30 secs -> less than 1 secs
"By Most Visited"  3.5 secs = 3.5 secs
"By Last Visited"  3.5 secs = 3.5 secs

Most and Last Visited have not been improved at all, here it would help to load data on demand as the user scrolls down. Or to show the data in some groups.

If necessary, I can create bigger places.sqlite in no time.

Comment 32

[Mike Schroepfer]

(In reply to comment #31)
> 22MB is just the size of my places.sqlite (basically just moz_places and
> moz_historyvisits), not of the whole profile. Here are some numbers I have just
> taken for the comparison:
> 
> "By Date and Site"  28 secs -> less than 1 secs
> "By Site"           22 secs -> 2 secs
> "By Date"           30 secs -> less than 1 secs
> "By Most Visited"  3.5 secs = 3.5 secs
> "By Last Visited"  3.5 secs = 3.5 secs
> 
> Most and Last Visited have not been improved at all, here it would help to load
> data on demand as the user scrolls down. Or to show the data in some groups.
> 
> If necessary, I can create bigger places.sqlite in no time.
> 

I think Seth S. has a really big Places profile he's been using to test - but these results are very encouraging...

Comment 33

[coth]

200MB+ / 300000+ records in moz_places should be working somehow OK as well, as a lot of users uses 365 or 999 days of history.

Comment 34

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (2nd draft)

A did only minor fixed in the patch. The main thing is that I have created a bigger database which has 80MB, 30000 places and 900000 visits. coth suggests even 10 times more places, but this would lead to a database with 800MB with the same distribution of visits. Already now the number of visits would mean that the user would have to visit nearly 2500 places every day for the whole year.

And here are the results (original -> patch):

"By Date and Site"  1m8s -> 1.5s
"By Site"           1m17s -> 22s
"By Date"           1m40s -> 1.4s
"By Most Visited"   17s = 16s
"By Last Visited"   16s = 15s

As already said, there is no improvement possible for "By Site", "By Most Visited" and for "By Last Visited" other than limiting the number of records displayed and loading the rest on demand as the user scrolls down. I'm disappointed especially by the time needed to display "By Site", but this is probably because 70% of places have unique domain - what leads to a very big number of records returned by the database. I would expect a single user would usually browse more pages inside of one domain.
I have not found any problems with bookmarks. But there are still following issues:

1) Content od some queries does not get updated when user browses the pages.
2) The query.png icon should not probably be used (comments please).
3) The 1st and last item for sites and day sorting is swapped.
4) "By Site" results do not display "(local files)".
5) There seems to be a big overhead when executing queries with many returned rows, the time needed to get data using C API from sqlite for "By Site" query is only about 1 second - however it takes 22 seconds to read and display them inside of Firefox.

Comment 35

[coth]

well, i was saying about 200, not 800... anyway - in 2,5 years of not really very active surfing i collected about 650000 records into moz_places table...

Comment 36

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (3rd draft)

Solves issues 3 and 4 from comment #34.

Comment 37

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (4th draft - ready for preliminary review)

Solves issues 1 and 2 from comment #34. This means that this is fully functional patch for this bug. I have encountered some access violations, which I believe were not the code I have changed, but I will hunt for them right now, because I have an idea how to reproduce them.

In the meanwhile I would like to ask for:

1) A preliminary review - this is my first patch of this size, so may be doing this via IRC (ondrej#places) would be better than commenting the patch here.
2) Functional information about sorting for the different views and levels of the history sidebar and for information where new items should be added (it looks they are not always added in the sort order). I can mimic the sorting of FF2, but I would like to be sure it has not been changed.

Comment 38

[Mike Schroepfer]

Given the scope of this patch I'd really like to get real-world feedback in B3.  So moving to P1.  If you think otherwise or this can't land for that let me know.

Comment 39

[Dietrich Ayala (:dietrich)]

i uploaded this patch to the try server.

Comment 40

[Dietrich Ayala (:dietrich)]

test builds (windows not done yet):

https://build.mozilla.org/tryserver-builds/2008-01-15_13:[e-mail]-bug385245-history-sidebar/

I should be able to start reviewing this later tonight.

Comment 41

[Dietrich Ayala (:dietrich)]

(In reply to comment #40)
> test builds (windows not done yet):
> 
> https://build.mozilla.org/tryserver-builds/2008-01-15_13:[e-mail]-bug385245-history-sidebar/
> 
> I should be able to start reviewing this later tonight.
> 

builds:

https://build.mozilla.org/tryserver-builds/2008-01-15_13:[e-mail]-bug385245-history-sidebar/

Comment 42

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (1st patch ready for review)

This patch fixes the crash I was referring to in comment #37. This was an error in the old code, so it may be better to solve this with separate bug, however, it only happens after my changes (with subqueries in the sidebar).

In nsNavHistoryQueryResultNode::OnClearHistory() a Refresh() was called on node which has been already unregistered from the history observer. However, Refresh() calls FillChildren() what registers it again, just before it is removed from memory. A call to nsNavHistoryResult::OnVisit() accessed invalid memory when enumerating registered nodes. I have fixed it by not calling FillChildren() when Refresh() is called from OnClearHistory().

I'm waiting for review.

Comment 43

[Mike Connor [:mconnor]]

Comment on attachment 297350 [details] [diff] [review]
Grouping replaced with multiple queries (1st patch ready for review)

actually requesting review from dietrich

Comment 44

[Dietrich Ayala (:dietrich)]

Ondrej, please separate out the crash issue, and file a new bug for it. Also, please run the xpcshell tests locally (if you haven't already) to confirm that fix doesn't regress anything. If possible to test in an isolated manner, please add a new xpcshell test for it.

Comment 45

[Ria Klaassen (not reading all bugmail)]

It takes 3 seconds with the patched build from the tryserver and it took 8 seconds without. Places.sqlite is 11 MB exactly.

Comment 46

[Ondrej Brablc]

Comment on attachment 297350 [details] [diff] [review]
Grouping replaced with multiple queries (1st patch ready for review)

Some tests failed, and I want to refactor method ConstructQueryString which got too long.

Comment 47

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (2nd patch ready for review)

Refactored ConstructQueryString, removed all WARNINGS from console. Fixed to pass all unit tests. Removed some other bugs found during testing. Includes fixes for 412751, 412988 for completeness, so it will be necessary to create later another patch after they are committed.

Comment 48

[Ondrej Brablc]

Attachment: Grouping replaced with multiple queries (3rd patch ready for review)

Fixed sorting problem.
Fixed problem when added site query did not contain date conditions.
Removed warnings when accessing root which is not opened on OnVisit.
Test case test_history_sidebar.js for query based views in the history sidebar added.

Comment 49

[Ondrej Brablc]

Attachment: Grouping completely removed (another draft - some issues remaining)

What has been done:

- Smart bookmark "Recent Tags" is working again.
- Grouping has been completely removed.
- New result type for tags has been introduced - RESULTS_AS_TAG_QUERY,
  it returns a list of tags with query uri and should be very fast.
- Optimization for reading bookmarks (removal of the subquery because we 
  already join with history visits).
- Fixed crash on non-windows due to missing .get() in printf.
- Bug 331487 is probably getting a duplicate of this.


Remaining Issues:

1) The "Recent Tags" uri has changed to: place:type=6&sort=12&maxResults=10
   (note there is no folder id in the query anymore). See related bug 413693.

2) Option maxResults excludes folders from the result. It was temporarily
   removed from test_sort_by_count.js so that the test PASSes. I still want
   to fix it with this bug.

Following pages will need an update due to changed API:

http://developer.mozilla.org/en/docs/Places:PlaceURIs
http://developer.mozilla.org/en/docs/nsINavHistoryQueryOptions

Comment 50

[Dietrich Ayala (:dietrich)]

Comment on attachment 298742 [details] [diff] [review]
Grouping completely removed (another draft - some issues remaining)


Ondrej: sorry for taking so long to get to this. there're only 10 failing hunks, so de-rotting this shouldn't be overly bad.

overall, the approach is good. this makes the api simpler, though slightly less flexible. i'm fine with dropping the groupers since they were inconsistent and only partially implemented to begin with. i like that tag results are explicitly declared, that'll make things much clearer for extension developers.

>Index: browser/themes/pinstripe/browser/places/places.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/places/places.css,v
>retrieving revision 1.19
>diff -u -1 -2 -p -d -a -r1.19 places.css
>--- browser/themes/pinstripe/browser/places/places.css	21 Nov 2007 01:35:15 -0000	1.19
>+++ browser/themes/pinstripe/browser/places/places.css	23 Jan 2008 18:26:31 -0000
>@@ -51,24 +51,34 @@ treechildren::-moz-tree-image(container,
>   list-style-image: url("chrome://browser/skin/page-livemarks.png");
> }
> 
> treechildren::-moz-tree-image(container, tagContainer) {
>   list-style-image: url("chrome://mozapps/skin/places/tagContainerIcon.png");
> }
> 
> /* query-nodes should be styled even if they're not expandable */
> treechildren::-moz-tree-image(query) {
>   list-style-image: url("chrome://browser/skin/places/query.png");
> }
> 
>+/* we want to decorate some queries as folders base on their parent query type */
>+treechildren::-moz-tree-image(title, query, folder) {
>+  list-style-image: url("chrome://global/skin/icons/folder-item.png");
>+  -moz-image-region: rect(0px, 32px, 16px, 16px);
>+}
>+
>+treechildren::-moz-tree-image(title, query, folder, open) {
>+  -moz-image-region: rect(16px, 32px, 32px, 16px);
>+}
>+

in the original patch, the sidebar icons were separated too far from the text on Mac. does it look ok on windows?

>@@ -748,28 +748,29 @@ interface nsINavHistoryObserver : nsISup
>    * resource (page, image, etc.) is seen as well as every subsequent time.
>    *
>    * Normally, transition types of TRANSITION_EMBED (corresponding to images in
>    * a page, for example) are not displayed in history results (unless
>    * includeHidden is set). Many observers can ignore _EMBED notifications
>    * (which will comprise the majority of visit notifications) to save work.
>    *
>    * @param aVisitID        ID of the visit that was just created.
>    * @param aTime           Time of the visit
>    * @param aSessionID      The ID of one connected sequence of visits.
>    * @param aReferringID    The ID of the visit the user came from. 0 if empty.
>    * @param aTransitionType One of nsINavHistory.TRANSITION_*
>+   * @param aAdded          Incremented by one if the node has been added
>    */
>   void onVisit(in nsIURI aURI, in long long aVisitID, in PRTime aTime,
>                in long long aSessionID, in long long aReferringID,
>-               in unsigned long aTransitionType);
>+               in unsigned long aTransitionType, out unsigned long aAdded);

rev the uuid please.

>@@ -954,56 +955,24 @@ interface nsINavHistoryQuery : nsISuppor
>    * Creates a new query item with the same parameters of this one.
>    */
>   nsINavHistoryQuery clone();
> };
> 
> /**
>  * This object represents the global options for executing a query.
>  */
> [scriptable, uuid(ff73bf85-2755-4c1a-a48d-8c91ccca770e)]
> interface nsINavHistoryQueryOptions : nsISupports

rev uuid please

>+// ** Helper class for ConstructQueryString **/

i appreciate the move for organization and clarity here! we sorely need it. but i'm not sure it's wise nor necessary this late in the cycle, if it's not either blocking fixing this bug properly, or providing some other concrete benefit. can you please list what changed (distinct from what just got re-arranged)?

re: the sorting optimization - yes, that may help load of "last visited", "most visited", etc so might be worth taking for that. please do some tests to see whether it helps or not.

also, i think that as-is, this is pretty fragile: it depends on all these methods being called in the proper order, for no reason that i can discern. i understand it's only got one consumer, but still doesn't explain why you couldn't just configure up front, and expose a single GetQueryString() method, which calls the proper sequence of private methods.

finally, the name should be something like PlacesSQLQueryBuilder, because:

1. less ambiguity: "query" and "querystring" are kinda overloaded in Places
2. it covers tags and bookmarks etc, not just history

>+
>+class HistoryQueryStringBuilder
>+{
>+public:
>+  HistoryQueryStringBuilder(nsCString & aQueryString, 
>+                     const nsCString & aConditions,

nit: remove the space before the ampersand, here and throughout the patch

>+
>+HistoryQueryStringBuilder::HistoryQueryStringBuilder(nsCString & aQueryString, 
>+                                       const nsCString & aConditions,
>+                                       PRUint16 aResultType, 
>+                                       PRUint16 aQueryType, 
>+                                       PRBool aIncludeHidden):
>+  mQueryString( aQueryString ),
>+  mConditions( aConditions ),
>+  mResultType( aResultType ),
>+  mQueryType( aQueryType ),
>+  mIncludeHidden( aIncludeHidden ),
>+
>+  mSkipOrderBy( PR_FALSE )

nit: remove spaces after arguments

>+    case nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY:
>+      rv = SelectAsTag();
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      break;
>+
>+    default:
>+      NS_NOTREACHED("Invalid sorting mode");

s/sorting mode/result type/

> nsresult
> nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries,
>                                    nsNavHistoryQueryOptions *aOptions, 
>-                                   nsCString &queryString)
>+                                   nsCString &queryString, 
>+                                   PRBool &aParamsPresent)
> {
>+  nsresult rv;
>+
>+  // Informatin about visit_type:

spelling nit: Information

>Index: toolkit/components/places/src/nsNavHistory.h
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v
>retrieving revision 1.120
>diff -u -1 -2 -p -d -a -r1.120 nsNavHistory.h
>--- toolkit/components/places/src/nsNavHistory.h	19 Jan 2008 06:58:21 -0000	1.120
>+++ toolkit/components/places/src/nsNavHistory.h	23 Jan 2008 18:26:33 -0000
>@@ -1,13 +1,13 @@
>-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit: whitespace

up to here.

Comment 51

[Dietrich Ayala (:dietrich)]

>     // If it's a simple folder node (i.e. a shortcut to another folder), apply
>-    // our options for it.
>-    if (*aResult && (*aResult)->IsFolder())  {
>+    // our options for it. However, if the parent type was tag query, we do not
>+    // apply them, because it would not yield any results.
>+    if ( *aResult && (*aResult)->IsFolder() && 
>+         aOptions->ResultType() != nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY )  {

style nit: no space after opening parens (and throughout patch)

>+
>+  switch (aOptions->ResultType())
>+  {
>+    case nsNavHistoryQueryOptions::RESULTS_AS_VISIT:
>+    case nsNavHistoryQueryOptions::RESULTS_AS_FULL_VISIT:
>+      // visit query - want exact visit time
>+      statement = mDBVisitToVisitResult;
>+      break;
>+
>+    case nsNavHistoryQueryOptions::RESULTS_AS_URI:
>+      // URL results - want last visit time
>+      statement = mDBVisitToURLResult;
>+      break;
>+
>+    default:
>+      return NS_OK;
>   }

why not defaulting to URI result like before? hm, actually if the result type isn't supported here than it's an error state, so shouldn't return NS_OK;

> // nsNavHistoryContainerResultNode::FindInsertionPoint
> //
> //    This returns the index that the given item would fall on if it were to
> //    be inserted using the given sorting.
> 
> PRUint32
> nsNavHistoryContainerResultNode::FindInsertionPoint(
>     nsNavHistoryResultNode* aNode, SortComparator aComparator,
>-    const char* aData)
>+    const char* aData, PRBool * aItemExists)

style nit: extra space after PRBool

Comment 52

[Ondrej Brablc]

Attachment: Grouping completely removed (ready for final review)

This patch fixes all the comments made by Dietrich. It removes sorting of folders by the count of their items and respective test case too - this was not working due to the removal of grouping, there is no use case for this and if there would be one, then this should be implemented using an SQL query. The patch fills correctly "time" for day queries as requested by bug 363621.

Some comments:

(In reply to comment #50)
> in the original patch, the sidebar icons were separated too far from the text
> on Mac. does it look ok on windows?

On Windows it looks fine, I did not check any other platform.

> i appreciate the move for organization and clarity here! we sorely need it. but
> i'm not sure it's wise nor necessary this late in the cycle, if it's not either
> blocking fixing this bug properly, or providing some other concrete benefit.
> can you please list what changed (distinct from what just got re-arranged)?

3/5 of the class PlacesSQLQueryBuilder code are new (they replace the GroupBy... methods) and the rest has been optimized (SelectAsURI and SelectAsVisit). I understand your point, but it was not possible to keep it in one function without losing readability at all. A test case test_history_sidebar.js is included to cover changed functionality.

(In reply to comment #51)
> >+  switch (aOptions->ResultType())
> >+  {
> >+    case nsNavHistoryQueryOptions::RESULTS_AS_VISIT:
> >+    case nsNavHistoryQueryOptions::RESULTS_AS_FULL_VISIT:
> >+      // visit query - want exact visit time
> >+      statement = mDBVisitToVisitResult;
> >+      break;
> >+
> >+    case nsNavHistoryQueryOptions::RESULTS_AS_URI:
> >+      // URL results - want last visit time
> >+      statement = mDBVisitToURLResult;
> >+      break;
> >+
> >+    default:
> >+      return NS_OK;
> >   }
> 
> why not defaulting to URI result like before? hm, actually if the result type
> isn't supported here than it's an error state, so shouldn't return NS_OK;

Comment added to the code. The reason is that queries use different way of adding nodes to them.

Comment 53

[Dietrich Ayala (:dietrich)]

Attachment: wonky spacing

Comment 54

[Dietrich Ayala (:dietrich)]

try-server builds: https://build.mozilla.org/tryserver-builds/2008-02-12_17:[e-mail]-bug385245-sidebar/

Comment 55

[Ondrej Brablc]

(In reply to comment #53)
> Created an attachment (id=302863) [details]
> wonky spacing
> 

It looks as if there were space for two icons, but none of them was shown. Can someone check what happens if theme changes are rolled back, I do not have Mac at the moment, but I can borrow one to check it if necessary?

Comment 56

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #54)
> try-server builds:
> https://build.mozilla.org/tryserver-builds/2008-02-12_17:[e-mail]-bug385245-sidebar/
>
I tested this repeatedly on a neutral page; after having opened and closed Firefox and the history sidebar several times I started to clock with a stop watch, about 15 times. OS = Windows XP, places.sqlite = 13,8 MB. View = By Last Visited. I used my default profile with 18 extensions.

Last trunk hourly tested nearly always the same: it took 4 seconds to open the sidebar.
Test build from link tested either 3 or 5 seconds; so the results seemed more irregular.

 

Comment 57

[Ondrej Brablc]

In reply to comment #56)
> Last trunk hourly tested nearly always the same: it took 4 seconds to open the
> sidebar.
> Test build from link tested either 3 or 5 seconds; so the results seemed more
> irregular.

Yes, this bug solves only one part of the problem. It speeds up significantly "By Site", "By Date and Site" and "By Date" views. Additionally it speeds up query for recent tags from smart bookmarks. The default view in the history sidebar is "By Date". So this bug should already improve experience of many users. 

For the remaining two "By Most Visited" and "By Last Visited" I have created a separate bug 417262.

Comment 58

[Ria Klaassen (not reading all bugmail)]

Yes, "By Site", "By Date and Site" and "By Date" views are very fast in the test build on Win XP. Less than one second, maybe even a half second. They open instantly; no delay.

Comment 59

[Dietrich Ayala (:dietrich)]

>@@ -2400,28 +2450,30 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
>   rv = InternalAddVisit(pageID, referringVisitID, aSessionID, aTime,
>                         aTransitionType, aVisitID);
>   transaction.Commit();
> 
>   // Update frecency (*after* the visit info is in the db)
>   // Swallow errors here, since if we've gotten this far, it's more
>   // important to notify the observers below.
>   (void)UpdateFrecency(pageID, PR_FALSE);
> 
>   // Notify observers: The hidden detection code must match that in
>   // GetQueryResults to maintain consistency.
>   // FIXME bug 325241: make a way to observe hidden URLs
>+  transaction.Commit();

did you mean to make this change?
 
>+// ** Helper class for ConstructQueryString **/
>+
>+class PlacesSQLQueryBuilder
>+{
>+public:
>+  PlacesSQLQueryBuilder( 
>+                     const nsCString& aConditions,
>+                     nsNavHistoryQueryOptions* aOptions,
>+                     PRBool aUseLimit);

style nit: please keep first param on the method name line, and line up subsequent params.

>+PlacesSQLQueryBuilder::PlacesSQLQueryBuilder(const nsCString& aConditions,
>+                                       nsNavHistoryQueryOptions* aOptions,
>+                                       PRBool aUseLimit):
>+  mConditions( aConditions),
>+  mResultType( aOptions->ResultType()),
>+  mQueryType( aOptions->QueryType()),
>+  mIncludeHidden( aOptions->IncludeHidden()),
>+  mSortingMode( aOptions->SortingMode()),
>+  mMaxResults( aOptions->MaxResults()),
>+  mUseLimit( aUseLimit),
>+  mSkipOrderBy( PR_FALSE)

style nit: no space after opening parens

>+nsresult
>+PlacesSQLQueryBuilder::GetQueryString(nsCString& aQueryString)
>+{
>+  nsresult rv;
>+
>+  rv = Select();

nit: could be on one line

>+nsresult
>+PlacesSQLQueryBuilder::SelectAsURI()
>+{
>+  switch (mQueryType) 
>+  {
>+    case nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY:
>+      // For URLs, it is more complicated, because we want each URL once. The
>+      // GROUP BY clause gives us this. To get the max visit time, we populate
>+      // one column by using a nested SELECT on the visit table. Also, ignore
>+      // session information.
>+      // FIXME(brettw) add nulls for full visit info

please remove that comment while you're here

>+      mQueryString = NS_LITERAL_CSTRING("\
>+SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date), f.url, null, null \
>+FROM moz_places h \
>+     LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id \
>+     LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");

please build literal strings as shown below, which is more readable, retains indentation, and is in the style of the rest of the Places code:

mQueryString = NS_LITERAL_CSTRING(
  "SELECT FOO "
  "FROM BAR "
  "WHERE BAZ");

please apply to the other queries as well.

>+nsresult 
>+PlacesSQLQueryBuilder::SelectAsVisit()
>+{
>+  // if we want visits, this is easy, just combine all possible matches
>+  // between the history and visits table and do our query.
>+  // FIXME(brettw) Add full visit info

remove comment

>+nsresult 
>+PlacesSQLQueryBuilder::SelectAsDay()
>+{
>+  mSkipOrderBy = PR_TRUE;
>+  PRBool asDayQuery = mResultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY;
>+
>+  mQueryString = nsPrintfCString(255, "\
>+SELECT null, \
>+  'place:type=%ld&sort=%ld&beginTime='||beginTime||'&endTime='||endTime, \
>+  dayTitle, null, null, endTime, null, null, null, null \
>+FROM (", 
>+   (asDayQuery
>+      ?nsINavHistoryQueryOptions::RESULTS_AS_URI
>+      :nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY),
>+    nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING);
>+
>+  nsNavHistory* history = nsNavHistory::GetHistoryService();
>+  NS_ENSURE_TRUE(history, 0);

nit: NS_ENSURE_SERVICE(history);

>+
>+  nsCAutoString dateName;
>+
>+  const PRInt32 MAX_DAYS = 6;
>+
>+  for (PRInt32 i=0; i<=MAX_DAYS; i++) 
>+  {
>+    switch (i) 
>+    {
>+      case 0:
>+        history->GetStringFromName(NS_LITERAL_STRING("finduri-AgeInDays-is-0").get(), dateName);
>+        break;
>+      case 1:
>+        history->GetStringFromName(NS_LITERAL_STRING("finduri-AgeInDays-is-1").get(), dateName);
>+        break;
>+      default:
>+        history->GetAgeInDaysString(i, NS_LITERAL_STRING("finduri-AgeInDays-is").get(), dateName);
>+        break;
>+    }
>+
>+    PRInt32 fromDayAgo = -i;
>+    PRInt32 toDayAgo = -i + 1;
>+
>+    nsPrintfCString dayRange(1024, "\
>+SELECT * FROM \
>+(SELECT %d dayOrder,\
>+        '%d' dayRange,\
>+        '%s' dayTitle,\
>+        CAST(strftime('%%s',current_date, '%d days') AS UNSIGNED)*1000000 beginTime,\
>+        CAST(strftime('%%s',current_date, '%d days') AS UNSIGNED)*1000000 endTime \
>+ FROM   moz_historyvisits \
>+ WHERE  visit_date >= CAST(strftime('%%s',current_date, '%d days') AS UNSIGNED)*1000000 \
>+   AND  visit_date <  CAST(strftime('%%s',current_date, '%d days') AS UNSIGNED)*1000000 \
>+   AND  visit_type NOT IN(0,4) \
>+ LIMIT 1) TUNION%d UNION ", i, i, dateName.get(), fromDayAgo, toDayAgo, fromDayAgo, toDayAgo, i);
>+

- iirc we had issues w/ sqlite's internal date and time functions deviating from PR_Now(). maybe we should do this calculation outside of the db
- again, indenting would help readability here
- please uppercase current_date (if you keep using it)

Comment 60

[Ondrej Brablc]

Attachment: Grouping completely removed (review comments)

(In reply to comment #59)
> >+  transaction.Commit();
> 
> did you mean to make this change?

Wrong CVS conflict resolution.

> - iirc we had issues w/ sqlite's internal date and time functions deviating
> from PR_Now(). maybe we should do this calculation outside of the db
> - again, indenting would help readability here
> - please uppercase current_date (if you keep using it)

There was a bug, UTC time was used, I'm still using CURRENT_DATE but modifying it to the localtime. Please check that you get correct TZ difference, if you run this SQL statement:

All other comments accepted.
  SELECT (strftime('%s',CURRENT_DATE, 'LOCALTIME', '-0 days') -
          strftime('%s',CURRENT_DATE, '-0 days'))  / 3600;

Comment 61

[Ondrej Brablc]

Attachment: Grouping completely removed (removed hardcoded numbers)

I forgot to say that I do not see NS_ENSURE_SERVICE anywhere else in Mozilla, so I did not replace the NS_ENSURE_TRUE. With this patch I'm replacing some hardcoded numbers in favor of already existing define.

Comment 62

[Dietrich Ayala (:dietrich)]

(In reply to comment #61)
> Created an attachment (id=303257) [details]
> Grouping completely removed (removed hardcoded numbers)
> 
> I forgot to say that I do not see NS_ENSURE_SERVICE anywhere else in Mozilla,
> so I did not replace the NS_ENSURE_TRUE. With this patch I'm replacing some
> hardcoded numbers in favor of already existing define.
> 

sorry, i meant NS_ENSURE_STATE(), eg:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.h#53

Comment 63

[Dietrich Ayala (:dietrich)]

more: 

>   /**
>-   * The grouping mode to be used for this query.
>-   * Grouping mode is an array of GROUP_BY_* values that specifies the
>-   * structure of the tree you want.  For example, an array consisting of
>-   * [GROUP_BY_DAY, GROUP_BY_DOMAIN] will give you a tree whose first level is
>-   * a list of days, and whose second level is a list of domains, and whose
>-   * third level is a list of pages in those domains.
>-   * If you don't want grouping, you can specify an empty array.
>+   * This returns just empty nodes for each day where we have visits.
>+   * The node contains information how to load its content:
>+   * - last visits for the given day range will be loaded.
>    */

s/just empty/query/

ditto for the other result types

s/last//, since it loads all visits for the date range.

>+nsresult
>+nsNavHistory::InitFunctions()
>+{
>+  nsresult rv;
>+
>+  rv = mDBConn->CreateFunction(
>+      NS_LITERAL_CSTRING("get_unreversed_host"), 1, new mozStorageFunctionGetUnreversedHost);
>+
>+  return NS_OK;
>+}
>+

check rv

>+
>+nsresult
>+PlacesSQLQueryBuilder::Where()
>+{
>+  // If we used WHERE already, we inject the conditions in place of {ADDITIONAL_CONDITIONS}
>+  PRUint32 useInnerCondition;
>+  useInnerCondition = mQueryString.Find("{ADDITIONAL_CONDITIONS}",0);
>+  if (useInnerCondition != kNotFound) {
>+
>+    nsCAutoString innerCondition;
>+    // If we have condition AND it
>+    if (!mConditions.IsEmpty()) {
>+      innerCondition = " AND ";
>+      innerCondition += mConditions;
>+    }
>+    mQueryString.ReplaceSubstring("{ADDITIONAL_CONDITIONS}", innerCondition.get());
>+
>+  } else {
>+
>+    if (!mConditions.IsEmpty()) {

nit: else if (...) {

>+nsresult
>+PlacesSQLQueryBuilder::OrderBy()
>+{
>+  if (mSkipOrderBy)
>+    return NS_OK;
>+
>+  // Sort clause: we will sort later, but if it comes out of the DB sorted,
>+  // our later sort will be basically free. The DB can sort these for free
>+  // most of the time anyway, because it has indices over these items.
>+  //
>+  // FIXME: do some performance tests, I'm not sure that the indices are getting
>+  // used, in which case we should just remove this except when there are max
>+  // results.

can't land without some data here.

also, would be nice to use the kGetChildrenIndex_* constants (+1) when building the query string, instead of hard-coding them.

+
> nsresult
> nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries,
>                                    nsNavHistoryQueryOptions *aOptions, 
>-                                   nsCString &queryString)
>+                                   nsCString &queryString, 
>+                                   PRBool& aParamsPresent)
> {
>+  nsresult rv;
>+
>+  // Information about visit_type:
>+  // 4 == TRANSITION_EMBED
>+  // 0 == undefined (see bug #375777 for details)
>+  // Some sites, especially Javascript-heavy ones, load things in frames to display them,
>+  // resulting in a lot of these entries. This is reasons why such visits are filtered out.

nit: "This is the reason"


>+  PlacesSQLQueryBuilder queryStringBuilder(
>+    conditions, 
>+    aOptions,
>+    // determine whether we can push maxResults constraints
>+    // into the queries as LIMIT, or if we need to do result count clamping later
>+    // using FilterResultSet()
>+    !NeedToFilterResultSet(aQueries, aOptions));

please either break it out into a separate variable or put the docs above the function call

>+
>+    // None of the registered query observers has accepted our URI, this means,
>+    // that a matching query either was not expanded or it does not exist.
>+    // If the it just was not expanded, we can ignore it, but if it did not

s/the//

>+      if (!childCount) {
>+        todayIsMissing = PR_TRUE;
>+      } else {
>+        nsCOMPtr<nsINavHistoryResultNode> firstChild;
>+        rv = mRootNode->GetChild(0, getter_AddRefs(firstChild));

check rv please

>+      if (todayIsMissing) { // Add "Today"
>+
>+        nsCAutoString queryUri;
>+        queryUri = nsPrintfCString(255, 
>+          "place:type=%ld&sort=%ld%s",
>+          resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY
>+            ?nsINavHistoryQueryOptions::RESULTS_AS_URI
>+            :nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY,
>+          nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
>+          dateRange.get());
>+
>+        nsRefPtr<nsNavHistoryQueryResultNode> todayNode;
>+        todayNode = new nsNavHistoryQueryResultNode(todayLabel, EmptyCString(), queryUri);
>+        mRootNode->InsertChildAt( todayNode, 0);

check rv

>+
>+      nsCAutoString queryUri;
>+      queryUri = nsPrintfCString(255, 
>+        "place:type=%ld&sort=%ld&domain=%s&domainIsHost=true%s",
>+        nsINavHistoryQueryOptions::RESULTS_AS_URI,
>+        nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
>+        host.get(),
>+        dateRange.get());
>+
>+      nsRefPtr<nsNavHistoryQueryResultNode> siteNode;
>+      siteNode = new nsNavHistoryQueryResultNode(host, EmptyCString(), queryUri);
>+      siteRoot->GetAsContainer()->InsertSortedChild( siteNode, PR_FALSE, PR_TRUE/*Ignore duplicates*/);

check rv

>@@ -546,36 +524,33 @@ PlacesTreeView.prototype = {
>       case Ci.nsINavHistoryQueryOptions.SORT_BY_TAGS_DESCENDING:
>         return [this.COLUMN_TYPE_TAGS, true];
>     }
>     return [this.COLUMN_TYPE_UNKNOWN, false];
>   },
> 
>   // nsINavHistoryResultViewer
>   itemInserted: function PTV_itemInserted(aParent, aItem, aNewIndex) {
>     if (!this._tree)
>       return;
>     if (!this._result)
>       throw Cr.NS_ERROR_UNEXPECTED;
>-
>     if (PlacesUtils.nodeIsSeparator(aItem) &&
>         this._result.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
>       aItem.viewIndex = -1;
>       return;
>     }
>-
>     // update parent when inserting the first item because twisty may
>     // have changed
>     if (aParent.childCount == 1)
>       this.itemChanged(aParent);
>-
>     // compute the new view index of the item
>     var newViewIndex = -1;
>     if (aNewIndex == 0) {
>       // item is the first thing in our child list, it takes our index +1. Note
>       // that this computation still works if the parent is an invisible root
>       // node, because root_index + 1 = -1 + 1 = 0
>       newViewIndex = aParent.viewIndex + 1;
>     }
>     else {
>       // Here, we try to find the next visible element in the child list so we
>       // can set the new visible index to be right before that. Note that we
>       // have to search DOWN instead of up, because some siblings could have

hunk not necessary

Comment 64

[Ondrej Brablc]

Attachment: Grouping completely removed (more review comments fixed)

(In reply to comment #63)
> >+  //
> >+  // FIXME: do some performance tests, I'm not sure that the indices are getting
> >+  // used, in which case we should just remove this except when there are max
> >+  // results.
> 
> can't land without some data here.

I have removed the comment, please let me know if I should file another bug for this. The comment was in the code already before and I was not touching any indexes with this bug.

Other comments were fixed.

Comment 65

[Dietrich Ayala (:dietrich)]

Comment on attachment 303524 [details] [diff] [review]
Grouping completely removed (more review comments fixed)


>   /**
>    * Determines whether or not a ResultNode is a host folder or not
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is a host item, false otherwise
>    */
>   nodeIsHost: function PU_nodeIsHost(aNode) {
>     NS_ASSERT(aNode, "null node");
>-    return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_HOST;
>+    return (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY && 
>+      aNode.parent &&
>+      asQuery(aNode.parent).queryOptions.resultType ==
>+            Ci.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY);
>   },

nit: fix indent

> It looks as if there were space for two icons, but none of them was shown. Can
> someone check what happens if theme changes are rolled back, I do not have Mac
> at the moment, but I can borrow one to check it if necessary?
> 

rolling back this change fixes the spacing, shows the query icon.

finally, we need to address the tag query in smart bookmarks folder. a one-time sql UPDATE of moz_places entries with that URI might be the best option at this point.

getting close!

Comment 66

[Dietrich Ayala (:dietrich)]

when opening the sidebar grouped by site, i get this warning:

WARNING: Unsafe use of LIKE detected!  Please ensure that you are using mozIStorageConnection::escapeStringForLIKE and that you are binding that result to the statement to prevent SQL injection attacks.: file /Users/dayala/moz/trunk/mozilla/storage/src/mozStorageStatement.cpp, line 176

Comment 67

[Dietrich Ayala (:dietrich)]

please run the tests in bug 384226 against this patch. it tests some complex history query scenarios and live update.

Comment 68

[Dietrich Ayala (:dietrich)]

Comment on attachment 303524 [details] [diff] [review]
Grouping completely removed (more review comments fixed)

canceling review. need to resolve the final issues in the previous few comments.

Comment 69

[Ondrej Brablc]

Attachment: Grouping completely removed (indent and icons fixed)

(In reply to comment #65)
> (From update of attachment 303524 [details] [diff] [review])

I fixed CSS for both Mac and Linux - should be fine, but I do not have environment/know how to make a test build.

Please give me hint where you would like to see the update code.

(In reply to comment #66)
> when opening the sidebar grouped by site, i get this warning:
> 
> WARNING: Unsafe use of LIKE detected!  Please ensure that you are using

I have studied this, this is a false alarm. There is no risk involved and modification would be too expensive (I would have to refactor a lot of existing code).

Comment 70

[Ondrej Brablc]

(In reply to comment #67)
> please run the tests in bug 384226 against this patch. it tests some complex
> history query scenarios and live update.
> 

test_places/queries/test_complexhistory1.js: PASS

Comment 71

[Dietrich Ayala (:dietrich)]

(In reply to comment #69)
> Created an attachment (id=303574) [details]
> Grouping completely removed (indent and icons fixed)
> 
> (In reply to comment #65)
> > (From update of attachment 303524 [details] [diff] [review] [details])
> 
> I fixed CSS for both Mac and Linux - should be fine, but I do not have
> environment/know how to make a test build.

works on mac, thanks

> 
> Please give me hint where you would like to see the update code.
> 

this is called before the first load of the toolbar, so maybe here?

http://mxr.mozilla.org/seamonkey/source/browser/base/content/browser-places.js#1023

Comment 72

[Dietrich Ayala (:dietrich)]

Comment on attachment 303574 [details] [diff] [review]
Grouping completely removed (indent and icons fixed)

removing review request, patch still needs smart folder migration.

Comment 73

[Ondrej Brablc]

Attachment: Grouping completely removed (complete patch)

This could be the final patch. Please check updating of the Recent Tags url. I have tested that it works, but I'm not sure, if this is the right way of doing it (I means handling of preferences).

Comment 74

[Dietrich Ayala (:dietrich)]

Comment on attachment 304024 [details] [diff] [review]
Grouping completely removed (complete patch)



>         }
>         // Remove the old annotation.
>         annosvc.removePageAnnotation(uri, oldPostDataAnno);
>       } catch(ex) {}
>     }
>     gPrefService.setBoolPref("browser.places.migratePostDataAnnotations", false);
>   }

nit: add a line here

>+  if (gPrefService.getBoolPref("browser.places.updateRecentTagsUri")) {
>+    var bmsvc = PlacesUtils.bookmarks;
>+    var tagsFolder = bmsvc.tagsFolder;
>+    var oldUriSpec = "place:folder=" + tagsFolder +
>+                     "&group=3&queryType=1&applyOptionsToContainers=1&sort=12&maxResults=10";

is applyOptionsToContainers used any more? if not, we may want to remove it. iirc it was added specifically for tag queries.

r=me.

mano, can you please do a pass on this?

Comment 75

[Ondrej Brablc]

Attachment: Grouping completely removed (complete patch - removed applyOptionsToContainers)

I have removed applyOptionsToContainers, it was really used just for tags query.

Comment 76

[Dietrich Ayala (:dietrich)]

Comment on attachment 304440 [details] [diff] [review]
Grouping completely removed (complete patch - removed applyOptionsToContainers)

thanks for the cleanup, r=me.

requesting a pass by mano.

Comment 77

[Asaf Romano (gone)]

So, I don't have cycles to review the toolkit parts here for beta 4. As for the browser/ changes, few questions/concerens:
 * Why did you remove the collapse-duplicates code?
 * You didn't remove it entriely.
 * The duplicated style blocks are bad, you can use "," in the selectors part instead...
 * When can |+treechildren::-moz-tree-image(title, query, tagContainer) {| happen? Which node does this match?

Comment 78

[Marco Bonardo [:mak]]

Bug 376706 is landing. Please check day container delete functionality after that lands.

Here is some minor nit:

+  const unsigned short RESULTS_AS_DATE_QUERY = 3;
+
+  /**
+   * This returns just empty nodes for each site where we have visits.
+   * The node contains information how to load its content:
+   * - last visits for the given host will be loaded.

Only last visits?

+  const nsCString& mConditions;
+  PRBool mUseLimit;
+
+  PRUint16 mResultType;
+  PRUint16 mQueryType;
+  PRBool mIncludeHidden;  

nit: trailing spaces

+  switch (mResultType) 
+  {

nit: trailing space

+    case nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS:
+      mQueryString = NS_LITERAL_CSTRING(
+        "SELECT b.fk, h.url, COALESCE(b.title, h.title), h.rev_host, h.visit_count, MAX(v.visit_date), "
+               "f.url, null, b.id, b.dateAdded, b.lastModified "
+        "FROM moz_bookmarks b "
+             "JOIN moz_places h ON b.fk = h.id AND b.type = 1 "
+             "LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id "
+             "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
+
+      mGroupBy = NS_LITERAL_CSTRING(" GROUP BY b.id");
+      break;

could you use the SQL MAX FRAGMENT here to get the max, or do you need additional conditions on moz_historyvisits?


+nsresult 
+PlacesSQLQueryBuilder::SelectAsVisit()

nit: trailing space

+  for (PRInt32 i=0; i<=MAX_DAYS; i++) 
+  {
+    switch (i) 
+    {

nit: trailing space, i think that style should be with parentheses on the same line (check other functions too) and expand with spaces for readability (check in the same file for current style):
  for (PRInt32 i = 0; i <= MAX_DAYS; ++i) 

+    mQueryString = nsPrintfCString(2048,
+      "SELECT DISTINCT null, "
+             "'place:type=%ld&sort=%ld&domain=&domainIsHost=true', "
+             "'%s', '%s', null, null, null, null, null "
+      "WHERE EXISTS(SELECT '*' "
+                   "FROM moz_places "
+                   "WHERE hidden <> 1 AND rev_host='.' AND url LIKE 

nit: spacing, rev_host = '.' 

+      "FROM (SELECT get_unreversed_host(rev_host) host "
+            "FROM (SELECT DISTINCT rev_host "
+                  "FROM moz_places "
+                  "WHERE hidden <> 1 AND rev_host<>'.') inner0 "

nit: spacing, rev_host <> '.'

+            "ORDER BY 1 ASC) inner1",
+      nsINavHistoryQueryOptions::RESULTS_AS_URI,
+      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
+      localFiles.get(), 

nit: trailing space

+                   "WHERE h.hidden <> 1 AND h.rev_host='.' AND h.url LIKE 

nit: spacing, also in other queries after this

+            "ORDER BY 1 ASC) inner1",
+      nsINavHistoryQueryOptions::RESULTS_AS_URI,
+      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING,
+      localFiles.get(), 

nit: trailing space

+      nsINavHistoryQueryOptions::RESULTS_AS_URI,
+      nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING);
+  } 

nit: trailing space

+nsresult 
+PlacesSQLQueryBuilder::SelectAsTag()
+{

nit: trailing space

 nsresult
 nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries,
                                    nsNavHistoryQueryOptions *aOptions, 
-                                   nsCString &queryString)
+                                   nsCString &queryString, 
+                                   PRBool& aParamsPresent)

nit: trailing space

         "SELECT DISTINCT p.id "
         "FROM moz_places p "
-        "JOIN moz_historyvisits ON place_id = p.id "
-        "WHERE hidden <> 1 AND visit_type NOT IN(0,4) "
+        "JOIN moz_historyvisits v ON place_id = p.id "
+        "WHERE p.hidden <> 1 AND v.visit_type NOT IN(0,4) "
         "ORDER BY visit_date DESC "
         "LIMIT ");

please use v. label on place_id and visit_date

     // If it's a simple folder node (i.e. a shortcut to another folder), apply
-    // our options for it.
-    if (*aResult && (*aResult)->IsFolder())  {
+    // our options for it. However, if the parent type was tag query, we do not
+    // apply them, because it would not yield any results.
+    if (*aResult && (*aResult)->IsFolder() && 

nit: trailing space

-  // if our options apply to containers and we are limiting our results
-  // remove items from the end of the mChildren array after sorting.
+  // if we are limiting our results remove items from the end of the 

nit: trailing space

+  // if we are limiting our results remove items from the end of the 

nit: trailing space

+      // USECS_PER_DAY == PR_USEC_PER_SEC * 60 * 60 * 24;
+      static const PRInt64 USECS_PER_DAY = LL_INIT(20, 500654080);
+
+      dateRange = nsPrintfCString(255, 

nit: trailing space

+      if (todayIsMissing) { // Add "Today"
+
+        nsCAutoString queryUri;

empty line? (there are others under other if-else)

+        queryUri = nsPrintfCString(255, 

nit: trailing space

 // If true, will migrate uri post-data annotations to
 // bookmark post-data annotations (bug 398914)
 // XXX to be removed after beta 2 (bug 391419)
 pref("browser.places.migratePostDataAnnotations", true);
 
+// If true, will update the Smart Bookmarks uri for
+// recent tags (bug 385245). Useful just for FX3 beta users.
+pref("browser.places.updateRecentTagsUri", true);
+

can't those be updated without a pref to level the userbase? why a pref that affects only beta users? Will be removed after final?

Comment 79

[Asaf Romano (gone)]

Comment on attachment 304440 [details] [diff] [review]
Grouping completely removed (complete patch - removed applyOptionsToContainers)

comment 77

Comment 80

[Dietrich Ayala (:dietrich)]

need response to comment 77 and comment 78 asap if we're going to get this in before freeze...

Comment 81

[Ondrej Brablc]

(In reply to comment #77)
> So, I don't have cycles to review the toolkit parts here for beta 4. As for the
> browser/ changes, few questions/concerens:
>  * Why did you remove the collapse-duplicates code?
To display history by days and/or sites grouping has been used. All visits were populated from database, grouped and duplicates removed. This is no more necessary and there is no other place on front-end which would need it. So the collapsing code is removed from front-end.

>  * You didn't remove it entriely.
Now fixed.

>  * The duplicated style blocks are bad, you can use "," in the selectors part
> instead...
Unfortunately I can't:
  treechildren::-moz-tree-image(title, query, folder)
gets overridden by 
  treechildren::-moz-tree-image(query)
I will check later whether this is a known bug or submit one.

>  * When can |+treechildren::-moz-tree-image(title, query, tagContainer) {|
> happen? Which node does this match?
This selector is never matched and will be removed (great insight).


(In reply to comment #78)
> Bug 376706 is landing. Please check day container delete functionality after
> that lands.

I have added new code for nodeIsDay, but it does not work. It deletes more days. Not sure if the bug could be in my code, but I will test it before submitting new patch.
 
> Here is some minor nit:
> 
> +  const unsigned short RESULTS_AS_DATE_QUERY = 3;
> +
> +  /**
> +   * This returns just empty nodes for each site where we have visits.
> +   * The node contains information how to load its content:
> +   * - last visits for the given host will be loaded.
> 
> Only last visits?

Yes, in history sidebar always only last visit is shown.

> nit: trailing spaces

All trailing spaces in new or changed code removed.

> +    case nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS:
> +      mQueryString = NS_LITERAL_CSTRING(
> +        "SELECT b.fk, h.url, COALESCE(b.title, h.title), h.rev_host,
> h.visit_count, MAX(v.visit_date), "
> +               "f.url, null, b.id, b.dateAdded, b.lastModified "
> +        "FROM moz_bookmarks b "
> +             "JOIN moz_places h ON b.fk = h.id AND b.type = 1 "
> +             "LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id "
> +             "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
> +
> +      mGroupBy = NS_LITERAL_CSTRING(" GROUP BY b.id");
> +      break;
> 
> could you use the SQL MAX FRAGMENT here to get the max, or do you need
> additional conditions on moz_historyvisits?

I have replaced it and tests passed through.

> +  for (PRInt32 i=0; i<=MAX_DAYS; i++) 
> +  {
> +    switch (i) 
> +    {
> 
> nit: trailing space, i think that style should be with parentheses on the same
> line (check other functions too) and expand with spaces for readability (check
> in the same file for current style):

Fixed using Mozilla Coding Style Guide, for on same line, switch on separate.

> nit: spacing, rev_host = '.'
> nit: spacing, rev_host <> '.'

Fixed.
 
> +                   "WHERE h.hidden <> 1 AND h.rev_host='.' AND h.url LIKE 
> 
> nit: spacing, also in other queries after this

Fixed.

> please use v. label on place_id and visit_date

Done.

> empty line? (there are others under other if-else)

Fixed all found.

>  // If true, will migrate uri post-data annotations to
>  // bookmark post-data annotations (bug 398914)
>  // XXX to be removed after beta 2 (bug 391419)
>  pref("browser.places.migratePostDataAnnotations", true);
> 
> +// If true, will update the Smart Bookmarks uri for
> +// recent tags (bug 385245). Useful just for FX3 beta users.
> +pref("browser.places.updateRecentTagsUri", true);
> +
> 
> can't those be updated without a pref to level the userbase? why a pref that
> affects only beta users? Will be removed after final?

This can probably be removed after final release (once all beta users have migrated). If the migration would not be performed to upgrading beta users, they would see in the "Recent Tags" folder 10 bookmarks that have been recently tagged, instead of seeing 10 recent tags. Please note that in case of downgrade, the "Recent Tags" would not return any results. See related bug 413693.

This bug is probably the cure for bug 400109.

Comment 82

[Ondrej Brablc]

Attachment: Collapsing removed, deletion of day folders fixed, review comments

This patch fixes all pending review comments.
Collapsing has now been completely removed from the front-end because there is no query on front-end that reads all visits.
For compatibility with bug 376706 I have fixed the way mTime gets filled for queries. I made little more readable the algorithm for time range selection in the controller. And implemented method nodeIsDay to work with queries. All tests passed.

Comment 83

[Ondrej Brablc]

Attachment: Interdiff for attachment 305562 (using alternative tool, cause interdiff fails)

Comment 84

[Ondrej Brablc]

(In reply to comment #83)
> Created an attachment (id=305565) [details]
> Interdiff for attachment 305562 [details] [diff] [review]  (using alternative tool, cause interdiff
> fails)
> 

This can be slightly more readable interdiff:

http://www.allpeers.com/download/mozilla/bug385245/bug385245nogroups7-8.html

Comment 85

[Dietrich Ayala (:dietrich)]

Comment on attachment 305562 [details] [diff] [review]
Collapsing removed, deletion of day folders fixed, review comments


>@@ -657,28 +639,29 @@ interface nsINavHistoryObserver : nsISup
>    * resource (page, image, etc.) is seen as well as every subsequent time.
>    *
>    * Normally, transition types of TRANSITION_EMBED (corresponding to images in
>    * a page, for example) are not displayed in history results (unless
>    * includeHidden is set). Many observers can ignore _EMBED notifications
>    * (which will comprise the majority of visit notifications) to save work.
>    *
>    * @param aVisitID        ID of the visit that was just created.
>    * @param aTime           Time of the visit
>    * @param aSessionID      The ID of one connected sequence of visits.
>    * @param aReferringID    The ID of the visit the user came from. 0 if empty.
>    * @param aTransitionType One of nsINavHistory.TRANSITION_*
>+   * @param aAdded          Incremented by one if the node has been added
>    */
>   void onVisit(in nsIURI aURI, in long long aVisitID, in PRTime aTime,
>                in long long aSessionID, in long long aReferringID,
>-               in unsigned long aTransitionType);
>+               in unsigned long aTransitionType, out unsigned long aAdded);

please add some text about the purpose of aAdded, and that it can be ignored if the observer is not a places view.

>   /**
>-   * The grouping mode to be used for this query.
>-   * Grouping mode is an array of GROUP_BY_* values that specifies the
>-   * structure of the tree you want.  For example, an array consisting of
>-   * [GROUP_BY_DAY, GROUP_BY_DOMAIN] will give you a tree whose first level is
>-   * a list of days, and whose second level is a list of domains, and whose
>-   * third level is a list of pages in those domains.
>-   * If you don't want grouping, you can specify an empty array.
>+   * This returns query nodes for each predefined date range where we 
>+   * had visits The node contains information how to load its content:

s/visits The/visits. The/

>+   * - visits for the given date range will be loaded.
>    */
>-  void getGroupingMode(out unsigned long groupCount,
>-                       [retval,array,size_is(groupCount)] out unsigned short groupingMode);
>-  void setGroupingMode([const,array,size_is(groupCount)] in unsigned short groupingMode,
>-                       in unsigned long groupCount);
>+  const unsigned short RESULTS_AS_DATE_QUERY = 3;
>+
>+  /**
>+   * This returns just empty nodes for each site where we have visits.
>+   * The node contains information how to load its content:
>+   * - last visit for each url in the given host will be loaded.
>+   */
>+  const unsigned short RESULTS_AS_SITE_QUERY = 4;
>+
>+  /**
>+   * This returns just empty nodes for each day where we have visits.
>+   * The node contains information how to load its content:
>+   * - list of hosts visited in the given period will be loaded.
>+   */
>+  const unsigned short RESULTS_AS_DATE_SITE_QUERY = 5;
>+
>+  /**
>+   * This returns just empty nodes for each tag.
>+   * The node contains information how to load its content:
>+   * - list of bookmarks with the given tag will be loaded.
>+   */
>+  const unsigned short RESULTS_AS_TAG_QUERY = 6;

s/just empty nodes/nsINavHistoryQueryResultNode nodes/

and for the preceding comments

>+  NS_IMETHODIMP mozStorageFunctionGetUnreversedHost::OnFunctionCall(
>+    mozIStorageValueArray* aFunctionArguments,
>+    nsIVariant** _retval)
>+  {
>+    NS_ENSURE_ARG_POINTER(_retval);
>+
>+    nsAutoString src;
>+    aFunctionArguments->GetString(0, src);
>+
>+    nsCOMPtr<nsIWritableVariant> result(do_CreateInstance("@mozilla.org/variant;1"));

nit: keep line length to 80 chars

>+nsresult
>+PlacesSQLQueryBuilder::SelectAsURI()
>+{
>+  switch (mQueryType)
>+  {
>+    case nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY:
>+      mQueryString = NS_LITERAL_CSTRING(
>+        "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date), f.url, null, null "
>+        "FROM moz_places h "
>+             "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
>+             "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
>+
>+      if (!mIncludeHidden)
>+        mQueryString += NS_LITERAL_CSTRING(" WHERE h.hidden <> 1 AND v.visit_type NOT IN (0,4) {ADDITIONAL_CONDITIONS} ");

nit: 80 chars please

>+nsresult
>+PlacesSQLQueryBuilder::SelectAsVisit()
>+{
>+  mQueryString = NS_LITERAL_CSTRING(
>+    "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, v.visit_date, f.url, v.session, null "
>+    "FROM moz_places h "
>+         "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
>+         "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
>+
>+  if (!mIncludeHidden)
>+    mQueryString += NS_LITERAL_CSTRING(" WHERE h.hidden <> 1 AND v.visit_type NOT IN (0,4) {ADDITIONAL_CONDITIONS} ");

ditto, and many other places in this patch

>-
>-      // don't display separators when sorted
>+      // don't display separators in tree
>       if (curChildType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) {
>-        if (this._result.sortingMode !=
>-            Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
>-          curChild.viewIndex = -1;
>-          continue;
>-        }
>+        curChild.viewIndex = -1;
>+        continue;

er, why is this?

Comment 86

[Ondrej Brablc]

Attachment: Review comments fixed

(In reply to comment #85)
> (From update of attachment 305562 [details] [diff] [review])
> >-      // don't display separators when sorted
> >+      // don't display separators in tree
> >       if (curChildType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) {
> >-        if (this._result.sortingMode !=
> >-            Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
> >-          curChild.viewIndex = -1;
> >-          continue;
> >-        }
> >+        curChild.viewIndex = -1;
> >+        continue;
> 
> er, why is this?

I have seen the ugly separator in the "Smart Bookmarks" displayed in the tree and did not see it in the Places Library. And because I was touching very close code, I have fixed it, but it does not belong here. I have removed it and fixed all other review comments.

Comment 87

[Dietrich Ayala (:dietrich)]

Comment on attachment 305718 [details] [diff] [review]
Review comments fixed

great, thanks for the final changes, r=me.

Comment 88

[Dietrich Ayala (:dietrich)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.287; previous revision: 1.286
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.97; previous revision: 1.96
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.57; previous revision: 1.56
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.208; previous revision: 1.207
done
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.25; previous revision: 1.24
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.97; previous revision: 1.96
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.40; previous revision: 1.39
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.105; previous revision: 1.104
done
Checking in browser/themes/gnomestripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/places.css,v  <--  places.css
new revision: 1.27; previous revision: 1.26
done
Checking in browser/themes/pinstripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/places.css,v  <--  places.css
new revision: 1.21; previous revision: 1.20
done
Checking in browser/themes/winstripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/places.css,v  <--  places.css
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.79; previous revision: 1.78
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.154; previous revision: 1.153
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.262; previous revision: 1.261
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.144; previous revision: 1.143
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v  <--  nsNavHistoryQuery.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.h,v  <--  nsNavHistoryQuery.h
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.135; previous revision: 1.134
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.54; previous revision: 1.53
done
Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.42; previous revision: 1.41
done
Checking in toolkit/components/places/tests/unit/test_331487.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_331487.js,v  <--  test_331487.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/places/tests/unit/test_markpageas.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_markpageas.js,v  <--  test_markpageas.js
new revision: 1.2; previous revision: 1.1
done

Checking in toolkit/components/places/tests/unit/test_history_sidebar.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history_sidebar.js,v  <--  test_history_sidebar.js
initial revision: 1.1
done

Comment 89

[Dietrich Ayala (:dietrich)]

/builds/tinderbox/Fx-Trunk/Linux_2.6.18-8.el5_Depend/mozilla/toolkit/components/places/src/nsNavHistory.cpp:925: error: extra ';'


Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.263; previous revision: 1.262
done

Comment 90

[Dietrich Ayala (:dietrich)]

test bustage fix:

Removing toolkit/components/places/tests/bookmarks/test_sort_by_count.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_sort_by_count.js,v  <--  test_sort_by_count.js
new revision: delete; previous revision: 1.2
done

Comment 91

[Tracy Walker [:tracy]]

sidebar is quite zippy.  Using a profile that has accumulated a few months worth of history on Mac.  On Win XP with a very large profile (30,000 visits) it takes about 2.5 seconds to open the older than six days folder.  That machine is 2GHz and 1 GB of ram.
verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre and the same nightly on Win XP

Comment 92

[Ash]

Does this change anything in nsINavHistoryService, and does the documentation need to be changed? In particular, is it still valid to get a nsINavHistoryFullVisitResultNode which provides the visitId and referringVisitId (which btw, has still not been implemented in code yet)?

Comment 93

[Ondrej Brablc]

I will use bug 422291 to update documentation. 

Comment 94

[Tuukka Tolvanen (sp3000)]

warningspam:
In constructor ‘PlacesSQLQueryBuilder::PlacesSQLQueryBuilder(const nsCString&, nsNavHistoryQueryOptions*, PRBool, nsDataHashtable<nsCStringHashKey, nsCString>&)’:
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#2760
warning: ‘PlacesSQLQueryBuilder::mMaxResults’ will be initialized after
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#2754
warning:   ‘PRBool PlacesSQLQueryBuilder::mUseLimit’
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#2773
warning:   when initialized here

Comment 95

[Ondrej Brablc]

I should have filed a new bug instead of reopening, sorry for that. See bug 424577.

Comment 96

[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