Closed Bug 1058884 Opened 5 years ago Closed 5 years ago

Layers code should not assume only ContainerLayers are scrollable

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: botond)

References

Details

Attachments

(1 file)

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.
I'll have a look at this.
Assignee: nobody → botond
Do we need to uplift the fixes to 34 (unless it's fixed in the next couple of days)?
(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.
Thanks - let's make sure we ask for uplift then, as this one sort of blocks bug 967844.
Attachment #8481406 - Flags: review?(bugmail.mozilla)
Attachment #8481406 - Flags: review?(bgirard)
Attachment #8481406 - Flags: review?(bgirard) → review+
(In reply to (away|aug30-sep3) Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8481406 - Attachment description: Part 1 - Update code for drawing scrollable layer border → Update code for drawing scrollable layer border
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.
(In reply to (away|aug30-sep3) Kartikaya Gupta (email:kats@mozilla.com) 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).
^ Sounds good. (I totally missed you saying that in comment 6 for some reason).
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.
Attachment #8481406 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/4911e706d559
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.