Closed
Bug 996669
Opened 9 years ago
Closed 9 years ago
onuninstall listener can be called after panel is unregistered
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(1 file)
1.43 KB,
patch
|
jdover
:
review+
jdover
:
feedback+
|
Details | Diff | Splinter Review |
There's not much we can do if the add-on author decides to unregister and uninstall their panel at the same time, but we should avoid hitting an exception in Home.jsm in that case.
Assignee | ||
Comment 1•9 years ago
|
||
We can't really control whether an add-on decides to unregister a panel before it's onuninstall listener gets called, but we can at least offer a more helpful error message. We should never run into this situation with the oninstall listener, but in case we do for some reason, it's also good to have a more helpful exception. jdover, I think you have enough experience to review this patch :)
Attachment #8407840 -
Flags: review?(jdover)
Comment 2•9 years ago
|
||
Comment on attachment 8407840 [details] [diff] [review] Assert panel exists in oninstall/onuninstall listeners Review of attachment 8407840 [details] [diff] [review]: ----------------------------------------------------------------- I think we can make this a bit safer by making a getter that does this check for you every time. ::: mobile/android/modules/Home.jsm @@ +301,1 @@ > let options = _registeredPanels[id](); What if we repurposed _assertPanelExists to also call optionsCallback and return the resulting object? This way every time we call for the options we don't have to remember that it's a function and we get the safe checking of the assert every time. So something like: let options = _getExistingPanel(id);
Attachment #8407840 -
Flags: review?(jdover) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
I started to implement this suggestion, but I decided I think it's an unnecessary level of complexity. In multiple places where we call _assertPanelExists we don't actually generate the panel, so if we're not going to get rid of _assertPanelExists, I don't want to introduce another small helper method. Also, but explicitly putting in an assert at the beginning of these methods, it's more obvious to developers that these methods might throw.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8407840 [details] [diff] [review] Assert panel exists in oninstall/onuninstall listeners Re-requesting review based on my pushback in my last comment :)
Attachment #8407840 -
Flags: review?(jdover)
Comment 5•9 years ago
|
||
Comment on attachment 8407840 [details] [diff] [review] Assert panel exists in oninstall/onuninstall listeners Review of attachment 8407840 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Margaret Leibovic from comment #3) > I started to implement this suggestion, but I decided I think it's an > unnecessary level of complexity. In multiple places where we call > _assertPanelExists we don't actually generate the panel, so if we're not > going to get rid of _assertPanelExists, I don't want to introduce another > small helper method. Also, but explicitly putting in an assert at the > beginning of these methods, it's more obvious to developers that these > methods might throw. Ah yes I see. LGTM
Attachment #8407840 -
Flags: review?(jdover) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/52cb8e937d12
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/52cb8e937d12
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•