1262969 - Turn the tabbrowser background color into a ColorLayer

Status: NEW

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: patch

The <tabbrowser> in the Firefox main browser window has a background color. This background color is not usually visible, but it can appear briefly when opening new tabs (see bug 558585), and with e10s probably in more cases, e.g. during slow tab switching or when the content process crashes.

In non-e10s mode, we know that the tab content is opaque, so we usually cull the tabbrowser background color during drawing. However, in e10s mode, we don't treat the tab content RefLayer as opaque, and we keep all the pixels for the tabbrowser background color in memory. This does not slow down compositing, because we do more culling before we composite, but it does mean we waste memory.

If we wrap the <tabbrowser> in a ContainerLayer using layer="true", then the tabbrowser background color gets turned into a ColorLayer and does not take up any memory. It also makes life a little easier for me in bug 1169502.

I'm going to push this patch to try and see whether it's happy with it.

Comment 1

[Markus Stange [:mstange]]

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

Comment 2

[Markus Stange [:mstange]]

So it seems to pass tests, but it doesn't help me with the test failure in bug 1169502. I'll try to find out what's going on before requesting review here.

Comment 3

[Markus Stange [:mstange]]

Oh, on Windows <vbox id="appcontent"> also has a background-color:

  #appcontent:not(:-moz-lwtheme) {
    background-color: -moz-dialog;
  }

I'll try to set the layer attribute on #appcontent instead.

Comment 4

[Markus Stange [:mstange]]

Moving it to #appcontent fixes my test failure. Let's do that.

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1262969 - Wrap #appcontent in a ContainerLayer. r?dao

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8740259 [details]
MozReview Request: Bug 1262969 - Wrap #appcontent in a ContainerLayer. r?dao

https://reviewboard.mozilla.org/r/45679/#review42447

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8740259 [details]
MozReview Request: Bug 1262969 - Wrap #appcontent in a ContainerLayer. r?dao

Sorry, the previous comments don't explain much to me. What kind of test failures did you run into and aren't front-end hacks the wrong way to handle those? Logically, according to comment 0, the attribute should be on #content, not #appcontent which is just a random wrapper around tabbrowser /and other things/. It doesn't help either that this obscure change comes with no in-code documentation.

Comment 8

[Markus Stange [:mstange]]

(In reply to Dão Gottwald [:dao] from comment #7)
> What kind of test
> failures did you run into

Let's not worry about the test failure. It has nothing to do with the true motivation for this change.
The true motivation is e10s memory usage.

> Logically, according to comment 0, the attribute should be on
> #content, not #appcontent which is just a random wrapper around tabbrowser
> /and other things/.

Thinking about this again, the "and other things" part is a problem. As soon as the layer=true element contains more than just a background-color item, we fall back to keeping the pixels around.

The reason I set the layer attribute on #appcontent was the rule I found in comment 3. Dão, do you know why we need that rule?

To fix this properly, I think we need to do the following:
 - Remove all background-color rules for tab backgrounds.
 - Set the background-color on each tab's <browser>, and make each <browser> layer="true". Or even tweak layout to automatically wrap remote browsers in a ContainerLayer.
We need the notification bars to be outside the layer="true" element. Otherwise, the notification bars will be added to the same layer as the background color, and they'll cause the whole layer to become pixel-backed.

Dão, how does this sound?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Markus Stange [:mstange] from comment #8)
> The reason I set the layer attribute on #appcontent was the rule I found in
> comment 3. Dão, do you know why we need that rule?

blame points to bug 621222 & bug 622328 

> To fix this properly, I think we need to do the following:
>  - Remove all background-color rules for tab backgrounds.
>  - Set the background-color on each tab's <browser>, and make each <browser>
> layer="true". Or even tweak layout to automatically wrap remote browsers in
> a ContainerLayer.
> We need the notification bars to be outside the layer="true" element.
> Otherwise, the notification bars will be added to the same layer as the
> background color, and they'll cause the whole layer to become pixel-backed.
> 
> Dão, how does this sound?

Sounds good to me. +1 for letting layout automatically do the right thing for remote browsers if that's what we generally want.