1058884 - Layers code should not assume only ContainerLayers are scrollable

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

In the recent past only ContainerLayers could hold FrameMetrics and be scrollable, but that is no longer the case, as of bug 1051985. There are still some bits of code that assume this, though. They should be moved/rewritten to not make that assumption.

1) Layer borders at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp?rev=8bf7fb1836c1#424
2) The background-color drawing code at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp?rev=8bf7fb1836c1#257
3) Maybe the uniformity print at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp?rev=8bf7fb1836c1#302 - I'm not sure what the PreparedLayers business is and if it includes all layers in the layer tree or not.

Comment 1

[Botond Ballo [:botond]]

I'll have a look at this.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Do we need to uplift the fixes to 34 (unless it's fixed in the next couple of days)?

Comment 3

[Botond Ballo [:botond]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Do we need to uplift the fixes to 34 (unless it's fixed in the next couple
> of days)?

If roc's patches in bug 967844 land in 34, then it would probably be useful to get this into 34 too so as to not break layer border drawing.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks - let's make sure we ask for uplift then, as this one sort of blocks bug 967844.

Comment 5

[Botond Ballo [:botond]]

Attachment: Update code for drawing scrollable layer border

Comment 6

[Botond Ballo [:botond]]

(In reply to (away|aug30-sep3) Kartikaya Gupta (email:[e-mail]) from comment #0)
> 2) The background-color drawing code at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> ContainerLayerComposite.cpp?rev=8bf7fb1836c1#257

This code is going to be removed as the overscroll effect is being changed to one that doesn't expose blank space in the composited region (bug 1057578), so there's no point in updating it.

> 3) Maybe the uniformity print at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> ContainerLayerComposite.cpp?rev=8bf7fb1836c1#302 - I'm not sure what the
> PreparedLayers business is and if it includes all layers in the layer tree
> or not.

I talked to Jeff about this; every layer that appears on the screen is processed in the loop in RenderLayers, either when called directly by ContainerRender (for layers drawn directly to the screen), or when called by RenderIntermediate (for layers drawn to an intermediate surface), so it seems the call to PrintUniformityInfo is fine.

Therefore, there is nothing else to be done for this bug beyond existing patch for scrollable layer borders.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8481406 [details] [diff] [review]
Update code for drawing scrollable layer border

Review of attachment 8481406 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +325,5 @@
> +
> +    // Draw a border around scrollable layers.
> +    for (uint32_t i = 0; i < layer->GetFrameMetricsCount(); i++) {
> +      // A layer can be scrolled by multiple scroll frames. Draw a border
> +      // for each.

Will this code get run for every layer in the tree? I don't know how the prepared layers array works but I assumed it only got run for children of the container layer, in which case container layers with scrollable FrameMetrics are now going to get skipped.

Comment 8

[Botond Ballo [:botond]]

(In reply to (away|aug30-sep3) Kartikaya Gupta (email:[e-mail]) from comment #7)
> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +325,5 @@
> > +
> > +    // Draw a border around scrollable layers.
> > +    for (uint32_t i = 0; i < layer->GetFrameMetricsCount(); i++) {
> > +      // A layer can be scrolled by multiple scroll frames. Draw a border
> > +      // for each.
> 
> Will this code get run for every layer in the tree? I don't know how the
> prepared layers array works but I assumed it only got run for children of
> the container layer, in which case container layers with scrollable
> FrameMetrics are now going to get skipped.

ContainerLayers are children of (other) ContainerLayers, too - with the exception of the rootmost layer, which cannot currently have an APZC (I don't know if that will change when we turn on APZ in the parent process; if it does, I suggest we update the code then).

Comment 9

[Kartikaya Gupta (email:[e-mail])]

^ Sounds good. (I totally missed you saying that in comment 6 for some reason).

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8481406 [details] [diff] [review]
Update code for drawing scrollable layer border

Review of attachment 8481406 [details] [diff] [review]:
-----------------------------------------------------------------

... and apparently I forgot to mark r+ on this. whoops.

Comment 11

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4911e706d559

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4911e706d559