Remove the tabbrowser element and binding

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

a year ago
Blocks: 1401846
(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 hidden (mozreview-request)
Should we wait until after the merge to land this given some of the subtleties with binding construction and CSS specificity changes?
(Assignee)

Comment 5

a year ago
(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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8955557 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

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

Updated

a year ago
Blocks: 1442564
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 14

a year ago
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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

a year ago
mozreview-review
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.
Flags: needinfo?(mconley)

Comment 16

a year ago
mozreview-review
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?
(Assignee)

Comment 17

a year ago
(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.
(Assignee)

Comment 18

a year ago
(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.
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.
(Assignee)

Comment 20

a year ago
(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

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
(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

a year ago
mozreview-review
Comment on attachment 8955557 [details]
Bug 1442651 - Remove the tabbrowser element and binding.

https://reviewboard.mozilla.org/r/224704/#review231070
Attachment #8955557 - Flags: review?(bgrinstead) → review+

Comment 26

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/562966f195b0
Remove the tabbrowser element and binding. r=bgrins
(Assignee)

Updated

a year ago
Depends on: 1443318
(Assignee)

Updated

a year ago
No longer depends on: 1443362
(Assignee)

Updated

a year ago
Depends on: 1443462
https://hg.mozilla.org/mozilla-central/rev/562966f195b0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
(Assignee)

Updated

a year ago
Depends on: 1443849
(Assignee)

Updated

a year ago
Depends on: 1443364

Updated

a year ago
Depends on: 1443937
(Assignee)

Updated

a year ago
Depends on: 1443360
(Assignee)

Updated

a year ago
Depends on: 1444042
Depends on: 1444165
Depends on: 1444183
(Assignee)

Updated

a year ago
No longer depends on: 1444165
(Assignee)

Updated

a year ago
No longer depends on: 1444183
Depends on: 1444510

Updated

a year ago
Depends on: 1444845
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
Depends on: 1457355
(Assignee)

Updated

11 months ago
Depends on: 1458956

Updated

7 months ago
Depends on: 1487394
You need to log in before you can comment on or make changes to this bug.