Closed Bug 1445737 Opened 7 years ago Closed 7 years ago

TabsInTitlebar initialization looks suspicious, and probably causes perf issues.

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Whiteboard: [fxperf])

Attachments

(4 files)

Bug 1445737: Update tabsintitlebar stuff before layout.
(deleted), text/x-review-board-request
dao
: review+
Details
Bug 1445737: Simplify a bit now that we unconditionally shuffle the window attributes after init().
(deleted), text/x-review-board-request
dao
: review+
Details
Bug 1445737: Ensure we only update tabsInTitlebar once during browser startup.
(deleted), text/x-review-board-request
dao
: review+
Details
Bug 1445737: Update stacks in perf tests.
(deleted), text/x-review-board-request
dao
: review+
Details
In particular, it does its stuff in "DOMContentLoaded", but it doesn't call update(). Whenever it calls update(), if tabsintitlebar is disabled, it'll shuffle the root element attributes around, causing probably a fair restyle. Once bug 1439875 lands, initialization, including messing up with the "chromewindow" attribute, should probably be moved to the "MozBeforeInitialXULLayout" stuff. This may be the root cause for the need for the patch for bug 1445519.
Component: General → Tabbed Browser
Whiteboard: [fxperf]
Priority: -- → P3
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment on attachment 8959497 [details] Bug 1445737: Update tabsintitlebar stuff before layout. https://reviewboard.mozilla.org/r/228296/#review234114 ::: browser/base/content/browser.js:1234 (Diff revision 1) > document.documentElement.setAttribute("sizemode", "maximized"); > } > } > + > + TabsInTitlebar.init(); > + TabsInTitlebar.updateAppearance(); Why call updateAppearance here? init should already be doing this if needed (see _updateOnInit).
Comment on attachment 8959497 [details] Bug 1445737: Update tabsintitlebar stuff before layout. https://reviewboard.mozilla.org/r/228296/#review234114 > Why call updateAppearance here? init should already be doing this if needed (see _updateOnInit). The window has the `chromemargin=` attributes set by default (they're in the XUL file), but we want to make sure they're up-to-date before doing the initial layout. They won't be if the pref is disabled, for example. So we at least need to do the equivalent of https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/browser-tabsintitlebar.js#288, but I think `update` should also be worth it, since that sets up the `tabsintitlebar` attribute properly.
Comment on attachment 8959497 [details] Bug 1445737: Update tabsintitlebar stuff before layout. https://reviewboard.mozilla.org/r/228296/#review234114 > The window has the `chromemargin=` attributes set by default (they're in the XUL file), but we want to make sure they're up-to-date before doing the initial layout. They won't be if the pref is disabled, for example. > > So we at least need to do the equivalent of https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/browser-tabsintitlebar.js#288, but I think `update` should also be worth it, since that sets up the `tabsintitlebar` attribute properly. Right now we're calling it on the resize event from the tiny window to the actual window size that bug 1439875 removes, so it doesn't flicker, but it probably would otherwise.
Comment on attachment 8959501 [details] Bug 1445737: Simplify a bit now that we unconditionally shuffle the window attributes after init(). https://reviewboard.mozilla.org/r/228308/#review234120
Attachment #8959501 - Flags: review?(dao+bmo) → review+
Comment on attachment 8959497 [details] Bug 1445737: Update tabsintitlebar stuff before layout. https://reviewboard.mozilla.org/r/228296/#review234122
Attachment #8959497 - Flags: review?(dao+bmo) → review+
Comment on attachment 8959497 [details] Bug 1445737: Update tabsintitlebar stuff before layout. https://reviewboard.mozilla.org/r/228296/#review234124 ::: commit-message-fdda0:4 (Diff revision 1) > +Bug 1445737: Update tabsintitlebar stuff before layout. r=dao > + > +Not doing can it cause perf issues, but old Linux distros aren't really happy > +about it, and they barf when the chromemargin is removed dynamically, so prevent Btw, this can't really be true, can it? chromemargin gets set and removed dynamically when toggling the Title Bar setting in customize mode.
(In reply to Dão Gottwald [::dao] from comment #9) > Comment on attachment 8959497 [details] > Bug 1445737: Update tabsintitlebar stuff before layout. > > https://reviewboard.mozilla.org/r/228296/#review234124 > > ::: commit-message-fdda0:4 > (Diff revision 1) > > +Bug 1445737: Update tabsintitlebar stuff before layout. r=dao > > + > > +Not doing can it cause perf issues, but old Linux distros aren't really happy > > +about it, and they barf when the chromemargin is removed dynamically, so prevent > > Btw, this can't really be true, can it? chromemargin gets set and removed > dynamically when toggling the Title Bar setting in customize mode. It is, this patch does fix bug 1446242. I have a machine that repros it, so I'll test if this is something particular to initialization (which could be), or it also happens in customize mode.
Comment on attachment 8959551 [details] Bug 1445737: Ensure we only update tabsInTitlebar once during browser startup. https://reviewboard.mozilla.org/r/228356/#review234240 ::: browser/base/content/tabbrowser.xml (Diff revision 1) > > <method name="handleEvent"> > <parameter name="aEvent"/> > <body><![CDATA[ > switch (aEvent.type) { > - case "DOMContentLoaded": If you remove this, you want to remove https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/tabbrowser.xml#127 too.
Comment on attachment 8959551 [details] Bug 1445737: Ensure we only update tabsInTitlebar once during browser startup. https://reviewboard.mozilla.org/r/228356/#review234246 ::: browser/base/content/browser-tabsintitlebar.js:118 (Diff revision 1) > > if (window.fullScreen) > return; > > - // In some edgecases it is possible for this to fire before we've initialized. > - if (!this._initialized) { > + // We want to do this from initialization anyway, so that the > + // "chromemargin" attributes are all correctly setup, but we don't want to _initialized can be removed now. ::: browser/base/content/browser-tabsintitlebar.js (Diff revision 1) > - if (!this._initialized) { > + // "chromemargin" attributes are all correctly setup, but we don't want to > + // do this before the DOM loads again, to prevent spurious reflows that > + // will be superseded by the one on onDOMContentLoaded. > + if (!this._domLoaded && !aFromInit) > return; > - } nit: we now usually put braces around single lines in JS
Comment on attachment 8959552 [details] Bug 1445737: Update stacks in perf tests. https://reviewboard.mozilla.org/r/228358/#review234248
Attachment #8959552 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #14) > Comment on attachment 8959551 [details] > Bug 1445737: Ensure we only update tabsInTitlebar once during browser > startup. > > https://reviewboard.mozilla.org/r/228356/#review234246 > > ::: browser/base/content/browser-tabsintitlebar.js:118 > (Diff revision 1) > > > > if (window.fullScreen) > > return; > > > > - // In some edgecases it is possible for this to fire before we've initialized. > > - if (!this._initialized) { > > + // We want to do this from initialization anyway, so that the > > + // "chromemargin" attributes are all correctly setup, but we don't want to > > _initialized can be removed now. Well, it guards against double-initialization, but I guess it can indeed go away. > ::: browser/base/content/browser-tabsintitlebar.js > (Diff revision 1) > > - if (!this._initialized) { > > + // "chromemargin" attributes are all correctly setup, but we don't want to > > + // do this before the DOM loads again, to prevent spurious reflows that > > + // will be superseded by the one on onDOMContentLoaded. > > + if (!this._domLoaded && !aFromInit) > > return; > > - } > > nit: we now usually put braces around single lines in JS Ok, put it back :) (In reply to Florian Quèze [:florian] from comment #13) > ::: browser/base/content/tabbrowser.xml > (Diff revision 1) > > > > <method name="handleEvent"> > > <parameter name="aEvent"/> > > <body><![CDATA[ > > switch (aEvent.type) { > > - case "DOMContentLoaded": > > If you remove this, you want to remove > https://searchfox.org/mozilla-central/rev/ > 6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/tabbrowser. > xml#127 too. Indeed, good catch :)
Comment on attachment 8959551 [details] Bug 1445737: Ensure we only update tabsInTitlebar once during browser startup. https://reviewboard.mozilla.org/r/228356/#review234254 ::: browser/base/content/browser-tabsintitlebar.js:45 (Diff revision 3) > }; > CustomizableUI.addListener(this); > > addEventListener("resolutionchange", this, false); > > this._initialized = true; please remove this too ::: browser/base/content/browser-tabsintitlebar.js:95 (Diff revision 3) > return; > } > } > }, > > _initialized: false, and this :) ::: browser/base/content/browser.js:1300 (Diff revision 3) > gBrowser.setIcon(gBrowser.selectedTab, "chrome://browser/skin/privatebrowsing/favicon.svg"); > } > }); > > this._setInitialFocus(); > + gBrowser.tabContainer.updateVisibility(); nit: add a blank line before this
Attachment #8959551 - Flags: review?(dao+bmo) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb2edb20085 Update tabsintitlebar stuff before layout. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/9161c98ddd46 Simplify a bit now that we unconditionally shuffle the window attributes after init(). r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ea6e952b07 Ensure we only update tabsInTitlebar once during browser startup. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5d0d34bddb Update stacks in perf tests. r=dao
Blocks: 1448078
Blocks: 1448303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: