Closed Bug 851461 Opened 7 years ago Closed 7 years ago

Make the JavaScript API for downloads available in parallel to nsIDownloadManager

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

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.
Attached patch The patch (obsolete) — Splinter Review
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #726714 - Flags: superreview?(gavin.sharp)
Attachment #726714 - Flags: review?(mak77)
Attached patch Updated patch (obsolete) — Splinter Review
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
Attachment #726714 - Attachment is obsolete: true
Attachment #726714 - Flags: superreview?(gavin.sharp)
Attachment #726714 - Flags: review?(mak77)
Attachment #726736 - Flags: superreview?(gavin.sharp)
Attachment #726736 - Flags: review?(mak77)
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.
Attachment #726736 - Flags: review?(mak77)
Comment on attachment 726736 [details] [diff] [review]
Updated patch

Oops, sorry for the delay here.
Attachment #726736 - Flags: superreview?(gavin.sharp) → superreview+
Attached patch Fix tests on Windows (obsolete) — Splinter Review
Attachment #726736 - Attachment is obsolete: true
Attachment #733235 - Flags: review?(mak77)
Attached patch More robust tests on Windows (obsolete) — Splinter Review
Attachment #733235 - Attachment is obsolete: true
Attachment #733235 - Flags: review?(mak77)
Attachment #733241 - Flags: review?(mak77)
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
Attachment #733241 - Flags: review?(mak77) → review+
Attachment #733241 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/833dc6a7c475
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/833dc6a7c475
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.