Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 1 • 10 years ago

I'll have a look at this.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 2 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 3 • 10 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 4 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 6 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 7 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 8 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 9 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 10 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 11 • 10 years ago
landing

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

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