This defines an initial, minimal skeleton of the asynchronous JavaScript API for downloads, upon which various features can be added in parallel.

This is the initial skeleton of the JavaScript API for downloads, for feedback. While it does not do anything particularly useful, it shows the proposed module structure and defines the infrastructure for regressions tests. There are already two working regression tests, showing a simple call for a direct download (but the code is not the final one, eventually we'll use the background file saver component). Since this is a back-end module, it should be possible to unit-test it using only xpcshell tests. I expect that it will be very simple to add features one by one, with their tests, over this initial skeleton. This way, reviews should also be easy. The proposed working strategy is to land this in mozilla-central and work there, so that people can contribute more easily. A branch is not needed because we're not touching other areas of the code. We're always free to change the API or module structure if we realize that something should be done differently. I've implemented a build flag, so that the component is only built when the line "export MOZ_JSDOWNLOADS=1" is specified in .mozconfig. This may even not be needed, however it makes sure the API is not used while it's in the early experimental stages. The downside is that we won't get regression tests on Nightly builds, so we'll be responsible for adapting the code in case some other work breaks it, but this could be appropriate at this stage.

+ yield Downloads.createDownload({ + source: { uri: NetUtil.newURI("about:blank") }, + target: { file: getTempFile(TEST_TARGET_FILE_NAME) }, + saver: { type: "copy" }, + }); Why is an object of objects passed to createDownload, instead of a single object with three properties?

(In reply to Neil Deakin from comment #2) > Why is an object of objects passed to createDownload, instead of a single > object with three properties? I expect "source" and "target" could be objects returned by other functions, with a structure similar to a DownloadSource or DownloadTarget object. You may not know in advance whether additional properties of the object are populated. For example, in the future, there could be a file picker function that is able to return a DownloadTarget object populated with an URI instead of a target file, to support downloads to GIO network locations similarly to what's being done in bug 682838. Grouping could also be clearer than flat properties when the objects are complex, like "source: { document: ..., uri: ..., postData: ... }" rather than "sourceDocument: ..., sourceUri: ..., sourcePostData: ...". "saver" is polymorphic and depends on the saver type, it may require additional properties that are passed to the specific DownloadSaver constructor based on the saver type. For simple downloads (like saving an URI to a local file) I think we should create a different function, like Downloads.simpleDownload(aSourceURIorURISpec, aTargetFileOrFilePath, aOptions).then(onSuccess, onFailure), following the model of OS.File.copy. Does it make sense, or do you think we should start flat and adapt as needed?

(In reply to Paolo Amadini [:paolo] from comment #3) > For simple downloads (like saving an URI to a local file) I think we should > create a different function, like > Downloads.simpleDownload(aSourceURIorURISpec, > aTargetFileOrFilePath, aOptions).then(onSuccess, onFailure), following the > model of OS.File.copy. This is what I was thinking. aOptions should also be optional.

In this version, I also implemented a simpleDownload function, along the lines of what was discussed here, though I didn't add the aOptions parameter yet because it would be unused. I also fixed a couple of undetected typos. This module is still excluded from the build by default as per comment 1, it requires its mozconfig option to be set in order to be included. This draft module structure should allow us to work on adding basic features, but can later be changed, extended or corrected at any time.

Comment on attachment 703260 [details] [diff] [review] Fixed and updated with simpleDownload + start: function D_start() + { + this._deferFinish.resolve(Task.spawn(function task_D_start() { + yield this.saver.execute(); + }.bind(this))); + + return this.finish(); + }, I'm not sure I understand what _deferFinish is for, but I assume it works. + +/** + * Provides functions to integrate with the host application, handling for + * example the global prompts on shutdown. + */ +this.DownloadIntegration = { +}; You write this (and DownloadUIHelper) differently than DownloadList and DownloadStore where you write: function DownloadList() { } DownloadList.prototype = { }; Also, you duplicate the comment in each of these files, both here and near the top. + createDownload: function D_createDownload(aProperties) + { + return Task.spawn(function task_D_createDownload() { + let download = new Download(); + + download.source = new DownloadSource(); + download.source.uri = aProperties.source.uri; + download.target = new DownloadTarget(); + download.target.file = aProperties.target.file; + download.saver = new DownloadCopySaver(); + download.saver.download = download; + This doesn't use aProperties.saver. If this is because it isn't implemented yet, I'd add a comment saying this. +add_task(function test_simpleDownload_object_arguments() +{ + let targetFile = getTempFile(TEST_TARGET_FILE_NAME); + yield Downloads.simpleDownload({ uri: TEST_SOURCE_URI }, + { file: targetFile }); + yield promiseVerifyContents(targetFile, TEST_DATA_SHORT); + targetFile.remove(false); getTempFile marks the file for removal automatically, so this last remove() isn't needed right?

Comment on attachment 703260 [details] [diff] [review] Fixed and updated with simpleDownload Review of attachment 703260 [details] [diff] [review]: ----------------------------------------------------------------- So, it's really hard to evaluate this in its current status, I suspect final shape may differ quite a bit when this hits actual use-cases... It surely looks well thought, but that also means it brings with it some complication than in the end we may found not being totally needed for our use-cases. Though, since this is a prototype and it's disabled, I think it's ok to proceed with it and evaluate issues when they come. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +65,5 @@ > +{ > + this._deferFinish = Promise.defer(); > +} > + > +Download.prototype = { I suggest to Object.seal the prototypes @@ +96,5 @@ > + * @rejects JavaScript exception if the download failed. > + */ > + start: function D_start() > + { > + this._deferFinish.resolve(Task.spawn(function task_D_start() { I suspect this oneliner would be more readable as a task spawn that resolves the promise internally... @@ +97,5 @@ > + */ > + start: function D_start() > + { > + this._deferFinish.resolve(Task.spawn(function task_D_start() { > + yield this.saver.execute(); is start() expected to do any check on _deferFinish and saver validity? Or I suppose being just a prototype implementation error checking will come later... Indeed I don't see much error management around. @@ +110,5 @@ > + * @return {Promise} > + * @resolves When the download has finished successfully. > + * @rejects JavaScript exception if the download failed. > + */ > + finish: function D_finish() { it's confusing to read Download.finish().then(), since finish() sounds line an action, not just an handler. We have start() that effectively executes the saver, and finish() that sounds like would cancel or stop the action, but instead is just an handler, like onFinish, promiseFinish... ::: toolkit/components/jsdownloads/test/unit/tail.js @@ +18,5 @@ > + // xpcshell testing framework supports asynchronous termination functions. > + let deferred = Promise.defer(); > + gHttpServer.stop(deferred.resolve); > + yield deferred.promise; > +}); I think alternatively you may just do_register_cleanup(function() gHttpServer.stop())) in the head and avoid a tail file.

(In reply to Neil Deakin from comment #6) > You write this (and DownloadUIHelper) differently than DownloadList and > DownloadStore where you write: > > function DownloadList() { } > > DownloadList.prototype = { > }; Some objects may have multiple instances, while others are singletons. > Also, you duplicate the comment in each of these files, both here and near > the top. The first comment is the "file comment", it appears in MXR when you browse the folder, but must be the first comment in the file. See for example: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/ The second is the "object comment", I expect it to appear in code completion if you have a development environment that supports that for JavaScript. I've usually seen it placed near the object's constructor function, so I assume it should be there in order for code completion to show it. Given the two conflicting requirements, I couldn't find other solutions than duplicating the comments. > getTempFile marks the file for removal automatically, so this last remove() > isn't needed right? This was done for robustness between individual tests, I've now made it so that getTempFile ensures that the file doesn't exist, that should be equivalent. Thanks! (In reply to Marco Bonardo [:mak] from comment #7) > > + this._deferFinish.resolve(Task.spawn(function task_D_start() { > > I suspect this oneliner would be more readable as a task spawn that resolves > the promise internally... So, we discussed on IRC and we're going with two types of notifications: * whenDone() returns a promise that is resolved if the download succeeds without interruptions, and is rejected if the download fails or is canceled during this attempt. * whenSucceeded() returns a promise that is resolved when the download succeeds, even if it fails and then is retried from the user interface. It is never rejected. So, _deferDone will be resolved with the task's result, meaning that it will be rejected if an exception occurs. _deferSucceeded will be resolved as the last line of the task, if it is ever reached. > Or I suppose being just a prototype implementation error checking will come > later... Indeed I don't see much error management around. As discussed, checking for misuse will come later if and when needed. (We're checking normal errors and handling exceptions, of course.) > it's confusing to read Download.finish().then(), since finish() sounds line > an action, not just an handler. We'll be generally using these conventions: * Nouns, or past verbs, for status properties: progress, done, succeeded * Present verbs for actions: start(), open() * "When" + past verbs for methods without side-effects: whenDone() * "On" + present verb (lowercase, DOM style) for settable callbacks: onchange

How can I test already committed patches? I don't see the changes in mozilla-central (for example changes in configure.in).

(In reply to jhorak from comment #12) > How can I test already committed patches? I don't see the changes in > mozilla-central (for example changes in configure.in). Some of these have been modified since the feature was first implemented. If you are interested in which parts may be tested in Nightly, you can take a look at the current state of the files, or at the dependencies of bug 825588.