Closed Bug 836443 Opened 8 years ago Closed 8 years ago

Automatically stop and restart downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla26

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 881062
No longer blocks: jsdownloads
Blocks: 899122
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attached patch The patch (obsolete) — Splinter Review
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.
Attachment #788528 - Flags: review?(enndeakin)
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.
Attached patch Updated patchSplinter Review
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.
Attachment #788528 - Attachment is obsolete: true
Attachment #788528 - Flags: review?(enndeakin)
Attachment #789644 - Flags: review?(enndeakin)
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.
Attachment #789644 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/81e6133aa152
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Verified as fixed on Latest Nightly 27. BuildID: 20130929030202
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.