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.

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.

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.

(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

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

Comment on attachment 725386 [details] [diff] [review] Updated patch Gavin, this patch implements integration between the old and the new API.

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.