Closed Bug 959366 Opened 7 years ago Closed 7 years ago

Unused LazyNotificationGetter.shutdown function

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

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
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]
https://hg.mozilla.org/mozilla-central/rev/357071e6fd87
Status: NEW → RESOLVED
Closed: 7 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
You need to log in before you can comment on or make changes to this bug.