Closed Bug 1051985 Opened 7 years ago Closed 7 years ago

Move async scrolling layer properties from ContainerLayer to Layer

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files)

This is a prerequisite for the goal of bug 967844.
BenWa, can you look at the changes to PrintUniformityInfo and make sure they're ok?
Attachment #8470916 - Flags: review?(matt.woodrow)
Attachment #8470916 - Flags: review?(bgirard)
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?
Attachment #8470916 - Flags: review?(bgirard) → review+
(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 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.
Attachment #8470928 - Flags: review?(botond) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8470916 - Flags: review?(matt.woodrow) → review+
Attachment #8470919 - Flags: review?(matt.woodrow) → review+
Attachment #8470923 - Flags: review?(matt.woodrow) → review+
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 :)
Attachment #8470928 - Flags: review?(matt.woodrow) → review+
You need to log in before you can comment on or make changes to this bug.