The JavaScript API for downloads will be easier to test, by add-on developers and by ourselves, if we introduce a preference controlling whether the Downloads Panel uses it instead of the old nsIDownloadManager service. This would allow us to switch between the two versions while we test our code when resolving possible regressions. The preference will be removed as soon as we have stabilized the API, probably in the same release cycle.

This depends on bug 852482 for the "launch" method, and bug 836437 for resuming.

Comment on attachment 782603 [details] [diff] [review] The patch Review of attachment 782603 [details] [diff] [review]: ----------------------------------------------------------------- As a first porting it looks good, I expect there will still be something to fix, but that can be better figured by using it. ::: browser/app/profile/firefox.js @@ +330,5 @@ > pref("browser.download.manager.scanWhenDone", true); > pref("browser.download.manager.resumeOnWakeDelay", 10000); > > +// This connects the Downloads Panel to the JavaScript API for downloads. > +pref("browser.download.useJSTransfer", false); I'd make it a bit more generic, and avoid "Connect" that sounds unclear. Maybe // Enables the asynchronous downloads API in the download manager. pref("browser.download.useAsyncDownloads", false); If this is ready for use, we may ifdef the default based on the channel, enabling it in Nightly and Aurora with an #ifndef RELEASE_BUILD ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +637,5 @@ > this._views = []; > + > + if (DownloadsCommon.useJSTransfer) { > + this._downloadToDataItemMap = new WeakMap(); > + } needs a brief comment explaining its use and contents, like the other stores. // Maps Download(s) to DataItem(s). Is there a specific reason to use a WeakMap instead of going for a Map, keeping it up-to-date and reportError eventual misuses? we lose some control with the weakmap since we can't tell when objects are GCed. @@ +682,5 @@ > Services.downloads.removeListener(this); > }, > > ////////////////////////////////////////////////////////////////////////////// > + //// Integration with the Downloads back-end "asynchronous Downloads back-end", just to distinguish from the other Downloads backend :) @@ +692,5 @@ > + this.dataItems[dataItem.downloadGuid] = dataItem; > + > + this._views.forEach( > + function (view) view.onDataItemAdded(dataItem, true) > + ); nit: arrow function please, or for..of @@ +694,5 @@ > + this._views.forEach( > + function (view) view.onDataItemAdded(dataItem, true) > + ); > + > + this.onDownloadChanged(aDownload); as it is it looks just like a forward to another handler, like if the backend could just do that (as, call added and then call changed). I suggest rather making both listeners invoke a common private method (like _updateDownloadState or whatever) that gets dataItem. It would be a bit cleaner and would save a key lookup in the map. @@ +701,5 @@ > + onDownloadChanged: function DD_onDownloadChanged(aDownload) > + { > + let dataItem = this._downloadToDataItemMap.get(aDownload); > + if (!dataItem) { > + Cu.reportError("Download doens't exist."); typo: doens @@ +733,5 @@ > + } > + > + this._views.forEach( > + function (view) view.getViewItem(dataItem).onProgressChange() > + ); ditto @@ +740,5 @@ > + onDownloadRemoved: function DD_onDownloadRemoved(aDownload) > + { > + let dataItem = this._downloadToDataItemMap.get(aDownload); > + if (!dataItem) { > + Cu.reportError("Download doens't exist."); copy/paste typo ;) @@ +747,5 @@ > + > + this.dataItems[dataItem.downloadGuid] = null; > + this._views.forEach( > + function (view) view.onDataItemRemoved(dataItem) > + ); ditto @@ +1268,4 @@ > */ > function DownloadsDataItem(aSource) > { > + if (DownloadsCommon.useJSTransfer) { why not checking instanceof/typeof instead? @@ +1281,5 @@ > /** > + * The JavaScript API does not need identifiers for Download objects, so they > + * are generated sequentially for the corresponding DownloadDataItem. > + */ > + _nextIdentifier: 1, I think you rather want an auto-incrementing getter @@ +1295,5 @@ > + _initFromJSDownload: function (aDownload) > + { > + this._download = aDownload; > + > + this.downloadGuid = "id:" + this._nextIdentifier++; I assume there's no interest in syncing downloads through network (Sync/PiCL)? @@ +1320,5 @@ > + (this._download.hasPartialData > + ? nsIDM.DOWNLOAD_PAUSED > + : nsIDM.DOWNLOAD_CANCELED) : > + this._download.stopped ? nsIDM.DOWNLOAD_NOTSTARTED : > + nsIDM.DOWNLOAD_DOWNLOADING; this is not extremely readable :( I find easier to follow a common if/else in these cases if (this._download.succeeded) this.state = return nsIDM.DOWNLOAD_FINISHED; else if (this._download.error && this._download.error.becauseBlockedByParentalControls) this.state = nsIDM.DOWNLOAD_BLOCKED_PARENTAL; else if (this._download.error) this.state = nsIDM.DOWNLOAD_FAILED); else if (this._download.canceled && this._download.hasPartialData) this.state = nsIDM.DOWNLOAD_PAUSED; else if (this._download.canceled) this.state = nsIDM.DOWNLOAD_CANCELED; else if (this._download.stopped) this.state = nsIDM.DOWNLOAD_NOTSTARTED; else this.state = nsIDM.DOWNLOAD_DOWNLOADING you could even move it to a local helper and return a state. @@ +1603,2 @@ > if (!this.inProgress || !this.resumable) > throw new Error("The given download cannot be paused or resumed"); are you skipping these checks on purpose? @@ +1626,2 @@ > if (!this.canRetry) > throw new Error("Cannot rerty this download"); ditto @@ +1660,2 @@ > if (!this.inProgress) > throw new Error("Cannot cancel this download"); ditto @@ +1672,5 @@ > remove: function DDI_remove() { > + if (DownloadsCommon.useJSTransfer) { > + let promiseList = this._download.source.isPrivate > + ? Downloads.getPrivateDownloadList() > + : Downloads.getPublicDownloadList(); nit: indent options 2 more spaces please ::: browser/components/downloads/src/DownloadsStartup.js @@ +110,5 @@ > .registerFactory(kTransferCid, "", > kTransferContractId, null); > + } else { > + // The other notifications are handled internally by the JavaScript > + // API for downloads, no need to observe when the API is enabled. s/the/that/ ::: toolkit/components/jsdownloads/src/DownloadList.jsm @@ +91,5 @@ > > for (let view of this._views) { > try { > if (view.onDownloadAdded) { > + view.onDownloadAdded.call(view, aDownload); why these scope changes? doesn't look like something changed in the view, just for correctness?

