916430 - General Downloads API review

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[:Paolo Amadini]

Attachment: The patch

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.

Comment 2

[:Paolo Amadini]

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.

Comment 3

[Neil Deakin]

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?

Comment 4

[:Paolo Amadini]

(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.

Comment 5

[:Paolo Amadini]

The recent try run on the previous patch is mostly green until now:

https://tbpl.mozilla.org/?tree=Try&rev=bc019bdb5589

Comment 6

[:Paolo Amadini]

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

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

Did you file a separate bug for the Temporary/User/System folders naming?

Comment 9

[:Paolo Amadini]

(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.

Comment 10

[:Paolo Amadini]

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

Comment 11

[:Paolo Amadini]

Attachment: Updated patch

Comment 12

[:Paolo Amadini]

(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!

Comment 13

[Ed Morley [:emorley]]

Unfortunately this conflicted with the backout of bug 913110, so a bunch of things, including this had to be backed out for now:
remote:   https://hg.mozilla.org/integration/fx-team/rev/5736f23d8a22
remote:   https://hg.mozilla.org/integration/fx-team/rev/fd257eef04b2
remote:   https://hg.mozilla.org/integration/fx-team/rev/00593fe07777
remote:   https://hg.mozilla.org/integration/fx-team/rev/ea0b23759685
remote:   https://hg.mozilla.org/integration/fx-team/rev/cbc7af0a20c4

Comment 14

[:Paolo Amadini]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a15a1117ddf1

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Same as with bug 913110 - please fill out a proper approval request that addresses the risks/reward of uplifting this.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Apologies for the flag mid-air, hadn't seen Lukas' comments before approving. I posted some reasoning in bug 913110 comment 23.

Comment 18

[:Paolo Amadini]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b63d1249e70c

Comment 19

[Mihai Morar, (:MihaiMorar)]

Marking as [qa-] based on the existing mochitest on the patch and on the in testsuite flag.