1448303 - Set the UI density and update the tabstrip visibility right before TabsInTitlebar needs that information

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Dão Gottwald [:dao]]

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

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1448303 - Refactor TabsInTitlebar initial update handling.

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

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8962122 [details]
Bug 1448303 - Refactor TabsInTitlebar initial update handling.

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

Comment 3

[Dão Gottwald [:dao]]

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

Comment 4

[Florian Quèze [:florian]]

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?

Comment 5

[Dão Gottwald [:dao]]

(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?

Comment 6

[Florian Quèze [:florian]]

(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.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8962122 [details]
Bug 1448303 - Refactor TabsInTitlebar initial update handling.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e32fb05601e
Refactor TabsInTitlebar initial update handling. r=florian

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[Natalia Csoregi [:nataliaCs]]

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

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8962122 [details]
Bug 1448303 - Refactor TabsInTitlebar initial update handling.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e343c24ea4c0
Refactor TabsInTitlebar initial update handling. r=florian

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e343c24ea4c0