1442651 - Remove the tabbrowser element and binding

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P1)

Description

[Dão Gottwald [:dao]]

I know the idea was to eventually make this a custom element, but I don't think we actually need this.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #0)
> I know the idea was to eventually make this a custom element, but I don't
> think we actually need this.

Yeah, I think this makes sense as a plain object as it's used in only one place and we can move the child content / styles into browser.xul.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1442651 - Remove the tabbrowser element and binding.

Review commit: https://reviewboard.mozilla.org/r/224704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224704/

Comment 3

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=004c838ced281e1436b08c2769eea6a48d513604

Comment 4

[Brian Grinstead [:bgrins]]

Should we wait until after the merge to land this given some of the subtleties with binding construction and CSS specificity changes?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Should we wait until after the merge to land this given some of the
> subtleties with binding construction and CSS specificity changes?

I'm thinking the opposite. The workaround for bug 1436351 is super gross and far from reassuring. Removing the tabbrowser and toolbox constructors as unpredictable factors sooner rather than later would make me feel better.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224704/diff/1-2/

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224704/diff/2-3/

Comment 9

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e8b335e605ac3ac40e645323b112d76120b1162

Comment 10

[Dão Gottwald [:dao]]

Some firefox-ui-functional tests are getting confused by bug 1442564 on Mac, so I'm going to fix that here as well.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224704/diff/3-4/

Comment 12

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a96e8ba57679a546aa6877db10ebdff855334ed

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Should we wait until after the merge to land this given some of the
> > subtleties with binding construction and CSS specificity changes?
> 
> I'm thinking the opposite. The workaround for bug 1436351 is super gross and
> far from reassuring. Removing the tabbrowser and toolbox constructors as
> unpredictable factors sooner rather than later would make me feel better.

I'd like a couple more opinions before we attempt to land this in 60. FWIW I really like the change and getting rid of the empty binding workaround is a win - just want to make sure we have some consensus on if this is a 'risky' patch as per the soft code freeze guidelines.

Comment 14

[:Gijs (he/him)]

If we can land this "soon", I think this works. I think this is probably less scary than bug 1392352. I also think keeping this the way it is on ESR will bite us in the long run, so I'd be in favour of landing this assuming r+, green try, etc.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

https://reviewboard.mozilla.org/r/224704/#review230976

I agree that getting this into 60 would probably be better in the long run to reduce uplift pain. The next merge date is March 12th, according to [the calendar](https://wiki.mozilla.org/RapidRelease/Calendar), which gives us a week to find and stamp out any regressions. I _suspect_ that's sufficient.

So I'm a thumbs up for getting this in if we can get it in in the short term.

::: browser/base/content/tabbrowser.js:28
(Diff revision 4)
> -      return document.getAnonymousElementByAttribute(this.container, "anonid", "tabbox");
> -    });
> -    XPCOMUtils.defineLazyGetter(this, "mPanelContainer", () => {
> -      return document.getAnonymousElementByAttribute(this.container, "anonid", "panelcontainer");
> -    });
> +    this.tabContainer = document.getElementById("tabbrowser-tabs");
> +
> +    this.tabs = this.tabContainer.childNodes;
> +
> +    this.tabbox = document.getElementById("tabbrowser-tabbox");

I notice that these were lazy before, and now they're not. Why is that?

::: browser/base/content/tabbrowser.js:1828
(Diff revision 4)
> -    b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu"));
> -    b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
> +    b.setAttribute("contextmenu", "contentAreaContextMenu");
> +    b.setAttribute("tooltip", "aHTMLTooltip");

FWIW, it might be useful to have the initial set of attributes for each browser centralized somewhere, to ensure that we keep it and the initial browser's attributes in the XBL file in sync.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

https://reviewboard.mozilla.org/r/224704/#review231002

::: browser/base/content/browser.js:1431
(Diff revision 4)
>        ZoomManager.setZoomForBrowser(browser, 1);
>      }, false, true);
>  
>      gBrowser.addEventListener("InsecureLoginFormsStateChange", function() {
>        gIdentityHandler.refreshForInsecureLoginForms();
> -    });
> +    }, true);

Why do these two need to become capturing event listeners?

