874808 - simpleDownload should accept string arguments and options

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The Downloads.simpleDownload API currently accepts an nsIURI source and an
nsIFile target, in addition to objects resembling a serialized representation
of DownloadSource and DownloadTarget.

For better interoperability with OS.File, the API should be updated to accept
simple string arguments as well. In fact, the internal representation of both
source and target, when serialized, is a simple string, so there is no
information loss involved.

A third aOptions arguments should allow the caller to specify options in the
style of OS.File, like { isPrivate: true }.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[:Paolo Amadini]

Comment on attachment 759700 [details] [diff] [review]
v1

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +447,5 @@
> +   * @return the url or empty string.
> +   */
> +  toString: function DS_toString() {
> +    return this.uri ? this.uri.spec : "";
> +  }

While this is not a bad idea, I'd leave the toString functions out for now, as I'm not sure whether they should only provide the spec or a more complete object representation for debugging. In the meantime, I'd not want our code to inadvertently start depending on the spec behavior.

We can decide what we want to do with them later.

@@ +472,5 @@
> +   * @return the file path or empty string.
> +   */
> +  toString: function DT_toString() {
> +    return this.file ? this.file.path : "";
> +  }

Same here.

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +41,5 @@
>                                    "resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
> +                                  "resource://gre/modules/FileUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +                                  "resource://gre/modules/NetUtil.jsm");

global-nit: can you please sort the defineLazyModuleGetter calls alphabetically, by the short name in the second parameter? (this is just a nit, so I'm taking the occasion of the short patch to mention it)

@@ +113,5 @@
>     * reference to a Download object using the createDownload function.
>     *
>     * @param aSource
> +   *        The nsIURI or url for the download source, or alternative
> +   *        DownloadSource.

"The nsIURI or string containing the URI spec for the download source"

@@ +118,3 @@
>     * @param aTarget
> +   *        The nsIFile or file path for the download target, or
> +   *        alternative DownloadTarget.

"The nsIFile or string containing the file path"

@@ +134,5 @@
>      if (aSource instanceof Ci.nsIURI) {
>        aSource = { uri: aSource };
> +    } else if (typeof aSource == "string") {
> +      aSource = { uri: NetUtil.newURI(aSource) };
> +    }

We should also handle the "new String('...')" case, I think.

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
@@ +89,5 @@
> +add_task(function test_simpleDownload_string_arguments()
> +{
> +  let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
> +  let download = yield Downloads.simpleDownload(TEST_SOURCE_URI.spec,
> +                                                targetFile.path);

And add a separate test case for the "new String('...')" case.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

> @@ +134,5 @@
> >      if (aSource instanceof Ci.nsIURI) {
> >        aSource = { uri: aSource };
> > +    } else if (typeof aSource == "string") {
> > +      aSource = { uri: NetUtil.newURI(aSource) };
> > +    }
> 
> We should also handle the "new String('...')" case, I think.

I've tried to use condition aSource instanceof String but it doesn't work.  I believe some properties get lost when passing parameter into simpleDownload().  Therefore I used (typeof aSource == "object" && "charAt" in aSource))

Comment 4

[:Paolo Amadini]

Comment on attachment 760279 [details] [diff] [review]
v2

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

(In reply to Raymond Lee [:raymondlee] from comment #3)
> I've tried to use condition aSource instanceof String but it doesn't work. 
> I believe some properties get lost when passing parameter into
> simpleDownload().  Therefore I used (typeof aSource == "object" && "charAt"
> in aSource))

That's fine, thanks! r+ with the changes below, assuming this passes all tests.

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +118,2 @@
>     * @param aTarget
> +   *        The nsIFile or string containing the file path.

These can still be objects like DownloadSource or DownloadTarget, and this should be reflected in the comments, sorry for being unclear.

@@ +142,4 @@
>      }
>      if (aTarget instanceof Ci.nsIFile) {
>        aTarget = { file: aTarget };
> +    } else if (typeof aTarget == "string" || 

nit: whitespace at end of line.

If you have a function in your text editor that removes all the whitespace at end of line from the file, it should work fine on files in the jsdownloads folder (while in other parts of the code this makes patches unmanageable because the files already have a lot of unneeded whitespace).

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +29,5 @@
>    do_check_true(download.source.uri.equals(TEST_SOURCE_URI));
>    do_check_eq(download.target.file, targetFile);
>    do_check_true(download.source.referrer === null);
> +  do_check_true(download.source.toString() === TEST_SOURCE_URI.spec);
> +  do_check_true(download.target.toString() === targetFile.path);

Leftover from the previous patch?

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #5)
> Created attachment 760954 [details] [diff] [review]
> Patch for check-in

Updated the patch based on comment 4

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=fe3ef638aa78

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/691667f48b24

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/691667f48b24

Comment 9

[Jeremie Patonnier :Jeremie]

Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885263