Closed
Bug 959366
Opened 10 years ago
Closed 10 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•10 years ago
|
||
I could take this if you tell me which one it is you want done?
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 2•10 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•10 years ago
|
||
Attachment #8368353 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 5•10 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•10 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•10 years ago
|
||
Like that?
Attachment #8368378 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 8•10 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•10 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•10 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•10 years ago
|
||
Attachment #8368642 -
Attachment is obsolete: true
Attachment #8368659 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 12•10 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•10 years ago
|
Assignee: nobody → errietta
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/357071e6fd87
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug][mentor=mfinkle] → [lang=js][good first bug][mentor=mfinkle][fixed-in-fx-team]
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/357071e6fd87
Status: NEW → RESOLVED
Closed: 10 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•3 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
•