Closed Bug 835872 Opened 7 years ago Closed 7 years ago

Handle download errors

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, errors should be reported correctly, and it should
be possible to distinguish between errors from the network or errors while
writing the target file.
If the download fails, a failureReason property would give access to an instance
of a DownloadError object containing the details of the failure, for example:
* DownloadError.becauseSourceFailed
* DownloadError.becauseTargetFailed
* DownloadError.becauseDiskFull
* DownloadError.result
Attached patch The patch (obsolete) — Splinter Review
This implements the DownloadError object, with the becauseSourceFailed and
becauseTargetFailed properties, with the model of OS.File.Error. I didn't
implement becauseDiskFull yet since I don't know how to write a sane
automated test for it, but we can add this later together with other
properties if we deem those handy.

In this patch, I haven't yet renamed the status properties as previously
discussed, will do that while implementing cancel and resume.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #712951 - Flags: review?(enndeakin)
Attachment #712951 - Flags: feedback?(mak77)
Comment on attachment 712951 [details] [diff] [review]
The patch

Review of attachment 712951 [details] [diff] [review]:
-----------------------------------------------------------------

off-topic nit: I dislike the because naming, I would have preferred reason. But I see OS.File is driving already here :(

btw, here is my feedback

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +102,5 @@
> +   * When the download fails, this is set to a DownloadError instance indicating
> +   * the cause of the failure.  If the download has been completed successfully
> +   * or has been canceled, this property is null.
> +   */
> +  failureReason: null,

exactly cause failureReason naming doesn't make sense if there was no failure, I suggest something more like lastError... in case of success lastError == null makes more sense than failureReason == null

@@ +291,5 @@
> + *        by a component that handles both aspects of the download.
> + *
> + * @returns The newly created DownloadError object.
> + */
> +DownloadError._fromResult = function DE_createFromResult(aResult, aInferCause)

it's a bit hard to figure if this is the right approach since there is only one caller that always passes true as second argument...

one thing I noticed is that "fromResult" is a bit too generic naming, I had no idea what a "result" was in this case. maybe .createFromResultValue could be better...

Though I'm not sure why aInferCause couldn't just be an optional param of the DownloadError constructor?
Attachment #712951 - Flags: feedback?(mak77) → feedback+
> +function DownloadError(aMessage, aResult)
> +{
> +  this.name = "DownloadError";

What is the name used for?

>+  let downloadError = new DownloadError(exception, exception.result);

You pass an exception here when calling the constructor but the constructor expects a string message.

Can you document the properties that are set by the constructor if they are meant to be public?

+    do_check_true(download.failureReason.becauseSourceFailed);

I'd also test that the becauseTargetFailed property is false. Same with the other test.
(In reply to Marco Bonardo [:mak] (Away 14-17 Feb) from comment #3)
> Though I'm not sure why aInferCause couldn't just be an optional param of
> the DownloadError constructor?

So, it's probably too early to draw a distinction between what's public and
what is semi-public for the DownloadError object, probably the only objects
that need to construct DownloadError objects are DownloadSaver implementations.
I've filed bug 825588 to figure out an interface when needed, for now I'll move
everything to the constructor, that is used only internally.
That was bug 841348.
Attached patch Updated patchSplinter Review
Attachment #712951 - Attachment is obsolete: true
Attachment #712951 - Flags: review?(enndeakin)
Attachment #713895 - Flags: review?(enndeakin)
> +function DownloadError(aMessage, aResult)
> +{
> +  this.name = "DownloadError";
>
> What is the name used for?


>Can you document the properties that are set by the constructor if they are meant to
> be public?

I didn't see these two comments addressed.
(In reply to Neil Deakin from comment #8)
> I didn't see these two comments addressed.

I thought I addressed them by documenting the "result" property and adding the
comment saying that "name" is used by the Error object prototype. I didn't
document again the six Error prototype properties that are mentioned here:

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Error/prototype

Did you have something else in mind?
Attachment #713895 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a57be02f88
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/a7a57be02f88
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.