Jared Wein [:jaws] (please needinfo? me) Assignee
	Description • 11 years ago

My homegrown profiler has shown that registerToolbar is the worst offender of CustomizableUIInternal when opening windows in tpaint.

The attached patch folds some calls to hasAttribute/getAttribute. It also removes isBuildAreaRegistered because the registerBuildArea already protects against a pre-registered build area.

While here I noticed that the aContainer argument of setLocationAttributes is unused so I killed it with fire.

Baseline try push:
https://tbpl.mozilla.org/?tree=Try&rev=9f4f2e41cc7c

With patch:
https://tbpl.mozilla.org/?tree=Try&rev=e527965b8891

	:Gijs (he/him)
	Comment 1 • 11 years ago

Comment on attachment 778329 [details] [diff] [review]
registerToolbar.patch

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

Driveby feedback - sorry. Looks good, but I have some suggestions/comments...

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -474,5 @@
>    registerMenuPanel: function(aPanel) {
> -    if (this.isBuildAreaRegistered(CustomizableUI.AREA_PANEL, aPanel)) {
> -      return;
> -    }
> -

Actually, this function gets called whenever the menupanel is opened (ensureRegistered in panelUI is called). Executing all the rest of this function every time seems senseless. If this case of running it twice is not an issue for toolbars, excellent, but then let's move the code here, or to panelUI, so we don't do this more than once...

@@ +1737,3 @@
>        let parent = aElement.parentNode;
> +      let parentFlex = parseInt(parent.getAttribute("flex") || 0, 10);
> +      elementFlex = parseInt(elementFlex, 10);

This will be NaN if add-ons put bogus values in the attribute. Should we use || 0 to care about that?

Also, if we're going for speed, you can just coerce the string value rather than using parseInt (which is slower because it can also deal with "123junk" rather than just "123").

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 2 • 11 years ago

Gijs: Thank you for the feedback!

I've updated the patch to use type coercion instead of parseInt and sent it tryserver (also including mochitest-bc), https://tbpl.mozilla.org/?tree=Try&rev=e68904b42bcd

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 3 • 11 years ago

I failed on getting mochitest-bc tests to run because I screw up the syntax for the try push. Repushed to try server to just run the mochitest-browser-chrome test suite, https://tbpl.mozilla.org/?tree=Try&rev=1bcde58db628

	:Gijs (he/him)
	Comment 4 • 11 years ago

Comment on attachment 778678 [details] [diff] [review]
895778.patch

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

LGTM.

	Matthew N. [:MattN]
	Comment 5 • 11 years ago

Comment on attachment 778678 [details] [diff] [review]
895778.patch

Gijs knows this code better than me.

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 11 years ago

Comment on attachment 778678 [details] [diff] [review]
895778.patch

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

Ditto!

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 11 years ago

https://hg.mozilla.org/projects/ux/rev/69e73126983c

	:Gijs (he/him)
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/69e73126983c