888571 - Don't load CustomizeMode.jsm upon startup

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This is probably hitting our startup costs pretty heavily, and we are only loading CustomizeMode.jsm because of the tabs progress listener, for which one already exists in browser.js. Piggy-backing on that tabs progress listener can still get us the same functionality without loading the new JSM.

I've confirmed using the Browser Debugger that CustomizeMode.jsm is not loaded anymore upon startup, while typing about:customizing in the location bar and hitting Enter still works.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=0d52639d2dc8

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 769326 [details] [diff] [review]
Patch

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +37,5 @@
> +  // user. Then there's the visible palette, which gets populated and displayed
> +  // to the user when in customizing mode.
> +  this.visiblePalette = this.document.getElementById(kPaletteId);
> +
> +  this.browser.tabContainer.addEventListener("TabSelect", this, false);

Nit: remove the third argument while you're touching this.

@@ +63,5 @@
>      return this.document.getElementById("PanelUI-contents");
>    },
>  
> +  uninit: function() {
> +    if (!this._initialized)

Nit: leftover code to remove

@@ +64,5 @@
>    },
>  
> +  uninit: function() {
> +    if (!this._initialized)
> +    this.browser.tabContainer.removeEventListener("TabSelect", this, false);

ditto

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Repushed to try with review issues fixed at https://tbpl.mozilla.org/?tree=Try&rev=31c54dc42c5b

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 769326 [details] [diff] [review]
Patch

>--- a/browser/base/content/browser-customization.js
>+++ b/browser/base/content/browser-customization.js

>+  isCustomizing: function(aDocument) {
>+    return aDocument.documentElement.hasAttribute("customizing") ||
>+           aDocument.documentElement.hasAttribute("customize-exiting");
>+  },

What is aDocument good for here? browser-customization.js belongs to a browser window. 'document' is accessible here and already used elsewhere in this file.

>+      // Try not to instantiate gCustomizeMode as much as possible,
>+      // so don't use CustomizeMode.jsm to check for URI or customizing.
>+      const kAboutURI = "about:customizing";

Prevailing browser.js style is to use caps or to just write it like a normal variable in case it's not a global.

Also, "about URI" seems unclear and could clash with other random code in this method. "customizing URI" would be clearer.

>+      if (aBrowser.currentURI.spec == kAboutURI &&
>+          !CustomizationHandler.isCustomizing(document)) {
>+        gCustomizeMode.enter();
>+      } else if (aBrowser.currentURI.spec != kAboutURI &&
>+                 CustomizationHandler.isCustomizing(document)) {
>+        gCustomizeMode.exit();
>+      }

As opposed to XULBrowserWindow.onLocationChange, this code handles background tabs. This means it will call gCustomizeMode.enter when about:customizing is loaded in a background tab and gCustomizeMode.exit when something else is loaded in a background tab while customizing. This doesn't seem to make sense.

>--- a/browser/components/customizableui/src/CustomizeMode.jsm
>+++ b/browser/components/customizableui/src/CustomizeMode.jsm

>+  isCustomizing: function() {
>+    return this._customizing;
>   },

This method seems unused?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Thanks for the drive-by review. Does this look better to you?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 769332 [details] [diff] [review]
Patch v2

This looks better, but at this point it seems that you should remove CustomizeMode.jsm's code dealing with the TabSelect event -- XULBrowserWindow.onLocationChange takes care of this.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

This is great, as it now allows us to remove gCustomizeMode.uninit(), which would load CustomizeMode.jsm upon shutdown even if Customize Mode was never entered.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 769337 [details] [diff] [review]
Patch v3

>+      // Try not to instantiate gCustomizeMode as much as possible,
>+      // so don't use CustomizeMode.jsm to check for URI or customizing.
>+      let customizingURI = "about:customizing";
>+      if (browser.currentURI.spec == customizingURI &&
>+          !CustomizationHandler.isCustomizing()) {
>+        gCustomizeMode.enter();
>+      } else if (browser.currentURI.spec != customizingURI &&
>+                 CustomizationHandler.isCustomizing()) {
>+        gCustomizeMode.exit();
>+      }

you can use 'location' instead of browser.currentURI.spec

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Cool, I've made that change and repushed to try on an older revision since the UX tree is currently burning on linux:

https://tbpl.mozilla.org/?tree=Try&rev=5e1adf8dde7e

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #9)
> Cool, I've made that change and repushed to try on an older revision since
> the UX tree is currently burning on linux:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=5e1adf8dde7e

For reasons completely beyond me this doesn't seem to buy us anything for Linux tspaint. Which doesn't mean this isn't a good idea, but that's still weird... :-\

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/ee81f5bac27a

Comment 12

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #11)
> https://hg.mozilla.org/projects/ux/rev/ee81f5bac27a

This turned mochitest-browser/chrome orange on all platforms:

01:52:38 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug422590.js | Test timed out

:-(

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Backed out in https://hg.mozilla.org/projects/ux/rev/30b4e0a35ef8

Comment 14

[:Gijs (he/him)]

Now that the dep that caused the orange is fixed, this can probably safely reland, but if you want to be safe you may want to do a try run beforehand.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

I'm pretty confident that the try run won't be necessary.

Relanded, https://hg.mozilla.org/projects/ux/rev/c901ba98ea87

Comment 16

[:Gijs (he/him)]

This has landed, so marking as fixed to fix bug queries. :-)

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/c901ba98ea87