Errors aren't passed sanely from getInstallForFile

VERIFIED FIXED in mozilla2.0b1

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [AddonsRewrite])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Currently if getInstallForFile encounters an error it throws an exception rather than calling the callback. This kind of doesn't make sense I think and it would be better to instead pass the callback the AddonInstall instance in the DOWNLOAD_FAILED state, exactly matching what you'd get from onDownloadFailed for the same XPI if you used getInstallForURL.

As part of this I'm also going to fix bug 553023 by exposing the error since it will not be exposed any other way for this case and I'm also going to make the error parameter on onInstallFailed a simple error code rather than an exception which you can't compare to anything.
(Assignee)

Updated

8 years ago
Whiteboard: [AddonsRewrite]
(Assignee)

Updated

8 years ago
Blocks: 569194
(Assignee)

Comment 1

8 years ago
Created attachment 449364 [details] [diff] [review]
patch rev 1

Adds error handling and tests a bunch of cases. This fixes bug 553023, bug 562303, bug 567173 and bug 569194.
Attachment #449364 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 2

8 years ago
Created attachment 450163 [details] [diff] [review]
patch rev 2

Updated from our discussion yesterday. I don't think it makes sense to call onNewInstall for AddonInstalls with errors so the only events that get installs in an error state are onDownloadFailed and onInstallFailed. It is perhaps a little redundant that we pass the error code as an argument to those while it is already available on the install object itself but I think for those states that code is more important so it makes some sense to include it. The alternative is removing it which would cause a certain amount of churn, probably doable at this stage but I'd still rather avoid it.
Attachment #449364 - Attachment is obsolete: true
Attachment #450163 - Flags: review?(robert.bugzilla)
Attachment #449364 - Flags: review?(robert.bugzilla)
Comment on attachment 450163 [details] [diff] [review]
patch rev 2

I would prefer consistency between them but understand wanting to avoid churn. How about a followup bug to make them consistent at some point?
Attachment #450163 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> (From update of attachment 450163 [details] [diff] [review])
> I would prefer consistency between them but understand wanting to avoid churn.
> How about a followup bug to make them consistent at some point?

If you think it is necessary to do at some point then we should just do it now to avoid changing the API on people later. I'll update the patch to do that.
(Assignee)

Comment 5

8 years ago
Created attachment 450419 [details] [diff] [review]
patch rev 3

Updated to remove those arguments.
Attachment #450163 - Attachment is obsolete: true
Attachment #450419 - Flags: review?(robert.bugzilla)
Comment on attachment 450419 [details] [diff] [review]
patch rev 3

Looks great
Attachment #450419 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 7

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/0d404b63f71b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Target Milestone: mozilla1.9.3a5 → mozilla1.9.3
Marking as verified fixed based on check-in and a green tinderbox in our area.
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.9.3 → mozilla1.9.3a6
You need to log in before you can comment on or make changes to this bug.