+++ This bug was initially created as a clone of Bug #1448078 +++

Comment on attachment 8962122 [details] Bug 1448303 - Refactor TabsInTitlebar initial update handling. https://reviewboard.mozilla.org/r/230968/#review236542 It seems this patch contains 2 different things: - calling window.TabBarVisibility.update() before the initial xul layout rather than in the DOMContentLoaded listener. I assume this is expected to save us some restyles, but I would like if you could clarify in the bug which problem you are fixing. - a refactoring that makes the code nicer but doesn't seem immediately related. If so, this would have been easier to review in a separate patch. ::: browser/base/content/browser-tabsintitlebar.js:148 (Diff revision 2) > + > + updateTitlebarDisplay(); > + > + this._layOutTitlebar(); > + > + ToolbarIconColor.inferFromText("tabsintitlebar", TabsInTitlebar.enabled); Let's not call the enabled getter which queries the information from the dom when we already have the 'allowed' local variable in the scope that contains the same information. ::: browser/base/content/browser.js:1234 (Diff revision 2) > document.documentElement.setAttribute("sizemode", "maximized"); > } > } > > - new LightweightThemeConsumer(document); > - CompactTheme.init(); > + // Update the chromemargin attribute so the window can be sized correctly. > + window.TabBarVisibility.update(); It looks strange that this uses the 'window.' prefix when the next line doesn't. Can we remove it to be consistent here?

(In reply to Florian Quèze [:florian] from comment #4) > Comment on attachment 8962122 [details] > Bug 1448303 - Refactor TabsInTitlebar initial update handling. > > https://reviewboard.mozilla.org/r/230968/#review236542 > > It seems this patch contains 2 different things: > - calling window.TabBarVisibility.update() before the initial xul layout > rather than in the DOMContentLoaded listener. I assume this is expected to > save us some restyles, but I would like if you could clarify in the bug > which problem you are fixing. TabsInTitlebar needs to know whether the tab bar is visibly, and for that we have TabsInTitlebar.allowedBy("tabs-visible", ...). But if that isn't called before TabsInTitlebar does its initial update, TabsInTitlebar will just assume that it can draw in the titlebar, which is correct in full browser windows but not in popup windows. I believe this doesn't cause perceivable problems right now but it's at least fragile. > - a refactoring that makes the code nicer but doesn't seem immediately > related. If so, this would have been easier to review in a separate patch. I had to break out the code that's now in _layOutTitlebar in order to avoid running it before the initial layout. This is pointless as TabsInTitlebar doesn't have the information to do the layout correctly, which is why we need to do it again in onDOMContentLoaded. With this patch we'll skip the first layout attempt that cannot work. > ::: browser/base/content/browser.js:1234 > (Diff revision 2) > > document.documentElement.setAttribute("sizemode", "maximized"); > > } > > } > > > > - new LightweightThemeConsumer(document); > > - CompactTheme.init(); > > + // Update the chromemargin attribute so the window can be sized correctly. > > + window.TabBarVisibility.update(); > > It looks strange that this uses the 'window.' prefix when the next line > doesn't. Can we remove it to be consistent here? eslint complains because no-undef. There are various ways to work around it, none seemed particularly pretty to me. Or maybe there's a clean way to fix this that I'm not aware of. How would you prefer I deal with it?

(In reply to Dão Gottwald [::dao] from comment #5) Thanks for the explanations. > > ::: browser/base/content/browser.js:1234 > > (Diff revision 2) > > > document.documentElement.setAttribute("sizemode", "maximized"); > > > } > > > } > > > > > > - new LightweightThemeConsumer(document); > > > - CompactTheme.init(); > > > + // Update the chromemargin attribute so the window can be sized correctly. > > > + window.TabBarVisibility.update(); > > > > It looks strange that this uses the 'window.' prefix when the next line > > doesn't. Can we remove it to be consistent here? > > eslint complains because no-undef. There are various ways to work around it, > none seemed particularly pretty to me. Or maybe there's a clean way to fix > this that I'm not aware of. How would you prefer I deal with it? You can ask Standard8 in #eslint if he has an opinion, but if this turns out to not be completely trivial, I'm ok with dropping this issue, as you were just moving this line.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3e32fb05601e Refactor TabsInTitlebar initial update handling. r=florian

*sigh* Looks like I forgot again to update the new stacks that were added somewhere in the middle of browser_windowopen.js. Pretty annoying. :(

Backed out - expected to fail on browser_windowopen.js backout: https://hg.mozilla.org/integration/autoland/rev/178c90db7421 push that got backed out: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3e32fb05601e4ae80a7df5ac4b6cda3bf6d4426d

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e343c24ea4c0 Refactor TabsInTitlebar initial update handling. r=florian