Integration with legacy interfaces to start new downloads

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

31.38 KB, patch
Gavin
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
The jsdownloads API should handle downloads started using legacy components,
and then added to the global list of downloads using nsITransfer.

This type of downloads will not have access to all the features of the new API,
for example cancellation and temporary file deletion will always be executed
through the synchronous nsICancelable interface.
(Assignee)

Comment 1

6 years ago
Created attachment 723206 [details] [diff] [review]
The patch

This is a milestone in the JavaScript API for downloads, since it contains the
logic that allows synchronous creation and handling of nsITransfer instances,
while deferring the real notifications after the download system has fully
initialized, that can be an asynchronous operation in the new API.

Restarting legacy downloads is not handled yet. Legacy single-file downloads
will need to go through DownloadCopySaver when restarted.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #723206 - Flags: review?(enndeakin)
Attachment #723206 - Flags: feedback?(mak77)

Comment 2

6 years ago
Comment on attachment 723206 [details] [diff] [review]
The patch

Looks good, although I'm not as familiar with the old interface. You might want to clarify in the document if this is intended for compatibility only and that new code should not use this component.
Attachment #723206 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 3

6 years ago
(In reply to Neil Deakin from comment #2)
> Looks good, although I'm not as familiar with the old interface. You might
> want to clarify in the document if this is intended for compatibility only
> and that new code should not use this component.

Yeah, when the API is ready we should deprecate the nsITransfer interface and
update the code snippets for downloads on MDN.
Comment on attachment 723206 [details] [diff] [review]
The patch

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

::: browser/installer/package-manifest.in
@@ +348,5 @@
>  @BINPATH@/browser/components/BrowserPlaces.manifest
> +#ifdef MOZ_JSDOWNLOADS
> +@BINPATH@/browser/components/Downloads.manifest
> +@BINPATH@/browser/components/DownloadLegacy.js
> +#endif

how do these end up under browser/?

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +727,5 @@
> +
> +  /**
> +   * Whether the notifyProgressBytes function has been called at least once.
> +   */
> +  onProgressBytesCalled: false,

this naming is confusing, in the sense starting by "on" I expect it to be sort of a listener function, not a bool, what about progressBytesReceived, or downloadProgressed, or just inProgress?

@@ +737,5 @@
> +   *        nsIRequest associated to the status update.
> +   * @param aStatus
> +   *        Status code received by the nsITransfer implementation.
> +   */
> +  onTransferFinished: function DLS_onTransferFinished(aRequest, aStatus)

you see, this is clearly not a bool

@@ +762,5 @@
> +    return Task.spawn(function task_DLS_execute() {
> +      try {
> +        // Wait for the component that executes the download to finish.
> +        yield this.deferExecuted.promise;
> +  

trailing spaces

@@ +771,5 @@
> +            this.request instanceof Ci.nsIChannel &&
> +            this.request.contentLength >= 0) {
> +          aSetProgressBytesFn(0, this.request.contentLength);
> +        }
> +  

trailing spaces

::: toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * This component implements the XPCOM interfaces required for proper
> + * integration with the legacy download components.

expanding the concept of "proper integration" would be nice, basically the purpose (I think this is about the same Neil asked for)

@@ +67,5 @@
> +
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// nsISupports
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsITransfer]),

I couldn't find specific documentation, though I think you may have to also add nsIWebProgressListener and nsIWebProgressListener2 here. My supposition comes from the fact any interface inherits from nsISupports and we specify nsISupports in QI (well, xpcomutils does that for you), if would be enough to just specify the final interface we should not even need to specify nsISupports.
If you find documentation for this I'd love to read it though
Attachment #723206 - Flags: feedback?(mak77) → feedback+
(Assignee)

Comment 5

6 years ago
Created attachment 725386 [details] [diff] [review]
Updated patch

(In reply to Marco Bonardo [:mak] from comment #4)
> > +@BINPATH@/browser/components/Downloads.manifest
> > +@BINPATH@/browser/components/DownloadLegacy.js
> > +#endif
> 
> how do these end up under browser/?

They don't, of course. Now verified with "make package".

> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsITransfer]),
> 
> I couldn't find specific documentation, though I think you may have to also
> add nsIWebProgressListener and nsIWebProgressListener2 here.

That's true, all the interfaces need to be specified explicitly (also when
implementing C++ components), this is how XPCOM works. Maybe I could add a
note to that effect to the generateQI docs on MDN, what do you think?
Attachment #723206 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Comment on attachment 725386 [details] [diff] [review]
Updated patch

Gavin, this patch implements integration between the old and the new API.
Attachment #725386 - Flags: feedback?(gavin.sharp)
Comment on attachment 725386 [details] [diff] [review]
Updated patch

cool!

meta-nit: we should probably avoid use of __proto__ in new code in favor of Object.create etc.
Attachment #725386 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0589a4207230
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/0589a4207230
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.