697686 - [Downloads Panel] Clear completed items from the list when the last browser window is closed

Status: RESOLVED DUPLICATE of bug 726444

(Firefox :: General, defect)

Description

[:Paolo Amadini]

| 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.

Comment 1

[:Paolo Amadini]

Attachment: The patch

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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 3

[Marco Bonardo [:mak]]

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

Comment 4

[:Paolo Amadini]

(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).

Comment 5

[Marco Bonardo [:mak]]

(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.

Comment 6

[:Paolo Amadini]

Attachment: Revised patch

Comment 7

[:Paolo Amadini]

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.