Closed Bug 835875 Opened 7 years ago Closed 7 years ago

Add the ability to cancel downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch The patch (obsolete) — Splinter Review
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #713926 - Flags: review?(enndeakin)
Attachment #713926 - Flags: feedback?(mak77)
Attached patch Minor changesSplinter Review
Refactored some test code to be more concise.
Attachment #713926 - Attachment is obsolete: true
Attachment #713926 - Flags: review?(enndeakin)
Attachment #713926 - Flags: feedback?(mak77)
Attachment #714898 - Flags: review?(enndeakin)
Attachment #714898 - Flags: feedback?(mak77)
Attachment #714898 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cdf7f47b0
Target Milestone: --- → mozilla21
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.
Attachment #714898 - Flags: feedback?(mak77) → feedback+
https://hg.mozilla.org/mozilla-central/rev/9b8cdf7f47b0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.