Closed Bug 916430 Opened 9 years ago Closed 9 years ago

General Downloads API review

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 + fixed
firefox27 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

We decided to implement some changes to the API early, to pave the way for
future improvements, as well as clarify the usage of some methods.

In particular, the DownloadList methods should return completion promises.
The simpleDownload method will be renamed to something clearer.
Attached patch The patch (obsolete) — Splinter Review
I've renamed simpleDownload to startDirect, or we may call it just "start".

The fact that calling "yield download.start()" waits for the download to be finished still look weird, but I haven't found a better name or alternate API
that hasn't more drawbacks than benefits.
Attachment #804873 - Flags: review?(mak77)
Note that I've not changed the download directory name getters in this patch, as those methods are still unused and we still have to examine how they are supposed to work exactly.
Attachment #804873 - Flags: review?(mak77) → review?(enndeakin)
Comment on attachment 804873 [details] [diff] [review]
The patch

 function DownloadAutoSaveView(aList, aStore) {
+  this._list = aList;
   this._store = aStore;
   this._downloadsMap = new Map();
-
-  // We set _initialized to true after adding the view, so that onDownloadAdded
-  // doesn't cause a save to occur.
-  aList.addView(this);
-  this._initialized = true;
 }

Add a comment to indicate that the caller needs to call initialize() on this object. Maybe the list and the store should be passed to initialize() rather than to the constructor.


>DownloadSummary.prototype = {
...
>   /**
>    * Removes a view that was previously added using addView.  The removed view
>    * will not receive any more notifications after this method returns.
>    *

You should remove the last sentence from this DownloadSummary.remove comment.


diff --git a/toolkit/components/jsdownloads/test/unit/head.js b/toolkit/components/jsdownloads/test/unit/head.js
         // When the download object is ready, make it available to the caller.
-        deferred.resolve(aDownload);
+        promise.then(() => deferred.resolve(aDownload))
+               .then(null, do_report_unexpected_exception);
       },
@@ -416,26 +417,27 @@ function promiseStartExternalHelperAppSe
         // When the download object is ready, make it available to the caller.
-        deferred.resolve(aDownload);
+        promise.then(() => deferred.resolve(aDownload))
+               .then(null, do_report_unexpected_exception);
       },

What exception would occur here that would warrant two separate then() calls?
Attachment #804873 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #3)
> Add a comment to indicate that the caller needs to call initialize() on this
> object. Maybe the list and the store should be passed to initialize() rather
> than to the constructor.

Added the comment, passing the list and the store in the constructor still seems more similar to what we do in other similar objects.

> -        deferred.resolve(aDownload);
> +        promise.then(() => deferred.resolve(aDownload))
> +               .then(null, do_report_unexpected_exception);
>        },
> 
> What exception would occur here that would warrant two separate then() calls?

Most probably none ("deferred" can't become null), I've updated this.
The recent try run on the previous patch is mostly green until now:

https://tbpl.mozilla.org/?tree=Try&rev=bc019bdb5589
Comment on attachment 804873 [details] [diff] [review]
The patch

This bug makes some changes to make the Downloads API clearer. We should
ensure it gets into the first Aurora build so that add-on developers do not
have to deal with a different API in different Aurora builds.

Testing completed on try, not landed on fx-team yet because of a tree closure:
https://tbpl.mozilla.org/?tree=Try&rev=bc019bdb5589
Attachment #804873 - Flags: approval-mozilla-aurora?
Comment on attachment 804873 [details] [diff] [review]
The patch

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

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +686,1 @@
>    

while here you may remove the trailing spaces

@@ +698,1 @@
>    

while here you may remove the trailing spaces

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +224,3 @@
>        new DownloadHistoryObserver(aList);
> +
> +      // 

leftover

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +131,5 @@
>     * @return {Promise}
>     * @resolves When the download has finished successfully.
>     * @rejects JavaScript exception if the download failed.
>     */
> +  startDirect: function (aSource, aTarget, aOptions) {

The concept of a "direct" download is fancy, what is an indirect download?
At that point a straight start() looks better.
We may play with adjectives, but nothing I can think of can well describe an unmanageable and silent download. At a maximum startInBackground, but it's not the others are not in background.
Alternatively, I may think of replacing start() with fetch(), since if you yield on it you get everything. Downloads.fetch(sourceUrl, targetFile) sounds good to me and it's clear you are not creating a Download.
Did you file a separate bug for the Temporary/User/System folders naming?
(In reply to Marco Bonardo [:mak] from comment #7)
> Alternatively, I may think of replacing start() with fetch(), since if you
> yield on it you get everything. Downloads.fetch(sourceUrl, targetFile)
> sounds good to me and it's clear you are not creating a Download.

That sounds good to me too. Too bad I *just* checked in the current patch:

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

I'll make a follow-up patch to change the name.
Attached patch Updated patchSplinter Review
Attachment #804873 - Attachment is obsolete: true
Attachment #804873 - Flags: approval-mozilla-aurora?
Attachment #805888 - Flags: approval-mozilla-aurora?
Depends on: 913110
(In reply to Marco Bonardo [:mak] from comment #8)
> Did you file a separate bug for the Temporary/User/System folders naming?

Just filed bug 917217, thanks for the reminder!
https://hg.mozilla.org/mozilla-central/rev/a15a1117ddf1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Same as with bug 913110 - please fill out a proper approval request that addresses the risks/reward of uplifting this.
Attachment #805888 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Apologies for the flag mid-air, hadn't seen Lukas' comments before approving. I posted some reasoning in bug 913110 comment 23.
Marking as [qa-] based on the existing mochitest on the patch and on the in testsuite flag.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.