Closed Bug 598229 Opened 14 years ago Closed 14 years ago

Increase performance of Win7 JumpList favorites queries

Categories

(Firefox :: Shell Integration, defect)

Product:
Firefox
Component:
Shell Integration
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jimm, Assigned: mak)

References

Details

(Keywords: main-thread-io)

Attachments

(5 files, 5 obsolete files)

cleanup v1.2
7.16 KB, patch
Details | Diff | Splinter Review
patch v1.2
27.11 KB, patch
sdwilsh
: review+
Details | Diff | Splinter Review
patch v1.4
27.43 KB, patch
robert.strong.bugs
: superreview+
Details | Diff | Splinter Review
patch v1.5
27.45 KB, patch
jimm
: review+
Details | Diff | Splinter Review
patch v1.6
24.49 KB, patch
Details | Diff | Splinter Review 
Spin off from bug 594821 - sync favorites db queries are taking a really long time on dirty profiles. I landed a patch that delays these queries on startup which helped Ts (http://tinyurl.com/2vxhoso), but we're still making them in thirty second intervals. We need to decrease the drag these have on the browser.

http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsJumpLists.jsm#306
blocking2.0: --- → ?
So, the solution here is going to be roll our own SQL.  A bit sucky, but we'll get better response times, and we can do it asynchronously.

This might be a fair bit of work though, and I'm not sure who has the cycles for it.
(In reply to comment #1)
> So, the solution here is going to be roll our own SQL.  A bit sucky, but we'll
> get better response times, and we can do it asynchronously.
> 
> This might be a fair bit of work though, and I'm not sure who has the cycles
> for it.

For consideration in the blocking nomination: the lists are updated every 30 seconds. Removing the initial startup update dropped our cold Ts dirty profile talos numbers from around 2500msec, to 500msec. On Ts it was a minimal gain, about 20msec.

We could opt to block on this and get it done, or we could disable jump lists by default for 4.0. I'm a little concerned about the hit we'll be taking on dirty profiles as things stand now.
Shawn, is there some code someplace I could look at that does this kind of sql query work?
(In reply to comment #3)
> Shawn, is there some code someplace I could look at that does this kind of sql
> query work?
Really depends on what we'd need to do.  What information do you want to know about?  Or are you asking about the API in general?
ah, I didn't know we were doing this kind of query here, nor that this code was using Places.

this hurts, because it's not even hitting the special (and thus faster) queries, using beginTime makes us go the slow path, it was probably done for the opposite reason, instead. Was this code reviewed by a Places peer?

You want AT LEAST use the same queries we use in history menu and most visited, thus drop beginTimeReference, beginTime, endTimeReference. Also don't use SORT_BY_LASTMODIFIED_DESCENDING, you want SORT_BY_DATE_DESCENDING.

also _clearHistory should probably use removePages and not removePage since it's faster.

doing the above won't make this code async, but it will be A TON faster.
(In reply to comment #2)
> For consideration in the blocking nomination: the lists are updated every 30
> seconds.

can we reduce this or update them when the list is requested? This is MUCH expensive even if we do it async.
by "reduce" I meant "update less often"
(In reply to comment #5)
> ah, I didn't know we were doing this kind of query here, nor that this code was
> using Places.
> 
> this hurts, because it's not even hitting the special (and thus faster)
> queries, using beginTime makes us go the slow path, it was probably done for
> the opposite reason, instead. Was this code reviewed by a Places peer?

The code was reviewed by a browser peer.

> 
> You want AT LEAST use the same queries we use in history menu and most visited,
> thus drop beginTimeReference, beginTime, endTimeReference. Also don't use
> SORT_BY_LASTMODIFIED_DESCENDING, you want SORT_BY_DATE_DESCENDING.
> 
> also _clearHistory should probably use removePages and not removePage since
> it's faster.
> 
> doing the above won't make this code async, but it will be A TON faster.

FWIW, I based everything I did here on the Places docs on the wiki. Adding some perf. info there might help future devs.

https://developer.mozilla.org/en/Places/Query_System
(In reply to comment #6)
> (In reply to comment #2)
> > For consideration in the blocking nomination: the lists are updated every 30
> > seconds.
> 
> can we reduce this or update them when the list is requested? This is MUCH
> expensive even if we do it async.

Is there some sort of Places db change notification I can tie into?
(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #2)
> > > For consideration in the blocking nomination: the lists are updated every 30
> > > seconds.
> > 
> > can we reduce this or update them when the list is requested? This is MUCH
> > expensive even if we do it async.
> 
> Is there some sort of Places db change notification I can tie into?

Actually for frequent lists, that would probably be worse since it changes all the time.
(In reply to comment #9)
> Is there some sort of Places db change notification I can tie into?
We have listeners, but things change all the time (every time a page is visited).  What information do you need to know about?
(In reply to comment #10)
> Actually for frequent lists, that would probably be worse since it changes all
> the time.
Whoops, didn't see this.  Honestly, I'd expect every five minutes to be sufficient for most frequently used history entries.
I think we really need a callback that allows to populate the jumplist onpopupshowing
(In reply to comment #13)
> I think we really need a callback that allows to populate the jumplist
> onpopupshowing
Except that I'm pretty sure we can't do that since jumplists work even when the application is closed.  jimm?
(In reply to comment #14)
> Except that I'm pretty sure we can't do that since jumplists work even when the
> application is closed.

in such a case we could do a first update on startup (delayed) then next ones on request. When the app is closed the jumplist is just a cache of the last items inserted.
(In reply to comment #15)
> in such a case we could do a first update on startup (delayed) then next ones
> on request. When the app is closed the jumplist is just a cache of the last
> items inserted.
What I mean to say is that windows doesn't give us an API for this.  We don't draw the jumplist stuff - windows does.
actually, I think it should also listen for 'idle' and 'back' and don't update during user idles, since that can hurt laptop batteries without a reason.
(In reply to comment #14)
> (In reply to comment #13)
> > I think we really need a callback that allows to populate the jumplist
> > onpopupshowing
> Except that I'm pretty sure we can't do that since jumplists work even when the
> application is closed.  jimm?

(In reply to comment #17)
> actually, I think it should also listen for 'idle' and 'back' and don't update
> during user idles, since that can hurt laptop batteries without a reason.

Yeah, there's no notification. We can increase the update time maybe. 30 seconds seemed reasonable at the time. We could listen to activity. I'm not sure how much that would save us. I think most users that have a browser open are using it.
Regardless of when we do the update, I don't want that to drag down the browser. So optimizing the queries seems to me to be the biggest priority.
If this bug does not require changes to the underlying jumplist code (so if there is nothing we can do there to be notified when the user requests the list), I could also take it and do a first pass using the optimized queries.

What I'd do is:
- keep using Places sync APIs but go throught the optimized path (history menu and most visited). We can move to async apis once bug 522572 will be fixed (4.1 I guess)
- stop the update timer on 'idle' and set it back on 'back'
- use removePages
- increase the update time to 2 minutes

As a side note? Any reason this code is not using PlacesUtils and Services modules, those have shortcuts to practically all the used services.
Or if we prefer to go async, I can write async queries, but if possible I'd prefer not adding other queries to be maintained unless they are really needed (like in the Sync case).

Is this bug valid for 3.6 too?
(In reply to comment #20)
> If this bug does not require changes to the underlying jumplist code (so if
> there is nothing we can do there to be notified when the user requests the
> list), I could also take it and do a first pass using the optimized queries.

That would be great! :)

> 
> What I'd do is:
> - keep using Places sync APIs but go throught the optimized path (history menu
> and most visited). We can move to async apis once bug 522572 will be fixed (4.1
> I guess)
> - stop the update timer on 'idle' and set it back on 'back'
> - use removePages
> - increase the update time to 2 minutes
> 
> As a side note? Any reason this code is not using PlacesUtils and Services
> modules, those have shortcuts to practically all the used services.

I don't believe that was implemented when this code was written. I think there's a bug open on doing this migration, I'll try to track it down.

(In reply to comment #21)
> Or if we prefer to go async, I can write async queries, but if possible I'd
> prefer not adding other queries to be maintained unless they are really needed
> (like in the Sync case).
> 
> Is this bug valid for 3.6 too?

Nope. 4.0 only.
(In reply to comment #22)
> (In reply to comment #20)
> > As a side note? Any reason this code is not using PlacesUtils and Services
> > modules, those have shortcuts to practically all the used services.
> 
> I don't believe that was implemented when this code was written. I think
> there's a bug open on doing this migration, I'll try to track it down.

Bug 566856.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
somebody has to make this a blocker for me to work on it though.
I can do that.  This is a really bad thing that we are doing on windows and our fix is going to need beta feedback.
blocking2.0: ? → betaN+
Attached patch cleanup v1.0 (obsolete) — — Splinter Review 
this is just a first cleanup using modules.

I'm evaluating asking an history query to give me back a statement without executing it. this way I could make this async without having to create another query that will have to be maintained.
Attachment #479723 - Flags: review?
the patch above is missing a QueryInterface(Ci.nsIPrefBranch2), fixed locally.
Attached patch cleanup v1.1 (obsolete) — — Splinter Review 
fixed the QI
Attachment #479723 - Attachment is obsolete: true
Attachment #479841 - Flags: review?(jmathies)
Attachment #479723 - Flags: review?
Attached patch patch v1.0 (obsolete) — — Splinter Review 
and this is an idea of what we could do.
- async queries
- 2 minutes refreshes
- stop on idle
- don't create timer if disabled

asking feedback to sdwilsh relative to the Places changes. I added an API to async execute a Places query, it is not a direct replacement for the sync API, but can be a shurtcut for some known case. I'm unsure if due to the nature of this API I should move it to nsPIPlacesDatabase as a private API for internal use.
Attachment #479845 - Flags: feedback?(sdwilsh)
Jim, if you have comments on the patch feel free to put them on the table. I'll ask you formal review later once I've figured out if I need to fix some test and if Places changes are acceptable.
(In reply to comment #30)
> Jim, if you have comments on the patch feel free to put them on the table. I'll
> ask you formal review later once I've figured out if I need to fix some test
> and if Places changes are acceptable.

This maintains the build order right, so init, add a list, add a list, commit?

How long do these async queries take usually? Is it less than a second, seconds, minutes?
(In reply to comment #31)
> How long do these async queries take usually? Is it less than a second,
> seconds, minutes?
They are marginally slower than the sync ones (due to passing data between threads, so not much) but it means we don't block the UI when the disk is being slow to get back to us.
(In reply to comment #31)
> This maintains the build order right, so init, add a list, add a list, commit?

yes, it's done through this._pendingQueries, it's just a countdown.

> How long do these async queries take usually? Is it less than a second,
> seconds, minutes?

usually less then a second but depends on load on the async thread, I'd expect some second in the really worse case.
(In reply to comment #32)
> Also, how does this work with shutdown?
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsJumpLists.jsm#219

on shutdown the idle observer is removed and the timer is canceled, once _shuttingDown is setup there is no way for anything to recreate them.

Regarding what happens if there are pending statements on shutdown, I think they will just fail to create, Places shutdowns at profile-before-change. If they have been already created they will complete and the connection will be closed as soon as they are complete.
ps: if you are worried by having pending builders on shutdown not aborted, I could check if pendingBuilder is positive and abort list creation, also the async builder could early return if (_shuttingDown).
(In reply to comment #36)
> ps: if you are worried by having pending builders on shutdown not aborted, I
> could check if pendingBuilder is positive and abort list creation, also the
> async builder could early return if (_shuttingDown).

I was curious if it would get cancelled due to the delay. The list (task entries) change depending on the open state of the browser.

Similarly, the 30 second delay we currently have on startup isn't optimal, since we keep the closed state of the list until the first update.  Could we potentially go back to updating on startup without killing Ts with these async changes?
No, queries are not canceled, but they could fail. So I can add checks so that on shutdown we don't try to update history lists, doesn't look like there is a need for that.

I think it's not a good idea to hit startup regardless, this is still IO, even if it's not in the main thread. Maybe we could update only the tasks list.
New plan: update every 2 minutes all, whil update only the tasks list on startup and shutdown. This should give back the best compromise.

Looks like there is no test for the module but only for the underlying API, so I'll most likely add a unit test to ensure lists are updated and maybe some check for idle, let's see. I'll have to call module methods manually, that's not fair, but it's less hackish than magic debug notifications.
(In reply to comment #39)
> New plan: update every 2 minutes all, whil update only the tasks list on
> startup and shutdown. This should give back the best compromise.
> 
> Looks like there is no test for the module but only for the underlying API, so
> I'll most likely add a unit test to ensure lists are updated and maybe some
> check for idle, let's see. I'll have to call module methods manually, that's
> not fair, but it's less hackish than magic debug notifications.

I don't think that will work. If I remember correctly, a commit resets the entire list, so you have to add all the lists you want on each commit. So what we would end up with is just a task list after shutdown. Is there some way we could force a sync update on shutdown?

If we can't what we should probably do for now is standardize on the open list in both cases, it has a couple entries (new window, new tab) that are kind of pointless but they won't break anything. Then later on we can add some sort of caching feature to the underlying widget code. I'd like to do that anyway so that the lists aren't built in real time, but that is future work when we have free time.

Another case where the update is important due to a list change is when we enter/exit privacy mode. The tasks entry has an enter/exit option in it that needs updating.
(In reply to comment #40)
> (In reply to comment #39)
> I don't think that will work. If I remember correctly, a commit resets the
> entire list, so you have to add all the lists you want on each commit. So what
> we would end up with is just a task list after shutdown. Is there some way we
> could force a sync update on shutdown?

yes, but we don't want IO on shutdown as well as on startup, if possible.

> If we can't what we should probably do for now is standardize on the open list
> in both cases, it has a couple entries (new window, new tab) that are kind of
> pointless but they won't break anything. Then later on we can add some sort of
> caching feature to the underlying widget code. I'd like to do that anyway so
> that the lists aren't built in real time, but that is future work when we have
> free time.

the open list is tasklist, that's what I wanted to update. I'll check if a commit kills the list, I thought that was only done by builder.deleteActiveList().

> Another case where the update is important due to a list change is when we
> enter/exit privacy mode. The tasks entry has an enter/exit option in it that
> needs updating.

I don't mind doing updates there, the critical points for me are just startup and shutdown.
(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #39)
> 
> the open list is tasklist, that's what I wanted to update. I'll check if a
> commit kills the list, I thought that was only done by
> builder.deleteActiveList().
> 

It commits whatever you build (between init and commit), so if you don't include favorites they won't be there. We should just disable the shutdown commit for now and I'll try and address this in widget code later on.
Attached patch patch v1.1 (obsolete) — — Splinter Review 
- added a simple unit test for the API
- don't update at shutdown since if we do that it is IO on shutdown (and possibly fails due to the fact Places shutdowns earlier), if we sync only tasks history lists are lost. As Jim said the backend should allow partial replacements through a cache.
- cancel statements and list build if nested updates are requested

Shawn, can you please review the Places part? Jim can review the module changes.
Attachment #479845 - Attachment is obsolete: true
Attachment #480097 - Flags: review?(sdwilsh)
Attachment #479845 - Flags: feedback?(sdwilsh)
Shawn: also see my question above, should I move this API to nsPIPlacesDatabase as a special private API or can we live with its current position?
Comment on attachment 479841 [details] [diff] [review]
cleanup v1.1

Doa should really do this, I'm not a peer here and I don't work on a lot of browser script.
Attachment #479841 - Flags: review?(jmathies) → review?(dao)
auto-comment
var query = _PlacesUtils.history.getNewQuery();

the underscore is wrong, looks like I fixed it locally but not here.
Whiteboard: [needs review]
(In reply to comment #44)
> Shawn: also see my question above, should I move this API to nsPIPlacesDatabase
> as a special private API or can we live with its current position?
We should move it to nsPIPlacesDatabase and have big text that it will be removed in the future and people should not rely on it (until we get async query api done).
Comment on attachment 479841 [details] [diff] [review]
cleanup v1.1

r+ with the typo fixed...
Attachment #479841 - Flags: review?(dao) → review+
Depends on: 594821
Attached patch cleanup v1.2 — — Splinter Review 
fixed typo.
Attachment #479841 - Attachment is obsolete: true
So is it possible for us to revert the change in http://hg.mozilla.org/mozilla-central/rev/ac0f0d30e66a when this is fixed?  We kind of need that immediate update, otherwise the jumplist menu might be in an inconsistent state for quite some time after Firerox starts up.  See bug 568816 for an instance of this problem.
Blocks: 568816
(In reply to comment #50)
> So is it possible for us to revert the change in
> http://hg.mozilla.org/mozilla-central/rev/ac0f0d30e66a when this is fixed?

The problem is that we want to avoid databases I/O on startup and shutdown since it's not really needed.
So the short answer is NO (indeed this patch also removed the update on shutdown).

The long answer is that we need to be able to update just a part of the jumplist without having to rebuild it all. On startup and shutdown we need to update the tasks list, but not the history lists. This can only be done in the backend caching the previous values (see comment 40). I think we should file a bug for that and ask for blocking.
(In reply to comment #50)
> So is it possible for us to revert the change in
> http://hg.mozilla.org/mozilla-central/rev/ac0f0d30e66a when this is fixed?  We
> kind of need that immediate update, otherwise the jumplist menu might be in an
> inconsistent state for quite some time after Firerox starts up.  See bug 568816
> for an instance of this problem.

apparently not, the perf hit is too big. mak felt we needed to lose the update on startup and shutdown. so right now the approach is to standardize the running/not-running lists so they don't need to be updated in these cases.

privacy mode is an open issue, maybe forced updates when entering / exiting? extra perf hits there probably isn't as big a deal.

(In reply to comment #51)
> (In reply to comment #50)
> > So is it possible for us to revert the change in
> > http://hg.mozilla.org/mozilla-central/rev/ac0f0d30e66a when this is fixed?
> 
> The problem is that we want to avoid databases I/O on startup and shutdown
> since it's not really needed.
> So the short answer is NO (indeed this patch also removed the update on
> shutdown).
> 
> The long answer is that we need to be able to update just a part of the
> jumplist without having to rebuild it all. On startup and shutdown we need to
> update the tasks list, but not the history lists. This can only be done in the
> backend caching the previous values (see comment 40). I think we should file a
> bug for that and ask for blocking.

caching the uri for these lists across sessions is going to be an interesting problem to solve. seems like this should be up in places rather than in widget. I'm not sure this is something we want to do this late in the ball game. (In the case of caching in widget, there's nobody to work on it anyway.)
(In reply to comment #52)
> caching the uri for these lists across sessions is going to be an interesting
> problem to solve. seems like this should be up in places rather than in widget.

Places does not look like the right place to cache that stuff, because on startup/shutdown it can change (import, clear on shutdown and so on), so we don't have a reason to cache it.
And actually if we clear history on shutdown we could skip a wanted update (thus we could want to listen for clear history)

Ehsan, is "private-browsing" notification fired when user is in persistent pb mode? jumplists are already listening for it, is that enough to fix your use case on startup/shutdown?
(In reply to comment #53)
> (In reply to comment #52)
> > caching the uri for these lists across sessions is going to be an interesting
> > problem to solve. seems like this should be up in places rather than in widget.
> 
> Places does not look like the right place to cache that stuff, because on
> startup/shutdown it can change (import, clear on shutdown and so on), so we
> don't have a reason to cache it.
> And actually if we clear history on shutdown we could skip a wanted update
> (thus we could want to listen for clear history)

On the caching issue, firstly how big is the cost if we do an async query?  I think we should make the queries *fast*, no matter how often we decide to update the list, right?

I don't really see what's so hard about caching the frequently visited pages...

> Ehsan, is "private-browsing" notification fired when user is in persistent pb
> mode? jumplists are already listening for it, is that enough to fix your use
> case on startup/shutdown?

It _is_ fired for permanent PB mode, but the timing isn't right for the jumplist service.  Here's what happens, for example:

1. You enter PB mode.  The jumplist menu item shows Quit PB.
2. You close Firefox.
3. You open it again (in normal mode).  The jumplist menu item still shows Quit PB (oops!).

There is *no way* around this except by updating the task list section of the menu on startup.  If we can't update the frequently visited list in the same time, so be it!  Let's skip it then?  Makes sense?
(In reply to comment #54)
> On the caching issue, firstly how big is the cost if we do an async query?  I
> think we should make the queries *fast*, no matter how often we decide to
> update the list, right?

The new query is much faster and making it async won't lock the ui thread, but we should globally reduce accesses to the disk on startup/shutdown, especially if they are not life-saver for the user.

> I don't really see what's so hard about caching the frequently visited pages...

where would you cache them? in a pref?

> > Ehsan, is "private-browsing" notification fired when user is in persistent pb
> > mode? jumplists are already listening for it, is that enough to fix your use
> > case on startup/shutdown?
> 
> It _is_ fired for permanent PB mode, but the timing isn't right for the
> jumplist service.  Here's what happens, for example:
> 
> 1. You enter PB mode.  The jumplist menu item shows Quit PB.
> 2. You close Firefox.
> 3. You open it again (in normal mode).  The jumplist menu item still shows Quit
> PB (oops!).

Yeah my question was if doing 2. fires "private-browsing" notification. So looks like once I enter PB mode if I close the browser there is no "exit-pb-mode" notification but we persist the mode?
I thought that private-browsing was notified on exiting the browser if it was active, and then activated again on startup.

> There is *no way* around this except by updating the task list section of the
> menu on startup.  If we can't update the frequently visited list in the same
> time, so be it!  Let's skip it then?  Makes sense?

We can't, if you want to even change a single element in the jumplist you have to provide them all, and the jump list is a write-only target, so you can't read its contents.
(In reply to comment #55)
> The new query is much faster and making it async won't lock the ui thread, but
> we should globally reduce accesses to the disk on startup/shutdown, especially
> if they are not life-saver for the user.
Yes, please.  It really is killing us.

> Yeah my question was if doing 2. fires "private-browsing" notification. So
> looks like once I enter PB mode if I close the browser there is no
> "exit-pb-mode" notification but we persist the mode?
> I thought that private-browsing was notified on exiting the browser if it was
> active, and then activated again on startup.
Yeah, I would expect that to fire on shutdown that we were leaving it too.  I don't see how add-ons could be expected to properly track this easily.
(In reply to comment #55)
> > There is *no way* around this except by updating the task list section of the
> > menu on startup.  If we can't update the frequently visited list in the same
> > time, so be it!  Let's skip it then?  Makes sense?
> 
> We can't, if you want to even change a single element in the jumplist you have
> to provide them all, and the jump list is a write-only target, so you can't
> read its contents.

Why don't we just write the tasks lists and do the favorites/frequent lists on the next update? (ehsan, I think that's what you were suggesting?)
(In reply to comment #55)
> (In reply to comment #54)
> > On the caching issue, firstly how big is the cost if we do an async query?  I
> > think we should make the queries *fast*, no matter how often we decide to
> > update the list, right?
> 
> The new query is much faster and making it async won't lock the ui thread, but
> we should globally reduce accesses to the disk on startup/shutdown, especially
> if they are not life-saver for the user.

I'm all in for that!

> > I don't really see what's so hard about caching the frequently visited pages...
> 
> where would you cache them? in a pref?

I'd say on the session object, but I don't really care.  Why is the "where" a question here?

> > > Ehsan, is "private-browsing" notification fired when user is in persistent pb
> > > mode? jumplists are already listening for it, is that enough to fix your use
> > > case on startup/shutdown?
> > 
> > It _is_ fired for permanent PB mode, but the timing isn't right for the
> > jumplist service.  Here's what happens, for example:
> > 
> > 1. You enter PB mode.  The jumplist menu item shows Quit PB.
> > 2. You close Firefox.
> > 3. You open it again (in normal mode).  The jumplist menu item still shows Quit
> > PB (oops!).
> 
> Yeah my question was if doing 2. fires "private-browsing" notification. So
> looks like once I enter PB mode if I close the browser there is no
> "exit-pb-mode" notification but we persist the mode?
> I thought that private-browsing was notified on exiting the browser if it was
> active, and then activated again on startup.

We do get that notification (as I told mak on IRC).  It's "private-browsing", with the data param set to "exit" and the first param set to an nsISupportsPRBool with the value true (to indicate that this is coming from an app quit).

It just happens that the jumplist service is also listening to "quit-application-granted" which is what PB is listening to as well, and it gets called before the PB service, so by the time that we fire this, jumplist is already gone away.

mak said that he has plans to move this to "profile-before-change" or something, which fires after "quit-application-granted".

> > There is *no way* around this except by updating the task list section of the
> > menu on startup.  If we can't update the frequently visited list in the same
> > time, so be it!  Let's skip it then?  Makes sense?
> 
> We can't, if you want to even change a single element in the jumplist you have
> to provide them all, and the jump list is a write-only target, so you can't
> read its contents.

We can still cache the favorite places in memory while the browser is running, and provide two types of update methods for the jumplist service: a fast one (only updating the tasks section) and a slow one (updating favorite places as well), and just call the fast one at startup/shutdown.
(In reply to comment #57)
> (In reply to comment #55)
> > > There is *no way* around this except by updating the task list section of the
> > > menu on startup.  If we can't update the frequently visited list in the same
> > > time, so be it!  Let's skip it then?  Makes sense?
> > 
> > We can't, if you want to even change a single element in the jumplist you have
> > to provide them all, and the jump list is a write-only target, so you can't
> > read its contents.
> 
> Why don't we just write the tasks lists and do the favorites/frequent lists on
> the next update? (ehsan, I think that's what you were suggesting?)

Yes!  This _is_ what I'm suggesting.
Attached patch patch v1.2 — — Splinter Review 
- moved API to nsPIPlacesDatabase
- moved module shutdown to profile-before-change. This allows us to update the list on shutdown if PB mode was active or if the user clears history at shutdown.

We could provide a cached update in some case (when a previous update completed and there is no need for a full update), but the effort looks greater than the gain we'd get back. I'd let that for a follow-up honestly.
Attachment #480097 - Attachment is obsolete: true
Attachment #480097 - Flags: review?(sdwilsh)
Comment on attachment 480762 [details] [diff] [review]
patch v1.2

As before, Shawn for a first-pass, jimm can do final review, please.
Attachment #480762 - Flags: review?(sdwilsh)
Comment on attachment 480762 [details] [diff] [review]
patch v1.2

general nit: brace your one line ifs that you add please.

>+++ b/browser/components/wintaskbar/WindowsJumpLists.jsm
>+  _pendingStatements: [],
Describe this more please since it seems to be a bit more complicated than a simple array.  In fact, it looks more like we use this as an object...

>+  _hasPendingStatements: function WTBJL__hasPendingStatements() {
>+    for (let listType in this._pendingStatements) {
>+      return true;
>+    }
>+    return false;
>+  },
Isn't this just return this._pendingStatements.length > 0?

>+    this._pendingStatements["frequent"] = this._getHistoryResults(
I think I'd rather see this use a constant than a string.

r=sdwilsh, but I think jimm should probably look at this too.  + the sr you need for the api change.
Attachment #480762 - Flags: review?(sdwilsh) → review+
(In reply to comment #62)
> general nit: brace your one line ifs that you add please.

this module style was looking without them, thus I did not add them. I thought we prefer module consistence for existing modules.

> >+++ b/browser/components/wintaskbar/WindowsJumpLists.jsm
> >+  _pendingStatements: [],
> Describe this more please since it seems to be a bit more complicated than a
> simple array.  In fact, it looks more like we use this as an object...

it should be an object indeed, it was an array in a previous version of the patch, this is a leftover.

> >+  _hasPendingStatements: function WTBJL__hasPendingStatements() {
> >+    for (let listType in this._pendingStatements) {
> >+      return true;
> >+    }
> >+    return false;
> >+  },
> Isn't this just return this._pendingStatements.length > 0?

nope because it's an object and __count__ is dead time ago. Sorry for confusion with those [].

> r=sdwilsh, but I think jimm should probably look at this too.  + the sr you
> need for the api change.

yep, I agree.
Attached patch patch v1.3 (obsolete) — — Splinter Review 
addressed Shawn's comments, now up to jimm. I'll look around for a superreviewer in the meanwhile.
Attachment #480762 - Attachment is obsolete: true
Attachment #481702 - Flags: review?
Attachment #481702 - Flags: review? → review?(jmathies)
Comment on attachment 481702 [details] [diff] [review]
patch v1.3

Robert agreed to superreview this in the next days
Attachment #481702 - Flags: superreview?(robert.bugzilla)
Blocks: 602968
Attached patch patch v1.4 — — Splinter Review 
ugh, 1.3 was the same as 1.2.
Attachment #481702 - Attachment is obsolete: true
Attachment #482196 - Flags: superreview?(robert.bugzilla)
Attachment #482196 - Flags: review?(jmathies)
Attachment #481702 - Flags: superreview?(robert.bugzilla)
Attachment #481702 - Flags: review?(jmathies)
Attachment #480762 - Attachment is obsolete: false
Comment on attachment 482196 [details] [diff] [review]
patch v1.4

I just went over the api changes that require sr... if you'd like me to review the rest let me know though I'm not knowledgeable regarding this code.

>diff --git a/toolkit/components/places/public/nsPIPlacesDatabase.idl b/toolkit/components/places/public/nsPIPlacesDatabase.idl
>--- a/toolkit/components/places/public/nsPIPlacesDatabase.idl
>+++ b/toolkit/components/places/public/nsPIPlacesDatabase.idl
>@@ -35,22 +35,43 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
> 
> interface mozIStorageConnection;
>+interface nsINavHistoryQuery;
>+interface nsINavHistoryQueryOptions;
>+interface mozIStorageStatementCallback;
>+interface mozIStoragePendingStatement;
> 
> /**
>  * This is a private interface used by Places components to get access to the
>  * database.  If outside consumers wish to use this, they should only read from
>  * the database so they do not break any internal invariants.
>  */
>-[scriptable, uuid(5fd91813-229c-4d30-851b-700afa39a987)]
>+[scriptable, uuid(6eb7ed3d-13ca-450b-b370-15c75e2f3dab)]
> interface nsPIPlacesDatabase : nsISupports
> {
>   /**
>    * The database connection used by Places.
>    */
>   readonly attribute mozIStorageConnection DBConnection;
>+
>+  /**
>+   * Async executes the statement created from queries.
s/Async/Asynchronously/ executes a statement

>+   *
>+   * @see nsINavHistoryService::executeQueries
>+   * @note THIS IS A TEMPORARY API.  Don't rely on it, since it will be replaced
>+   *       in future versions by a real async querying API.
>+   * @note Results obtained from this method differ from results obtained from
>+   *       executeQueries, because there is additional filtering and sorting
>+   *       done by the latter.  Thus you should use executeQueries, unless you
>+   *       are absolutely sure that returned results are fine for your use-case.
s/that returned/that the returned/

>+   */
>+  mozIStoragePendingStatement asyncExecuteQueries(
Since this is temporary and you will likely be adding a new version in the future perhaps it would be better to use a different name than asyncExecuteQueries so the future version can just have it vs. take this one over? I'll leave that to your discretion.

>+    [array,size_is(aQueryCount)] in nsINavHistoryQuery aQueries,
nit: space after comma.
array, size_is = Found 171 matching lines in 85 files
array,size_is = Found 23 matching lines in 13 files

>+    in unsigned long aQueryCount,
>+    in nsINavHistoryQueryOptions options,
should be aOptions

>+    in mozIStorageStatementCallback aCallback);
> };

sr=me with the above changes.
Attachment #482196 - Flags: superreview?(robert.bugzilla) → superreview+
(In reply to comment #67)
> I just went over the api changes that require sr... if you'd like me to review
> the rest let me know though I'm not knowledgeable regarding this code.

the idl is fine, thanks.  The code will have 2 reviews, those should be enough.

> >diff --git a/toolkit/components/places/public/nsPIPlacesDatabase.idl > >+  mozIStoragePendingStatement asyncExecuteQueries(
> Since this is temporary and you will likely be adding a new version in the
> future perhaps it would be better to use a different name than
> asyncExecuteQueries so the future version can just have it vs. take this one
> over? I'll leave that to your discretion.

I've renamed the method to asyncExecuteLegacyQueries, indeed we will probably provide a new query API, even if I'm unsure it will be available through XPCOM. Your concern is valid though, if we would fix all the underlying code that filters and sorts out of SQL, we could most likely move the API to the official history one, thus using a proper name. Thus the rename.
Attached patch patch v1.5 — — Splinter Review 
fixed SR comments
Attachment #482511 - Flags: review?(jmathies)
Attachment #482196 - Flags: review?(jmathies)
Whiteboard: [needs review] → [needs review jimm]
note to myself: I should move the implementation in nsNavHistory.cpp below the nsIPIPlacesDatabase section comment
(In reply to comment #69)
> Created attachment 482511 [details] [diff] [review]
> patch v1.5
> 
> fixed SR comments

mak, what patches do I need in this series to get this to apply cleanly? The WindowsJumpLists.jsm parts aren't applying.
cleanup v1.2 and patch v1.5
Comment on attachment 482511 [details] [diff] [review]
patch v1.5


>   _shutdown: function WTBJL__shutdown() {
>     this._shuttingDown = true;
>-    this.update();
>+
>+    // Correctly handle a clear history on shutdown.  If there are no
>+    // entries be sure to empty all history lists.  Luckily Places caches
>+    // this value, so it's a pretty fast call.
>+    if (!PlacesUtils.history.hasHistoryEntries) {
>+      this.update();
>+    }
>+
>     this._free();
>   },

We should flip all the close flags to true in tasksCfg so that our task list always matches regardless of how we shutdown.

>diff --git a/toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js b/toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js
>@@ -0,0 +1,102 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */

Is this an acceptable license? I thought everything had to have the MPL.

You might want to get Dao to review syntax of the script as I'm not a peer here.

Functionality wise it looks to be working well.
Attachment #482511 - Flags: review?(jmathies) → review+
> Is this an acceptable license? I thought everything had to have the MPL.

For simple tests, PD is better (mainly because shorter).

/be
licenses are also documented in http://www.mozilla.org/MPL/license-policy.html
"The following license is acceptable for trivial pieces of Support Code, such as testcase"

> You might want to get Dao to review syntax of the script as I'm not a peer
> here.

I think sdwilsh review covered the script changes as well, Shawn?
Attached patch patch v1.6 — — Splinter Review 
Addressed comment, moved the implementation in nsNavHistory.cpp. Should be ready to land once Shawn confirms his review was also on the script syntax.
(In reply to comment #75)
> I think sdwilsh review covered the script changes as well, Shawn?
Pretty sure my review is fine here.
part 1: http://hg.mozilla.org/mozilla-central/rev/d32b0acce090
part 2: http://hg.mozilla.org/mozilla-central/rev/36043368b3dd
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs review jimm]
(In reply to comment #63)
> > Isn't this just return this._pendingStatements.length > 0?
> 
> nope because it's an object and __count__ is dead time ago. Sorry for confusion
> with those [].

Fwiw, we have Object.keys now: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/keys , so this could have been Object.keys(this._pendingStatements).length > 0.
(In reply to comment #79)
> Fwiw, we have Object.keys now:
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/keys
> , so this could have been Object.keys(this._pendingStatements).length > 0.

Oh cool, thanks for the hint, we could fix it when touching the module in future.
Depends on: 609592
Depends on: 870529
Hello,
Can you please take a look on bug 870529?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: