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)
Firefox Graveyard
Web Apps
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 33
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 4 obsolete files)
6.84 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
We should wait the local installation to finish before sending events to content.
Attachment #791382 -
Flags: feedback?(fabrice)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #791382 -
Attachment is obsolete: true
Attachment #791382 -
Flags: feedback?(fabrice)
Attachment #795660 -
Flags: review?(fabrice)
Assignee | ||
Comment 2•11 years ago
|
||
Try run here: https://tbpl.mozilla.org/?tree=Try&rev=76dcb664bcef
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #796916 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #796916 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #796916 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Fixed a syntax error.
Attachment #8451619 -
Attachment is obsolete: true
Attachment #8451619 -
Flags: review?(myk)
Attachment #8451656 -
Flags: review?(myk)
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Try run was green: https://tbpl.mozilla.org/?tree=Try&rev=9d64176cc055 https://hg.mozilla.org/integration/mozilla-inbound/rev/c43489001722 I've filed bug 1037247, as we agreed.
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c43489001722
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•