Closed Bug 908256 Opened 6 years ago Closed 6 years ago

Automatically stop and restart downloads when suspending or going offline

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 --- verified

People

(Reporter: Paolo, Assigned: enndeakin)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Anyone working on this? Otherwise, I'll take a look.
(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.
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?
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #801631 - Flags: feedback?(paolo.mozmail)
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.
Attachment #801631 - Flags: feedback?(paolo.mozmail)
Attached patch offline-pause (obsolete) — Splinter Review
Attachment #801631 - Attachment is obsolete: true
Attachment #803109 - Flags: review?(paolo.mozmail)
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.
Attachment #803109 - Flags: review?(paolo.mozmail) → feedback+
Attachment #803109 - Attachment is obsolete: true
Attachment #804941 - Flags: review?(paolo.mozmail)
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.
Attachment #804941 - Flags: review?(paolo.mozmail) → review+
Enn, do you plan to attach an updated patch or should I go ahead and push with changes?
Attached patch Updated patchSplinter Review
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.
Attachment #804941 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1b1707ee3c37
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #806136 - Flags: approval-mozilla-aurora+
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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.