835875 - Add the ability to cancel downloads

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

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

Comment 1

[:Paolo Amadini]

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.

Comment 2

[:Paolo Amadini]

Attachment: The patch

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.

Comment 3

[:Paolo Amadini]

Attachment: Minor changes

Refactored some test code to be more concise.

Comment 4

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cdf7f47b0

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b8cdf7f47b0

Comment 7

[:Paolo Amadini]

(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).

Comment 8

[Marco Bonardo [:mak]]

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