908256 - Automatically stop and restart downloads when suspending or going offline

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

We should automatically stop and restart downloads for these notifications:

- "sleep_notification" / "wake_notification" (with delay)
- "suspend_process_notification" / "resume_process_notification"
  (with delay, this is specific to Metro apps)
- "network:offline-about-to-go-offline" / "network:offline-status-changed"
  (the latter when invoked with the "online" indication)

The "browser.download.manager.resumeOnWakeDelay" preference defines the wait
delay for the two suspend cases invoked by the operating system.

We should keep a Set of the downloads that we stopped, and auto-restart only
those, if they still exist when the application is resumed. I don't think it's
important to persist this set of downloads to be auto-restarted. In case the
browser is closed while in offline mode, they may just not be auto-restarted.

Comment 1

[Neil Deakin]

Anyone working on this? Otherwise, I'll take a look.

Comment 2

[Raymond Lee [:raymondlee]]

(In reply to Neil Deakin from comment #1)
> Anyone working on this? Otherwise, I'll take a look.

I don't think so.  Please take a look.

Comment 3

[Neil Deakin]

Attachment: Implement offline/online notifications

I had to add a timeout to the test, but I don't think it will necessarily be reliable. The test wants to know when the downloads have finished restarting.

Or perhaps I should create a new set of downloads instead?

Comment 4

[:Paolo Amadini]

Comment on attachment 801631 [details] [diff] [review]
Implement offline/online notifications

(In reply to Neil Deakin from comment #3)
> I had to add a timeout to the test, but I don't think it will necessarily be
> reliable. The test wants to know when the downloads have finished restarting.

I guess you should just wait for all the individual downloads to finish, using
promiseDownloadStopped, whenSucceeded or a similar method.  You have a reference
to the downloads so it shouldn't be difficult.

> Or perhaps I should create a new set of downloads instead?

I think we should use a different Set than the one used to determine if there
are active downloads. Actually, canceling the downloads should have the
effect of removing them from the active downloads set, so that if the browser
is closed while offline, for example, we don't get a second notification in
addition to the one given before we went offline.

Comment 5

[Neil Deakin]

Attachment: offline-pause

Comment 6

[:Paolo Amadini]

Comment on attachment 803109 [details] [diff] [review]
offline-pause

Review of attachment 803109 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +696,5 @@
>     */
>    _privateInProgressDownloads: new Set(),
>  
>    /**
> +   * Set that contains the in downloads that have been canceled when going offline

"the in"

@@ +775,5 @@
> +    this._wakeTimer = null;
> +
> +    for (let download of this._canceledOfflineDownloads) {
> +      download.start();
> +    }

The download object may be removed from the list while we are offline, in which case we restart a download that doesn't exist anymore.

So, either we ensure the download is removed from the Set when the view is notified, or we use a WeakMap, enumerate getAll(), and only restart those downloads that are still found in the map.

We should add a test for this case also.

Comment 7

[Neil Deakin]

Attachment: Pause downloads when going offline

Comment 8

[:Paolo Amadini]

Comment on attachment 804941 [details] [diff] [review]
Pause downloads when going offline

Review of attachment 804941 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +789,5 @@
>          downloadsSet.delete(aDownload);
> +        // The download must also be removed from the canceled when offline set.
> +        if (this._canceledOfflineDownloads.has(aDownload)) {
> +          this._canceledOfflineDownloads.delete(aDownload);
> +        }

You don't need to check "has", just call "delete" (it returns false if the element did not exist, you can ignore that).

@@ +894,5 @@
> +        try {
> +          wakeDelay = Services.prefs.getIntPref("browser.download.manager.resumeOnWakeDelay");
> +        } catch(e) {}
> +
> +        if (wakeDelay > 0) {

This check is >= 0 in the old implementation. Not sure if setting the delay to -1 as a way to disable resuming is anything we intentionally supported. Setting the delay to 0 should make the resume happen almost immediately.

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +381,5 @@
> +  yield download2.whenSucceeded();
> +  yield download3.whenSucceeded();
> +  yield download4.whenSucceeded();
> +
> +  Services.prefs.setIntPref("browser.download.manager.resumeOnWakeDelay", oldWakeDelay);

I think clearUserPref should work fine here, without needing to store the old value explicitly.

Comment 9

[:Paolo Amadini]

Enn, do you plan to attach an updated patch or should I go ahead and push with changes?

Comment 10

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b1707ee3c37

Comment 11

[:Paolo Amadini]

Attachment: Updated patch

Since this landed mozilla-inbound while other patches landed on fx-team, I'm
attaching a version rebased on fx-team in case this can facilitate the merge.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b1707ee3c37

Comment 13

[:Paolo Amadini]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c59189443334

Gavin's comment in bug 913110 comment 23 contains a general summary about the reason for tracking Firefox 26.

Comment 14

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on Latest Aurora and Latest Nightly using Windows 7x64, Mac OS 10.8.4 and Ubuntu 13.04 x86_64