Closed Bug 1442651 Opened 6 years ago Closed 6 years ago

Remove the tabbrowser element and binding

Categories

(Firefox :: Tabbed Browser, task, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Bug 1442651 - Remove the tabbrowser element and binding.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
I know the idea was to eventually make this a custom element, but I don't think we actually need this.
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.
Should we wait until after the merge to land this given some of the subtleties with binding construction and CSS specificity changes?
(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.
Attachment #8955557 - Flags: review?(bgrinstead)
Some firefox-ui-functional tests are getting confused by bug 1442564 on Mac, so I'm going to fix that here as well.
Blocks: 1442564
(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)
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 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 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?
(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.
(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.
(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 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
(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 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+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/562966f195b0
Remove the tabbrowser element and binding. r=bgrins
Depends on: 1443318
No longer depends on: 1443362
Depends on: 1443462
https://hg.mozilla.org/mozilla-central/rev/562966f195b0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1443849
Depends on: 1443364
Depends on: 1443937
Depends on: 1443360
Depends on: 1444042
Depends on: 1444165
Depends on: 1444183
No longer depends on: 1444165
No longer depends on: 1444183
Depends on: 1444510
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
Depends on: 1458956
Depends on: 1487394
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: