Closed Bug 906114 Opened 11 years ago Closed 10 years ago

Send installed messages to content process when the local installation is successful

Categories

(Firefox Graveyard :: Web Apps, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
We should wait the local installation to finish before sending events to content.
Attachment #791382 - Flags: feedback?(fabrice)
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch Patch (obsolete) — Splinter Review
Attachment #791382 - Attachment is obsolete: true
Attachment #791382 - Flags: feedback?(fabrice)
Attachment #795660 - Flags: review?(fabrice)
Comment on attachment 795660 [details] [diff] [review]
Patch

Needs to be rebased, I'm considering a small refactoring.
Attachment #795660 - Attachment is obsolete: true
Attachment #795660 - Flags: review?(fabrice)
Attached patch Patch (obsolete) — Splinter Review
Attachment #796916 - Flags: review?(fabrice)
Attachment #796916 - Flags: review?(fabrice)
Depends on: 910473
Attachment #796916 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
I need this for a test for bug 997848.
I haven't changed the behavior in case of exceptions during the "native" installation (that is, ignoring them). We could change the behavior in another bug.
Attachment #8451619 - Flags: review?(myk)
Attached patch PatchSplinter Review
Fixed a syntax error.
Attachment #8451619 - Attachment is obsolete: true
Attachment #8451619 - Flags: review?(myk)
Attachment #8451656 - Flags: review?(myk)
Comment on attachment 8451656 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #5)
> Created attachment 8451619 [details] [diff] [review]
> I haven't changed the behavior in case of exceptions during the "native"
> installation (that is, ignoring them). We could change the behavior in
> another bug.

Good idea!  But if native installation fails, then I would make NativeApp responsible for removing the app from the registry.  See below.

::: dom/apps/src/Webapps.jsm
@@ +2591,5 @@
> +          yield aInstallSuccessCallback(app, app.manifest);
> +        } catch (e) {
> +          // For now, ignore exceptions during the local installation of
> +          // an app. If it fails, the app will anyway be considered
> +          // as not installed because isLaunchable will return false.

I think we should actually do this in perpetuity, not only "for now," as it's the callback provider's responsibility to handle exceptions in the local install process.

And I don't think we should yield to the callback, even though in general I'm a fan of yielding to all asynchronous operations (to avoid subtle race conditions).

Ignoring the callback's behavior and return value, except to prevent an exception in it from propagating further, would be defensive in the case of an errant callback, whereas yielding gives the callback the ability to prevent the rest of the function from executing.

Which reminds me of some bugs I fixed in the past with callback-based APIs that accepted multiple callbacks, where an exception in one callback (f.e. registered by an addon) prevented the rest of them (f.e. from other addons or core chrome code) from running: the fix was to drop callback exceptions on the floor (or possibly to report them to the error console).
Attachment #8451656 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/c43489001722
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.