1094234 - Get rid of unused BrowserApp.shutdown method

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We always talk about how this method is never called in practice given the Android lifecycle. Well, it turns out, it's literally never called in our code. Let's get rid of it. And all the uninit methods that it calls.

Comment 1

[:Margaret Leibovic]

Attachment: Get rid of unused BrowserApp.shutdown method

I left the UserAgentOverrides uninit method, since that lives in shared code and is used by b2g.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e12b87a079ce

Comment 2

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

Comment on attachment 8517514 [details] [diff] [review]
Get rid of unused BrowserApp.shutdown method

># HG changeset patch
># User Margaret Leibovic <[e-mail]>
># Date 1415203787 28800
>#      Wed Nov 05 08:09:47 2014 -0800
># Node ID 69d72e8cf6ede891e15be8fbb0028f8ac0e279db
># Parent  25ad77f0119335ac4111b7b96b702865cdab8c12
>Bug 1094234 - Get rid of unused BrowserApp.shutdown method
>
>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js
>--- a/mobile/android/chrome/content/CastingApps.js
>+++ b/mobile/android/chrome/content/CastingApps.js
>@@ -79,32 +79,16 @@ var CastingApps = {
>     Services.obs.addObserver(this, "ssdp-service-lost", false);
> 
>     BrowserApp.deck.addEventListener("TabSelect", this, true);
>     BrowserApp.deck.addEventListener("pageshow", this, true);
>     BrowserApp.deck.addEventListener("playing", this, true);
>     BrowserApp.deck.addEventListener("ended", this, true);
>   },
> 
>-  uninit: function ca_uninit() {
>-    BrowserApp.deck.removeEventListener("TabSelect", this, true);
>-    BrowserApp.deck.removeEventListener("pageshow", this, true);
>-    BrowserApp.deck.removeEventListener("playing", this, true);
>-    BrowserApp.deck.removeEventListener("ended", this, true);
>-
>-    Services.obs.removeObserver(this, "Casting:Play");
>-    Services.obs.removeObserver(this, "Casting:Pause");
>-    Services.obs.removeObserver(this, "Casting:Stop");
>-    Services.obs.removeObserver(this, "Casting:Mirror");
>-    Services.obs.removeObserver(this, "ssdp-service-found");
>-    Services.obs.removeObserver(this, "ssdp-service-lost");
>-
>-    NativeWindow.contextmenus.remove(this._castMenuId);
>-  },
>-
>   _mirrorStarted: function(stopMirrorCallback) {
>     this.stopMirrorCallback = stopMirrorCallback;
>     NativeWindow.menu.update(this.mirrorStartMenuId, { visible: false });
>     NativeWindow.menu.update(this.mirrorStopMenuId, { visible: true });
>   },
> 
>   serviceAdded: function(aService) {
>     if (this.isMirroringEnabled() && aService.mirror && this.mirrorStartMenuId == -1) {
>diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js

>-  uninit: function Reader_uninit() {

>-    let requests = this._requests;
>-    for (let url in requests) {
>-      let request = requests[url];
>-      if (request.browser) {
>-        let browser = request.browser;
>-        browser.parentNode.removeChild(browser);
>-      }
>-    }
>-    delete this._requests;

This part looks like we are releasing memory / resources. Maybe we should keep this method, but call it from the MemoryObserver (or from inside Reader) in response to a low memory trigger?

>diff --git a/mobile/android/chrome/content/WebcompatReporter.js b/mobile/android/chrome/content/WebcompatReporter.js

>   init: function() {
>     Services.obs.addObserver(this, "DesktopMode:Change", false);
>     Services.obs.addObserver(this, "chrome-document-global-created", false);
>     Services.obs.addObserver(this, "content-document-global-created", false);
>     this.addMenuItem();
>   },

Unrelated to this patch. I see that we pull this code into browser.js early, but it's only ever used if the menu is clicked. We need the init() to add the menu, but we could add the menu using layout.xml instead and make this code truly lazy loaded, waiting for the notifications.

It's Nightly only right? We don't call this init() method on any other channels? If so, maybe it's not worth the effort.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-  shutdown: function shutdown() {
>-    NativeWindow.uninit();
>-    LightWeightThemeWebInstaller.uninit();
>-    FormAssistant.uninit();
>-    IndexedDB.uninit();
>-    ViewportHandler.uninit();
>-    XPInstallObserver.uninit();
>-    HealthReportStatusListener.uninit();
>-    CharacterEncoding.uninit();
>-    SearchEngines.uninit();
>-    RemoteDebugger.uninit();
>-    Reader.uninit();
>-    UserAgentOverrides.uninit();
>-    DesktopUserAgent.uninit();
>-    ExternalApps.uninit();
>-    CastingApps.uninit();
>-    Distribution.uninit();
>-    Tabs.uninit();
>-#ifdef NIGHTLY_BUILD
>-    WebcompatReporter.uninit();
>-#endif
>-  },
>@@ -5461,31 +5416,16 @@ var FormAssistant = {
>     BrowserApp.deck.addEventListener("blur", this, true);
>     BrowserApp.deck.addEventListener("click", this, true);
>     BrowserApp.deck.addEventListener("input", this, false);
>     BrowserApp.deck.addEventListener("pageshow", this, false);
> 
>     LoginManagerParent.init();
>   },
> 
>-  uninit: function() {
>-    Services.obs.removeObserver(this, "FormAssist:AutoComplete");
>-    Services.obs.removeObserver(this, "FormAssist:Blocklisted");
>-    Services.obs.removeObserver(this, "FormAssist:Hidden");
>-    Services.obs.removeObserver(this, "FormAssist:Remove");
>-    Services.obs.removeObserver(this, "invalidformsubmit");
>-    Services.obs.removeObserver(this, "PanZoom:StateChange");
>-
>-    BrowserApp.deck.removeEventListener("focus", this);
>-    BrowserApp.deck.removeEventListener("blur", this);
>-    BrowserApp.deck.removeEventListener("click", this);
>-    BrowserApp.deck.removeEventListener("input", this);
>-    BrowserApp.deck.removeEventListener("pageshow", this);
>-  },

FormAssistant is one of those classes I'd like to audit to make sure we are locally controlling the notifications / handlers as well as possible

>-  uninit: function uninit() {
>-    Services.obs.removeObserver(this, "SearchEngines:Add");
>-    Services.obs.removeObserver(this, "SearchEngines:GetVisible");
>-    Services.obs.removeObserver(this, "SearchEngines:Remove");
>-    Services.obs.removeObserver(this, "SearchEngines:RestoreDefaults");
>-    Services.obs.removeObserver(this, "SearchEngines:SetDefault");
>-    Services.obs.removeObserver(this, "browser-search-engine-modified");
>-    if (this._contextMenuId != null)
>-      NativeWindow.contextmenus.remove(this._contextMenuId);
>-  },

Hmm, the second place (SearchEngines) where the only thing holding us back from lazy loading is a menu. I am considering a BrowserUI.js script, pulled in for Fennec but not for WebApps or GeckoView, that acts as a UI shim for menus and buttons. This could allow us to move the larger chunks of code out of browser.js

>-  uninit: function helper_uninit() {
>-    if (this._contextMenuId !== null) {
>-      NativeWindow.contextmenus.remove(this._contextMenuId);
>-    }
>-    this._contextMenuId = null;
>-  },

HelperApps too?

>-  uninit: function dc_uninit() {
>-    Services.obs.removeObserver(this, "Distribution:Set");
>-    Services.obs.removeObserver(this, "prefservice:after-app-defaults");
>-    Services.obs.removeObserver(this, "Campaign:Set");
>-  },

Could we make Distribution code lazy now?

I think we could r+ this now, but I want a little discussion on the questions and maybe a plan (filed bugs) for the followup before some of the code we could look at disappears.

Comment 3

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> >-    let requests = this._requests;
> >-    for (let url in requests) {
> >-      let request = requests[url];
> >-      if (request.browser) {
> >-        let browser = request.browser;
> >-        browser.parentNode.removeChild(browser);
> >-      }
> >-    }
> >-    delete this._requests;
> 
> This part looks like we are releasing memory / resources. Maybe we should
> keep this method, but call it from the MemoryObserver (or from inside
> Reader) in response to a low memory trigger?

This is going away in bug 1087722, so don't worry about it :)

> >diff --git a/mobile/android/chrome/content/WebcompatReporter.js b/mobile/android/chrome/content/WebcompatReporter.js
> 
> >   init: function() {
> >     Services.obs.addObserver(this, "DesktopMode:Change", false);
> >     Services.obs.addObserver(this, "chrome-document-global-created", false);
> >     Services.obs.addObserver(this, "content-document-global-created", false);
> >     this.addMenuItem();
> >   },
> 
> Unrelated to this patch. I see that we pull this code into browser.js early,
> but it's only ever used if the menu is clicked. We need the init() to add
> the menu, but we could add the menu using layout.xml instead and make this
> code truly lazy loaded, waiting for the notifications.
> 
> It's Nightly only right? We don't call this init() method on any other
> channels? If so, maybe it's not worth the effort.

Yeah, I feel like I don't really care if it's just on Nightly...

> FormAssistant is one of those classes I'd like to audit to make sure we are
> locally controlling the notifications / handlers as well as possible

Yeah, we can file a follow-up bug.

> >-  uninit: function uninit() {
> >-    Services.obs.removeObserver(this, "SearchEngines:Add");
> >-    Services.obs.removeObserver(this, "SearchEngines:GetVisible");
> >-    Services.obs.removeObserver(this, "SearchEngines:Remove");
> >-    Services.obs.removeObserver(this, "SearchEngines:RestoreDefaults");
> >-    Services.obs.removeObserver(this, "SearchEngines:SetDefault");
> >-    Services.obs.removeObserver(this, "browser-search-engine-modified");
> >-    if (this._contextMenuId != null)
> >-      NativeWindow.contextmenus.remove(this._contextMenuId);
> >-  },
> 
> Hmm, the second place (SearchEngines) where the only thing holding us back
> from lazy loading is a menu. I am considering a BrowserUI.js script, pulled
> in for Fennec but not for WebApps or GeckoView, that acts as a UI shim for
> menus and buttons. This could allow us to move the larger chunks of code out
> of browser.js

Sounds like a good follow-up bug.

> >-  uninit: function helper_uninit() {
> >-    if (this._contextMenuId !== null) {
> >-      NativeWindow.contextmenus.remove(this._contextMenuId);
> >-    }
> >-    this._contextMenuId = null;
> >-  },
> 
> HelperApps too?

HelperApps doesn't have an uninit method.

> >-  uninit: function dc_uninit() {
> >-    Services.obs.removeObserver(this, "Distribution:Set");
> >-    Services.obs.removeObserver(this, "prefservice:after-app-defaults");
> >-    Services.obs.removeObserver(this, "Campaign:Set");
> >-  },
> 
> Could we make Distribution code lazy now?

It looks like Distribution.init reads a JSON file to set some prefs, so I don't know if it can be lazy. Although it seems pretty bad if we're always trying to read a file on startup... could be worth a follow-up bug to investigate. However, if a distribution is present, I don't know if there's any way around this, since we'll need to set the default prefs.

Comment 4

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

(In reply to :Margaret Leibovic from comment #3)

> > >-  uninit: function helper_uninit() {
> > >-    if (this._contextMenuId !== null) {
> > >-      NativeWindow.contextmenus.remove(this._contextMenuId);
> > >-    }
> > >-    this._contextMenuId = null;
> > >-  },
> > 
> > HelperApps too?
> 
> HelperApps doesn't have an uninit method.

Sorry, I meant "whatever code this uninit method is in". I guessed at HelperApps given the "helper_uninit" method.

> > >-  uninit: function dc_uninit() {
> > >-    Services.obs.removeObserver(this, "Distribution:Set");
> > >-    Services.obs.removeObserver(this, "prefservice:after-app-defaults");
> > >-    Services.obs.removeObserver(this, "Campaign:Set");
> > >-  },
> > 
> > Could we make Distribution code lazy now?
> 
> It looks like Distribution.init reads a JSON file to set some prefs, so I
> don't know if it can be lazy. Although it seems pretty bad if we're always
> trying to read a file on startup... could be worth a follow-up bug to
> investigate. However, if a distribution is present, I don't know if there's
> any way around this, since we'll need to set the default prefs.

My interest is two-fold:
1. Can the code be lazy initied giving browser.js a faster startup?
2. Can the code be avoided in WebApp and GeckoView use cases?

In this case, I think #1 is not going to happen since we need to listen for distribution work at startup. But maybe #2 could be achieved?

Comment 5

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

Comment on attachment 8517514 [details] [diff] [review]
Get rid of unused BrowserApp.shutdown method

Discussed. Followups identified.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6c9c21a00ce5

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c9c21a00ce5