851461 - Make the JavaScript API for downloads available in parallel to nsIDownloadManager

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

An initial version of the JavaScript API for downloads should become available
to Toolkit consumers before nsIDownloadManager is actually removed, so that
add-ons and host application front-ends can start experimenting with it.

The API would be present in the background, and would be activated only if an
add-on uses it. Ideally, it should be possible to redirect downloads initiated
through nsITransfer to the new API using a hidden about:config preference.
Even when the preference is not set, consumers that use the new API both to
start and to view downloads should be able to see the downloads they start.

A preliminary super-review on the overall API calls is welcome before making
the API available, and early documentation of the validated API on the
Mozilla Developer Network site would also be appropriate at that point.

Comment 1

[:Paolo Amadini]

Attachment: The patch

This enables the JavaScript API for downloads for all Toolkit consumers.

This does not affect the behavior of existing consumers, though in Firefox
there is now a preference (that is read in the Downloads component on startup
for convenience) to redirect legacy downloads to the new API. Together with the
patch in bug 847863, this allows downloads to go through the new API and be
seen in the Downloads Panel.

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

Forgot to update the xpcshell tests to always create the nsITransfer
implementation of the JavaScript API. Original and fixed tryserver
builds are running here:

https://tbpl.mozilla.org/?tree=Try&rev=2942eefaf723
https://tbpl.mozilla.org/?tree=Try&rev=39b42d981741

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 726736 [details] [diff] [review]
Updated patch

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

please reflag when answering questions

::: browser/components/downloads/src/DownloadsStartup.js
@@ +32,5 @@
>                                     "@mozilla.org/browser/sessionstartup;1",
>                                     "nsISessionStartup");
>  
>  const kObservedTopics = [
> +  "profile-after-change",

just add category profile-after-change to http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/BrowserDownloads.manifest#3

While there, is there a specific reason we need to begin at app-startup compared to just putting everything in profile-after-change?

@@ +102,5 @@
> +      case "profile-after-change":
> +        // If the integration preference is enabled, override Toolkit's
> +        // nsITransfer implementation with the one from the JavaScript API for
> +        // downloads.  This should be used only by developers while testing new
> +        // code that uses the JavaScript API, and will eventually be removed.

add bug number to remove this

::: toolkit/components/jsdownloads/src/Downloads.manifest
@@ +1,3 @@
>  component {1b4c85df-cbdd-4bb6-b04e-613caece083c} DownloadLegacy.js
> +
> +# This is disabled since we also build "toolkit/components/downloads"

a better comment about what happens and what will happen in future would be more than welcome here. Even with bug numbers.

Comment 4

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

Comment on attachment 726736 [details] [diff] [review]
Updated patch

Oops, sorry for the delay here.

Comment 5

[:Paolo Amadini]

Attachment: Fix tests on Windows

Comment 6

[:Paolo Amadini]

Attachment: More robust tests on Windows

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 733241 [details] [diff] [review]
More robust tests on Windows

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

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +107,4 @@
>  {
> +  // Prepend a serial number to the first dot in the suggested leaf name.
> +  let leafName = aLeafNameWithExtension.replace(".", "-" + gFileCounter + ".");
> +  gFileCounter++;

I wonder if you may use DownloadPaths.jsm utils here

::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
@@ +34,5 @@
>    let persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>                    .createInstance(Ci.nsIWebBrowserPersist);
> +
> +  // We must create the nsITransfer implementation using its class ID until we
> +  // always implement the "@mozilla.org/transfer;1" contract.

add bug# where that will happen

Comment 8

[:Paolo Amadini]

Attachment: Address review and fix other intermittent failures

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/833dc6a7c475

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/833dc6a7c475