959366 - Unused LazyNotificationGetter.shutdown function

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[:Margaret Leibovic]

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

Comment 1

[Errietta Kostala [:errietta]]

I could take this if you tell me which one it is you want done?

Comment 2

[: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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Yeah, I guess we can remove it.

Comment 4

[Errietta Kostala [:errietta]]

Attachment: patch

Comment 5

[:Margaret Leibovic]

Comment on attachment 8368353 [details] [diff] [review]
patch

Review of attachment 8368353 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Errietta!

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[Errietta Kostala [:errietta]]

Attachment: proposed patch

Like that?

Comment 8

[: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.

Comment 9

[Errietta Kostala [:errietta]]

Attachment: patch

I hope this is ok? Haven't used arrow functions before :p
How do we test this?

Comment 10

[: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.

Comment 11

[Errietta Kostala [:errietta]]

Attachment: Patch again

Comment 12

[: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!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/357071e6fd87

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/357071e6fd87