Closed
Bug 959366
Opened 11 years ago
Closed 11 years ago
Unused LazyNotificationGetter.shutdown function
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: Margaret, Assigned: errietta)
References
Details
(Whiteboard: [lang=js][good first bug][mentor=mfinkle])
Attachments
(1 file, 3 obsolete files)
3.12 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
We should either call this function somewhere during shutdown, or we should just get rid of LazyNotificationGetter altogether.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#99
Assignee | ||
Comment 1•11 years ago
|
||
I could take this if you tell me which one it is you want done?
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 2•11 years ago
|
||
mfinkle, what do you think? I think we just get rid of LazyNotificationGetter, since we don't usually have an opportunity for a clean shutdown anyway.
Flags: needinfo?(margaret.leibovic) → needinfo?(mark.finkle)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8368353 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 8368353 [details] [diff] [review]
patch
Review of attachment 8368353 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, Errietta!
Attachment #8368353 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 6•11 years ago
|
||
Comment on attachment 8368353 [details] [diff] [review]
patch
Review of attachment 8368353 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, Errietta!
::: mobile/android/chrome/content/browser.js
@@ -127,3 @@
> notifications.forEach(function (aNotification) {
> let o = {
> notification: aNotification,
Actually, I spoke too soon. You can also get rid of these notification properties in the observers. And in fact, instead of creating an observer object, we can now just pass a function directly to the addObserver call.
Assignee | ||
Comment 7•11 years ago
|
||
Like that?
Attachment #8368378 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 8368378 [details] [diff] [review]
proposed patch
Review of attachment 8368378 [details] [diff] [review]:
-----------------------------------------------------------------
Yup, that's the right idea! I just spotted one mistake. And we should do some testing to make sure that all these modules are still working properly.
::: mobile/android/chrome/content/browser.js
@@ +127,5 @@
> let [name, notifications, resource] = module;
> XPCOMUtils.defineLazyModuleGetter(this, name, resource);
> notifications.forEach(notification => {
> + Services.obs.addObserver(function(s,t,d) {
> + this[name].observe(s,t,d)
You should keep this as an arrow function, otherwise the `this` reference won't work.
Attachment #8368378 -
Flags: review?(margaret.leibovic) → feedback+
Assignee | ||
Comment 9•11 years ago
|
||
I hope this is ok? Haven't used arrow functions before :p
How do we test this?
Attachment #8368353 -
Attachment is obsolete: true
Attachment #8368378 -
Attachment is obsolete: true
Attachment #8368642 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 8368642 [details] [diff] [review]
patch
Review of attachment 8368642 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/chrome/content/browser.js
@@ +127,5 @@
> let [name, notifications, resource] = module;
> XPCOMUtils.defineLazyModuleGetter(this, name, resource);
> notifications.forEach(notification => {
> + (s,t,d) => this[name].observe(s,t,d)
> + }, notification, false);
Oops, looks like you lost the actual addObserver call here. This should look like:
notifications.forEach(notification => {
Services.addObserver((s, t, d) => {
this[name].observe(s, t, d)
}, notification, false);
}
Here's a good post about fat arrow functions if you're looking to learn some more background:
http://robcee.net/2013/fat-arrow-functions-in-javascript/
Testing this is kinda tricky, since this "HomePanels:Get" message is fired by the PanelManager, but we haven't landed code that consumes that PanelManager API yet. So... to test this, we can just add a local hack that makes a new PanelManager object somewhere and call requestAvailablePanels, and then make sure the callback actually gets fired.
If this sounds too complicated, I can test this for you if you upload a new version of your patch.
Attachment #8368642 -
Flags: review?(margaret.leibovic) → review-
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8368642 -
Attachment is obsolete: true
Attachment #8368659 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 12•11 years ago
|
||
Comment on attachment 8368659 [details] [diff] [review]
Patch again
Review of attachment 8368659 [details] [diff] [review]:
-----------------------------------------------------------------
Nice, I tested this myself and verified the PanelManager messages are still working. Thanks!
Attachment #8368659 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → errietta
Keywords: checkin-needed
Comment 13•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug][mentor=mfinkle] → [lang=js][good first bug][mentor=mfinkle][fixed-in-fx-team]
Comment 14•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][mentor=mfinkle][fixed-in-fx-team] → [lang=js][good first bug][mentor=mfinkle]
Target Milestone: --- → Firefox 29
Updated•5 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
•