(In reply to Marco Bonardo [:mak] from comment #2) > I'd make it a bit more generic, and avoid "Connect" that sounds unclear. > Maybe > > // Enables the asynchronous downloads API in the download manager. > pref("browser.download.useAsyncDownloads", false); Updated the comment, though this preference is so short-lived (will be removed in Firefox 26 together with all the old code) that I think it's not worth changing its name. It's easier for us, and by keeping the same name there will be no confusion in case someone comes across an old post that mentions it. > If this is ready for use, we may ifdef the default based on the channel, > enabling it in Nightly and Aurora with an > #ifndef RELEASE_BUILD The feature is not ready for use by all Nightly and Aurora users, it should be enabled only by add-on developers who want to test it with their add-ons. > Is there a specific reason to use a WeakMap instead of going for a Map, > keeping it up-to-date and reportError eventual misuses? we lose some control > with the weakmap since we can't tell when objects are GCed. I think it's pretty much the same, since we receive removal notifications. The only advantage of WeakMap is that we don't leak if something goes wrong in the code path that should remove the element. I'm not familiar with the GC issue you mentioned, but let me know what you think is better in the light of the potential leak issue also. > @@ +1268,4 @@ > > */ > > function DownloadsDataItem(aSource) > > { > > + if (DownloadsCommon.useJSTransfer) { > > why not checking instanceof/typeof instead? We don't have access to the Download object's prototype. > I think you rather want an auto-incrementing getter How does an auto-incrementing getter look like? > @@ +1295,5 @@ > > + _initFromJSDownload: function (aDownload) > > + { > > + this._download = aDownload; > > + > > + this.downloadGuid = "id:" + this._nextIdentifier++; > > I assume there's no interest in syncing downloads through network > (Sync/PiCL)? I assume so, yes. In case a need appears, it may be better addressed later by an extensibility mechanism, like serializing generic externally set properties, of which one could be the GUID. > @@ +1603,2 @@ > > if (!this.inProgress || !this.resumable) > > throw new Error("The given download cannot be paused or resumed"); > > are you skipping these checks on purpose? Yes, because the race condition / multiple click case is now handled correctly by the back-end, and we don't want console errors in that case. > ::: toolkit/components/jsdownloads/src/DownloadList.jsm > > if (view.onDownloadAdded) { > > + view.onDownloadAdded.call(view, aDownload); > > why these scope changes? doesn't look like something changed in the view, > just for correctness? The callbacks on DownloadsData were called with "this" set to undefined, so they didn't work. I'll add a regression test to test_DownloadList.js also.

(In reply to Paolo Amadini [:paolo] from comment #3) > > Is there a specific reason to use a WeakMap instead of going for a Map, > > keeping it up-to-date and reportError eventual misuses? we lose some control > > with the weakmap since we can't tell when objects are GCed. > > I think it's pretty much the same, since we receive removal notifications. > The > only advantage of WeakMap is that we don't leak if something goes wrong in > the > code path that should remove the element. well, my point is that if something goes wrong we should somehow know. Probably leaking is not the best way to achieve that, though trying to use a removed entry would be signaled with an error if we manually remove things, while with the weakmap the entry could still exist because it has not yet been GCed. Or we could expect an entry to not exist anymore while it's still there for the same reason. WeakMaps are mostly a nice thing for connectiong objects to DOM elements without the risk of leaking those elements (and their globals), imo. > > > I think you rather want an auto-incrementing getter > > How does an auto-incrementing getter look like? trivially, I may think of __lastId: 0, get _autoIncrementId() this.__lastId++; // or ++__lastId to start from 1

Changed to Map with explicit removal, and addressed all the other comments. Thanks for the quick review! Actually, the "this" issue I observed does not appear anymore, I might have misinterpreted some results, or it could have been something transient that happened in a previous build. I've left the regression test just in case.