::: toolkit/components/startup/tests/browser/browser_bug537449.js:33
(Diff revision 4)
>      ok(!window.closed, "Shouldn't have closed the window");
>  
>      let win2 = window.openDialog(location, "", "chrome,all,dialog=no", "about:blank");
>      ok(win2 != null, "Should have been able to open a new window");
> -    win2.addEventListener("load", function() {
> +    win2.addEventListener("load", () => {
> +      executeSoon(() => {

Why is this change needed?

Comment 17

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8955557 [details]
> Bug 1442651 - Remove the tabbrowser element and binding.
> 
> https://reviewboard.mozilla.org/r/224704/#review231002
> 
> ::: browser/base/content/browser.js:1431
> (Diff revision 4)
> >        ZoomManager.setZoomForBrowser(browser, 1);
> >      }, false, true);
> >  
> >      gBrowser.addEventListener("InsecureLoginFormsStateChange", function() {
> >        gIdentityHandler.refreshForInsecureLoginForms();
> > -    });
> > +    }, true);
> 
> Why do these two need to become capturing event listeners?

Because these events don't bubble. It worked before because the event would target the binding parent.

> ::: toolkit/components/startup/tests/browser/browser_bug537449.js:33
> (Diff revision 4)
> >      ok(!window.closed, "Shouldn't have closed the window");
> >  
> >      let win2 = window.openDialog(location, "", "chrome,all,dialog=no", "about:blank");
> >      ok(win2 != null, "Should have been able to open a new window");
> > -    win2.addEventListener("load", function() {
> > +    win2.addEventListener("load", () => {
> > +      executeSoon(() => {
> 
> Why is this change needed?

This races with the browser.js onLoad handler -- excecuteSoon ensures it runs after.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #15)
> ::: browser/base/content/tabbrowser.js:28
> (Diff revision 4)
> > -      return document.getAnonymousElementByAttribute(this.container, "anonid", "tabbox");
> > -    });
> > -    XPCOMUtils.defineLazyGetter(this, "mPanelContainer", () => {
> > -      return document.getAnonymousElementByAttribute(this.container, "anonid", "panelcontainer");
> > -    });
> > +    this.tabContainer = document.getElementById("tabbrowser-tabs");
> > +
> > +    this.tabs = this.tabContainer.childNodes;
> > +
> > +    this.tabbox = document.getElementById("tabbrowser-tabbox");
> 
> I notice that these were lazy before, and now they're not. Why is that?

These seem cheap enough not to bother, especially now that they're plain getElementById calls rather than obscure anonymous DOM APIs which might perform less than ideal.

> ::: browser/base/content/tabbrowser.js:1828
> (Diff revision 4)
> > -    b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu"));
> > -    b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
> > +    b.setAttribute("contextmenu", "contentAreaContextMenu");
> > +    b.setAttribute("tooltip", "aHTMLTooltip");
> 
> FWIW, it might be useful to have the initial set of attributes for each
> browser centralized somewhere, to ensure that we keep it and the initial
> browser's attributes in the XBL file in sync.

Yeah. I'll file a followup.

Comment 19

[Brian Grinstead [:bgrins]]

Make sure you do a try push with debug enabled. I remember when investigating this before there were cases where the window.onunload function in browser.xul didn't seem to get called: https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c25.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Make sure you do a try push with debug enabled. I remember when
> investigating this before there were cases where the window.onunload
> function in browser.xul didn't seem to get called:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c25.

Right, browser_bug537449.js was a problem in debug builds.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a96e8ba57679a546aa6877db10ebdff855334ed runs both opt and debug.

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

https://reviewboard.mozilla.org/r/224704/#review231038

There's a CSS selector in tabs.inc.css that didn't get updated: https://searchfox.org/mozilla-central/search?q=%5Etabbrowser&regexp=true&path=css

::: browser/base/content/browser.xul:70
(Diff revision 4)
>  # so that they can be shared by macBrowserOverlay.xul.
>  #include global-scripts.inc
>  
>  <script type="application/javascript">
>    Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", this);
> +  Services.scriptloader.loadSubScript("chrome://browser/content/tabbrowser.js", this);

Just noting that loading this after global-scripts.inc means that if something in browser-*.js tries to immediately access gBrowser it will be null (because TabBrowser will be undefined). I think that's OK, because at that stage the constructor in TabBrowser should fail since the DOM isn't ready yet.

::: browser/base/content/browser.xul:1226
(Diff revision 4)
> +                <vbox flex="1" class="browserContainer">
> +                  <stack flex="1" class="browserStack">
> +                    <browser id="tabbrowser-initialBrowser" type="content"
> +                             message="true" messagemanagergroup="browsers"
> +                             primary="true" blank="true"
> +                             tooltip="aHTMLTooltip"

These properties uesd to be wired up with xbl:inherits. So I'm assuming we don't ever change these (in which case they won't be synced any longer)? I don't see any time we do, just want to double check

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224704/diff/4-5/

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224704/diff/5-6/

Comment 24

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #21)
> Comment on attachment 8955557 [details]
> Bug 1442651 - Remove the tabbrowser element and binding.
> 
> https://reviewboard.mozilla.org/r/224704/#review231038
> 
> There's a CSS selector in tabs.inc.css that didn't get updated:
> https://searchfox.org/mozilla-central/
> search?q=%5Etabbrowser&regexp=true&path=css

Fixed

> ::: browser/base/content/browser.xul:70
> (Diff revision 4)
> >  # so that they can be shared by macBrowserOverlay.xul.
> >  #include global-scripts.inc
> >  
> >  <script type="application/javascript">
> >    Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", this);
> > +  Services.scriptloader.loadSubScript("chrome://browser/content/tabbrowser.js", this);
> 
> Just noting that loading this after global-scripts.inc means that if
> something in browser-*.js tries to immediately access gBrowser it will be
> null (because TabBrowser will be undefined). I think that's OK, because at
> that stage the constructor in TabBrowser should fail since the DOM isn't
> ready yet.
> 
> ::: browser/base/content/browser.xul:1226
> (Diff revision 4)
> > +                <vbox flex="1" class="browserContainer">
> > +                  <stack flex="1" class="browserStack">
> > +                    <browser id="tabbrowser-initialBrowser" type="content"
> > +                             message="true" messagemanagergroup="browsers"
> > +                             primary="true" blank="true"
> > +                             tooltip="aHTMLTooltip"
> 
> These properties uesd to be wired up with xbl:inherits. So I'm assuming we
> don't ever change these (in which case they won't be synced any longer)? I
> don't see any time we do, just want to double check

Correct. Also only the initial browser would inherit attribute changes, subsequent browsers wouldn't.

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

https://reviewboard.mozilla.org/r/224704/#review231070

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/562966f195b0
Remove the tabbrowser element and binding. r=bgrins

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/562966f195b0

Comment 28

[Ionuț Goldan [:igoldan]]

Performance wins:

== Change summary for alert #11940 (as of Mon, 05 Mar 2018 17:52:32 GMT) ==

Improvements:

  8%  tabpaint osx-10-10 opt e10s stylo     65.28 -> 60.21
  5%  tabpaint linux64 pgo e10s stylo       52.29 -> 49.55
  1%  tabpaint linux64 opt e10s stylo       53.16 -> 52.43

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11940