1051985 - Move async scrolling layer properties from ContainerLayer to Layer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

This is a prerequisite for the goal of bug 967844.

Comment 1

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

Attachment: Part 1 - Move FrameMetrics from ContainerLayer to Layer

BenWa, can you look at the changes to PrintUniformityInfo and make sure they're ok?

Comment 2

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

Attachment: Part 2 - Move the scroll handoff parent id from ContainerLayer to Layer

Comment 3

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

Attachment: Part 3 - Move the background color and content description

Comment 4

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

Attachment: Part 4 - Move the APZC from ContainerLayer to Layer

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 8470916 [details] [diff] [review]
Part 1 - Move FrameMetrics from ContainerLayer to Layer

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +171,5 @@
>  
>    ContainerLayer* container = aLayer->AsContainerLayer();
>    AsyncPanZoomController* apzc = nullptr;
>    mApzcTreeLog << aLayer->Name() << '\t';
>    if (container) {

Is the container check here still useful?

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +248,5 @@
>  {
>    bool isRootFixed = aLayer->GetIsFixedPosition() &&
>      !aLayer->GetParent()->GetIsFixedPosition();
>    bool isStickyForSubtree = aLayer->GetIsStickyPosition() &&
>      aTransformedSubtreeRoot->AsContainerLayer() &&

Is the AsContainer check here still needed as well?

Comment 6

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

(In reply to Benoit Girard (:BenWa) from comment #5)
> >    mApzcTreeLog << aLayer->Name() << '\t';
> >    if (container) {
> 
> Is the container check here still useful?

This gets removed in part 4, once GetAsyncPanZoomController is moved to Layer as well.

> >    bool isRootFixed = aLayer->GetIsFixedPosition() &&
> >      !aLayer->GetParent()->GetIsFixedPosition();
> >    bool isStickyForSubtree = aLayer->GetIsStickyPosition() &&
> >      aTransformedSubtreeRoot->AsContainerLayer() &&
> 
> Is the AsContainer check here still needed as well?

Ah, good catch. We should be able to take that out as well. Will do.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8470928 [details] [diff] [review]
Part 4 - Move the APZC from ContainerLayer to Layer

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +175,1 @@
>      const FrameMetrics& metrics = aLayer->GetFrameMetrics();

Do you plan to un-indent this before landing (or in a subsequent patch), or will you keep it like this to preserve blame (if so, a comment might be appropriate)?

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +623,5 @@
>    return false;
>  }
>  
>  static bool
>  LayerIsContainerForScrollbarTarget(Layer* aTarget, ContainerLayer* aScrollbar)

Should this method name be revised to not include 'Container'?

@@ +646,5 @@
>    // avoid moving scroll-bars in the situation that only a scroll information
>    // layer has been built for a scroll frame, as this would result in a
>    // disparity between scrollbars and visible content.
> +  if (aContent->AsContainerLayer() &&
> +      !LayerHasNonContainerDescendants(aContent->AsContainerLayer())) {

We may want to check with roc if scroll-info layers will still be represented as container layers.

Comment 8

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

(In reply to Botond Ballo [:botond] from comment #7)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +175,1 @@
> >      const FrameMetrics& metrics = aLayer->GetFrameMetrics();
> 
> Do you plan to un-indent this before landing (or in a subsequent patch), or
> will you keep it like this to preserve blame (if so, a comment might be
> appropriate)?

In a future patch (which looks roughly like part 5 on bug 967844) I unindent some of it and add another if condition that preserves the indentation level for most of that code. I figured it would be easier to review if I just left the code indented here.

> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> >  static bool
> >  LayerIsContainerForScrollbarTarget(Layer* aTarget, ContainerLayer* aScrollbar)
> 
> Should this method name be revised to not include 'Container'?

Yup, I'll rename that to LayerIsScrollbarTarget.

> @@ +646,5 @@
> >    // avoid moving scroll-bars in the situation that only a scroll information
> >    // layer has been built for a scroll frame, as this would result in a
> >    // disparity between scrollbars and visible content.
> > +  if (aContent->AsContainerLayer() &&
> > +      !LayerHasNonContainerDescendants(aContent->AsContainerLayer())) {
> 
> We may want to check with roc if scroll-info layers will still be
> represented as container layers.

Good point. tn said that they might not be.

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> > We may want to check with roc if scroll-info layers will still be
> > represented as container layers.
> 
> Good point. tn said that they might not be.

... what that means though is that we need to keep this hunk of code until we land the rest of bug 967844, at which point we can get rid of it. So it makes sense to leave it in this patch for now.

Also, the try push at https://tbpl.mozilla.org/?tree=Try&rev=802e117328f3 includes these 4 patches and is green.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8470928 [details] [diff] [review]
Part 4 - Move the APZC from ContainerLayer to Layer

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +175,1 @@
>      const FrameMetrics& metrics = aLayer->GetFrameMetrics();

Just fix it now imo :)

Comment 11

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

Ok, I unindented the code and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f44a174d245f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c32803344ef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/710a213a67ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b96541d8a0d

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f44a174d245f
https://hg.mozilla.org/mozilla-central/rev/9c32803344ef
https://hg.mozilla.org/mozilla-central/rev/710a213a67ea
https://hg.mozilla.org/mozilla-central/rev/4b96541d8a0d