In the jsdownloads API, it should be possible to cancel in-progress downloads.

The "cancel" method allows for cancellation, and a "canceled" property on the download object indicates the canceled state. After cancel has been called, the download goes to the canceled state, regardless of whether there is an exception during the actual cancellation process.

I added the "cancel" method, and renamed "done" to "stopped" for clarity as previously discussed. I also removed the whenDone method, now the return value of "start" should be used for the same purpose. I'll add a new whenSucceeded method in bug 835876, that will be invoked even when a download is canceled and then restarted.

Refactored some test code to be more concise.

Comment on attachment 714898 [details] [diff] [review] Minor changes Review of attachment 714898 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +448,5 @@ > }, > }, null); > + > + // If the operation succeeded, store the object to allow cancellation. > + this._backgroundFileSaver = backgroundFileSaver; would not be better to null this once the operation is complete? @@ +464,5 @@ > + */ > + cancel: function DCS_cancel() > + { > + if (this._backgroundFileSaver) { > + this._backgroundFileSaver.finish(Cr.NS_ERROR_FAILURE); I'm not sure NS_ERROR_FAILURE is the best error to return for an operation I explicitly requested, NS_ERROR_ABORT could make more sense, also above in the Download object. ::: toolkit/components/jsdownloads/test/unit/head.js @@ +172,5 @@ > + deferResponse.promise.then(function SIRH_onSuccess() { > + aResponse.write(TEST_DATA_SHORT); > + aResponse.finish(); > + gHttpServer.registerPathHandler(TEST_INTERRUPTIBLE_PATH, null); > + }, function SIRH_onFailure() { nit: sometimes you use bracing functions on new line, sometimes not.

(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #5) > I'm not sure NS_ERROR_FAILURE is the best error to return for an operation I > explicitly requested, NS_ERROR_ABORT could make more sense, also above in > the Download object. I did this because one might then expect that NS_ERROR_ABORT indicates that the user cancelled the download, but saver implementations are free to throw NS_ERROR_ABORT spontaneously even if the download was not canceled. NS_ERROR_FAILURE is a generic error code, so this misunderstanding is less likely to occur. Does this make sense, or am I over-thinking this, or both? :-) > ::: toolkit/components/jsdownloads/test/unit/head.js > @@ +172,5 @@ > > + deferResponse.promise.then(function SIRH_onSuccess() { > > + aResponse.write(TEST_DATA_SHORT); > > + aResponse.finish(); > > + gHttpServer.registerPathHandler(TEST_INTERRUPTIBLE_PATH, null); > > + }, function SIRH_onFailure() { > > nit: sometimes you use bracing functions on new line, sometimes not. They're generally braced inline when they're used inline as arguments (with the understanding that there might be exceptions when it makes sense).

(In reply to Paolo Amadini [:paolo] from comment #7) > I did this because one might then expect that NS_ERROR_ABORT indicates that > the user cancelled the download, but saver implementations are free to throw > NS_ERROR_ABORT spontaneously even if the download was not canceled. > NS_ERROR_FAILURE is a generic error code, so this misunderstanding is > less likely to occur. > > Does this make sense, or am I over-thinking this, or both? :-) Well, it depends on which exceptions we pass to the user and which ones we catch and handle internally (rejecting a promise), I assume we have control over what reaches the user. Btw, I see your point now, if you fail in a generic way the consumer MUST go verifying the status properties to figure out what's up. Fine with that.