836443 - Automatically stop and restart downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the jsdownloads API, when the browser is closed, or when the last private
browsing window is closed, all the in-progress downloads should be stopped,
after notifying the user if necessary. When the browser is then reopened,
downloads that were in progress should be restarted automatically.

Comment 1

[:Paolo Amadini]

Attachment: The patch

This implements persistence across sessions, using a delayed save model that
doesn't require anything special to be done on shutdown. This model is subject
to the usual shutdown race conditions, as described in the code, but this is
unavoidable unless we support asynchronous operations that block shutdown.

I tested it with several cases of paused and in-progress downloads and it
correctly keeps all the visible properties across sessions.

Comment 2

[Neil Deakin]

Comment on attachment 788528 [details] [diff] [review]
The patch

>-    if (this.contentType) {
>-      serializable.contentType = this.contentType;
>+    // These properties are serialized if they don't evaluate to false.

They actually get serialized 'if (this[property])', rather than specifically being false, meaning that they don't get serialized when null or undefined either.


>+  // If the download should not be restarted automatically, update its state to
>+  // reflect success or failure during a previous session.
>+  if (!("stopped" in aSerializable) || aSerializable.stopped) {

Could you just write aSerializable.stopped !== false ? There's another similar line in DownloadStore.load as well.


>+ * Conversely, if the browser is closed before this interval has passed and the
>+ * download is still in progress, it will not be restored in the next session.
>+ * If the download has partial data associated with it, then the ".part" file
>+ * will not be deleted when the browser starts again.

Does this mean that the download will get lost? Or just that it doesn't get restarted automatically?


>+    // Load the list of persistent downloads, then add the DownloadAutoSaveView
>+    // even if the load operation failed.
>+    return this._store.load().then(null, Cu.reportError).then(() => {
>+      new DownloadAutoSaveView(aList, this._store);
>+    });

I'm not as familiar with this, but what happens if you write:

    return this._store.load().then(() => { new DownloadAutoSaveView(aList, this._store); },
                                   Cu.reportError);


>+function DownloadAutoSaveView(aList, aStore) {
>+  this._store = aStore;
>+  this._downloadsMap = new Map();
>+
>+  // Load all the data before allowing changes to be detected.
>+  aList.addView(this);
>+  this._initialized = true;

The comment suggests some loading happens here but I don't see that. I think what you mean to say here is that
_initialized is set to true after adding the view so that onDownloadAdded doesn't cause a save to occur.


>         return;
>       }
> 
>       let storeData = JSON.parse(gTextDecoder.decode(bytes));
> 
>       // Create live downloads based on the static snapshot.
>       for (let downloadData of storeData.list) {
>         try {
>-          this.list.add(yield Downloads.createDownload(downloadData));
>+          let download = yield Downloads.createDownload(downloadData);
>+          try {
>+            if (("stopped" in downloadData) && !downloadData.stopped) {
>+              // After the download has been added to the list, try to restart
>+              // it if it was in progress during the previous session.
>+              download.start();

The comment says 'After the download has been added to the list, try to restart...' but you restart the download before adding it to the list.

Comment 3

[:Paolo Amadini]

Attachment: Updated patch

I added a "refresh" method, since it will have a general utility later (to
check if the target file has been moved and disable the option to open) and
for now will be required also during the import from "downloads.sqlite".

(In reply to Neil Deakin from comment #2)
> They actually get serialized 'if (this[property])', rather than specifically
> being false, meaning that they don't get serialized when null or undefined
> either.

Clarified the comment, mentioning the three specific cases we may encounter
(false, null, empty string) rather than generically saying "evaluate to
false". Also reworded the other comments.

> >+  if (!("stopped" in aSerializable) || aSerializable.stopped) {
> 
> Could you just write aSerializable.stopped !== false ?

I believe that would trigger a JavaScript strict warning if "stopped" is not
in aSerializable.

>     return this._store.load().then(() => { new DownloadAutoSaveView(aList,
> this._store); },
>                                    Cu.reportError);

In that case, the view would not be added if "load" fails.

Comment 4

[Neil Deakin]

Comment on attachment 789644 [details] [diff] [review]
Updated patch

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

>> >+  if (!("stopped" in aSerializable) || aSerializable.stopped) {
>> 
>> Could you just write aSerializable.stopped !== false ?

> I believe that would trigger a JavaScript strict warning if "stopped" is not
> in aSerializable.

if (aSerializable.stopped) is a special case that doesn't trigger a warning.

Comment 5

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/81e6133aa152

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/81e6133aa152

Comment 7

[Mihai Morar, (:MihaiMorar)]

Verified as fixed on Latest Nightly 27. BuildID: 20130929030202