| This bug describes changes to the version of the downloads panel and | indicator that is currently present in the UX branch, more specifically: | * Bug 564934, attachment 558500 [details] [diff] [review] | * Bug 663772, attachment 558504 [details] [diff] [review] | | Patches for this bug should apply on top of the two mentioned patches. Completed items (finished or canceled, as opposed to in-progress or paused) should disappear from the list as soon as the last browser window is closed, unless the last window is closed because of a planned restart (e.g. browser update) or in case of crashes. However, in the latter cases, if it's too much work to adapt the current patch, it's also compatible that completed downloads disappear as well. If the last browser window is closed but the application is not restarted, for any reason, completed downloads should disappear as well.

This patch should land on the UX branch to change the way downloads are handled between sessions. There is no need for a full review, but feedback is welcome. If we think that the NO_SESSION case is rare enough, the code here can be further simplified by always loading all the downloads from the database. Tryserver build running here: https://tbpl.mozilla.org/?tree=Try&rev=084ee9eb1275

I have just pushed this patch to the UX branch: https://tbpl.mozilla.org/?tree=UX&rev=a9cf107a40cb It should appear in tomorrow's nightly build on the UX branch.

Comment on attachment 574273 [details] [diff] [review] The patch Review of attachment 574273 [details] [diff] [review]: ----------------------------------------------------------------- the patch looks fine, apart a couple details. As a more general thought, we have to do work on shutdown that ideally we should try to avoid. This may be something to look into in future, since we should be as light as possible on startup and shutdown paths. Maybe we could use a time filter to ignore old completed downloads and remove them asynchronously during the app lifecycle. Btw, follow-up thoughts mostly. ::: browser/components/downloads/src/DownloadsStartup.js @@ +177,5 @@ > + // Delay so that the Download Manager service, if already initialized, > + // has a chance to pause or cancel in-progress downloads. > + this._executeSoon(function() { > + Services.downloads.cleanUp(); > + }); this worries me, since between quit-application and xpcom-shutdown we unlikely have pauses in the events loop, so this likely ends up being handled after xpcom-shutdown, that is not really nice. you may have to listen also to profile-before-change or any other topic that comes after quit-application and do the work there synchronously. Does the dm service do its work at quit-application? @@ +203,2 @@ > */ > + _recoverAllDownloads: false, the "All" clashes with "load completed items", I think that means "load all items, included completed ones," but that's not well expressed

(In reply to Marco Bonardo [:mak] from comment #3) > this worries me, since between quit-application and xpcom-shutdown we > unlikely have pauses in the events loop, so this likely ends up being > handled after xpcom-shutdown, that is not really nice. > you may have to listen also to profile-before-change or any other topic that > comes after quit-application and do the work there synchronously. > Does the dm service do its work at quit-application? Ouch! Yes, the Download Manager pauses or stops in-progress downloads at "quit-application". Maybe I can set a flag indicating whether we're restarting, then invoke the actual cleanup if needed in "profile-change-teardown" (whose name suggests that we're shutting down). Regarding startup instead, what do you think about special-casing NO_SESSION? Is that really a rare case as I suspect? A special case would've been more useful if we knew for sure when we crashed or restarted, but the type of session doesn't give us that information: a crash can appear as DEFER_SESSION (meaning that we show the page asking which other pages to reopen after the crash).

(In reply to Paolo Amadini from comment #4) > Ouch! Yes, the Download Manager pauses or stops in-progress downloads at > "quit-application". Maybe I can set a flag indicating whether we're > restarting, > then invoke the actual cleanup if needed in "profile-change-teardown" (whose > name suggests that we're shutting down). profile-change-teardown sounds good, it is fired between quit-application and profile-before-change. > Regarding startup instead, what do you think about special-casing NO_SESSION? > Is that really a rare case as I suspect? Well, some user may never have a session because they setup to never restore the session. I'm not sure how sessionstore behaves in such a case (if really the session exists but is ignored, or we also save time not saving one), you may ask zpao about how often it happens. Sessionstore has so many special cases that I prefer letting someone more expert give you a decent answer.

The patch on this bug is now included as part of the patch in bug 726444. Any pending issue here should have already been filed as one of the bug's follow-ups.