Closed
Bug 836443
Opened 12 years ago
Closed 11 years ago
Automatically stop and restart downloads
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
No longer blocks: jsdownloads
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 7•11 years ago
|
||
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.
Description
•