Closed Bug 959366 Opened 11 years ago Closed 11 years ago

Unused LazyNotificationGetter.shutdown function

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

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)

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
I could take this if you tell me which one it is you want done?
Flags: needinfo?(margaret.leibovic)
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)
Yeah, I guess we can remove it.
Flags: needinfo?(mark.finkle)
Attached patch patch (obsolete) — Splinter Review
Attachment #8368353 - Flags: review?(margaret.leibovic)
Comment on attachment 8368353 [details] [diff] [review] patch Review of attachment 8368353 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Errietta!
Attachment #8368353 - Flags: review?(margaret.leibovic) → review+
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.
Attached patch proposed patch (obsolete) — Splinter Review
Like that?
Attachment #8368378 - Flags: review?(margaret.leibovic)
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+
Attached patch patch (obsolete) — Splinter Review
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)
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-
Attached patch Patch againSplinter Review
Attachment #8368642 - Attachment is obsolete: true
Attachment #8368659 - Flags: review?(margaret.leibovic)
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+
Assignee: nobody → errietta
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug][mentor=mfinkle] → [lang=js][good first bug][mentor=mfinkle][fixed-in-fx-team]
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: