Set up scroll and clip parents

Set up scroll and clip parents

There are two interesting components to this patch.

1) I've added an ancestor scroll clipping layer to the RLB's hierarchy. This is
used to ensure that the clip required by scroll children is processed in cc
before we visit the scroll children. In previous versions of this patch, this
required some awkward math, but I realized that this layer shares the same
size and position as our scroll parent's m_scrollingLayer, so I could just
steal those values. However, to use this I had to...

2) update RLC::rebuildCompositingLayerTree and RLC::updateLayerTreeGeometry so
that we visit our children in an order that guarantees scroll parents will
be visited before scroll children.

This turned out to be relatively simple. We can visit the children in any order,
provided that when we're rebuilding the layer tree we populate the list of child
layers in the correct order. So we can visit the normal flow list first (which
is where the scroll parent will usually live and where scroll children -- who
must be stacking containers -- will not live), and modify the pos/neg z-order
list traversal so that we could visit scroll parents ahead of time, if necessary
(the scroll parent can potentially be in the pos z-order list (with z-index 0) if
it has a positioned ancestor).

This is blink-side patch 5 of https://docs.google.com/a/chromium.org/document/d/1Ln57_v6_4RmuuvtWnpsfBOUeDFrPsYNFW13dwuQ8UD8

For more details, please refer to the design doc
https://docs.google.com/document/d/1Ln57_v6_4RmuuvtWnpsfBOUeDFrPsYNFW13dwuQ8UD8/edit?usp=sharing

BUG=249349, 249357
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157710

[Ian Vollick, 2013-09-05 02:49:03]

Hey enne!

This is the ready-for-review version of
https://codereview.chromium.org/22419002/.

I've managed to get rid of nearly all the complexity in RenderLayer (no more
changes to clip rect computation) and in RenderLayerBacking (no more complex
geometry).

I did have to add a bit of complexity to RLC to ensure that I've updated the
geometry of scroll parents before their children, but hopefully it's not to
awful.

PTAL!

[Ian Vollick, 2013-09-05 03:07:09]

On 2013/09/05 02:49:03, Ian Vollick wrote:
> Hey enne!
> 
> This is the ready-for-review version of
> https://codereview.chromium.org/22419002/.
> 
> I've managed to get rid of nearly all the complexity in RenderLayer (no more
> changes to clip rect computation) and in RenderLayerBacking (no more complex
> geometry).
> 
> I did have to add a bit of complexity to RLC to ensure that I've updated the
> geometry of scroll parents before their children, but hopefully it's not to
> awful.
> 
> PTAL!

> Hey enne!
> 
> This is the ready-for-review version of
> https://codereview.chromium.org/22419002/.
> 
> I've managed to get rid of nearly all the complexity in RenderLayer (no more
> changes to clip rect computation) and in RenderLayerBacking (no more complex
> geometry).
> 
> I did have to add a bit of complexity to RLC to ensure that I've updated the
> geometry of scroll parents before their children, but hopefully it's not to
> awful.
> 
> PTAL!

> Source/core/page/scrolling/ScrollingCoordinator.cpp:502: if (!child ||
> !child->backing())
> Shouldn't these always be true?

Yep, removed.

[Ian Vollick, 2013-09-05 21:11:26]

On 2013/09/05 03:07:09, Ian Vollick wrote:
> On 2013/09/05 02:49:03, Ian Vollick wrote:
> > Hey enne!
> > 
> > This is the ready-for-review version of
> > https://codereview.chromium.org/22419002/.
> > 
> > I've managed to get rid of nearly all the complexity in RenderLayer (no more
> > changes to clip rect computation) and in RenderLayerBacking (no more complex
> > geometry).
> > 
> > I did have to add a bit of complexity to RLC to ensure that I've updated the
> > geometry of scroll parents before their children, but hopefully it's not to
> > awful.
> > 
> > PTAL!
> 
> > Hey enne!
> > 
> > This is the ready-for-review version of
> > https://codereview.chromium.org/22419002/.
> > 
> > I've managed to get rid of nearly all the complexity in RenderLayer (no more
> > changes to clip rect computation) and in RenderLayerBacking (no more complex
> > geometry).
> > 
> > I did have to add a bit of complexity to RLC to ensure that I've updated the
> > geometry of scroll parents before their children, but hopefully it's not to
> > awful.
> > 
> > PTAL!
> 
> > Source/core/page/scrolling/ScrollingCoordinator.cpp:502: if (!child ||
> > !child->backing())
> > Shouldn't these always be true?
> 
> Yep, removed.

Improved the layout test. (Also, fixed a typo in the test I was cribbing from).

[hartmanng, 2013-09-06 01:46:37]

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2100: // rects. Semantically, though, this
is const, so we will cast here.
I wonder if it might be cleaner to make the cached clip rects mutable instead?
just a thought.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:553: // Our scroll parent must have
been processed before us.
I think a slightly longer comment explaining why our we know our parent has
already been processed might be useful here.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1050: void
RenderLayerCompositor::rebuildCompositingLayerTreeForLayerAndScrollParents(RenderLayer*
curLayer, Vector<GraphicsLayer*>& childList, HashSet<RenderLayer*>& visited,
HashMap<RenderLayer*, Vector<GraphicsLayer*> >& scrollParentChildLists, int
depth)
I think this function could use an explanation in a comment

[Ian Vollick, 2013-09-06 02:38:10]

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2100: // rects. Semantically, though, this
is const, so we will cast here.
On 2013/09/06 01:46:38, hartmanng wrote:
> I wonder if it might be cleaner to make the cached clip rects mutable instead?
> just a thought.
(blush) I was wrong. As I went to do this, I realized that the const-ification
wouldn't even need to go that far. I've updated RLC::clippedByAncestor and it
looks like that was sufficient.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:553: // Our scroll parent must have
been processed before us.
On 2013/09/06 01:46:38, hartmanng wrote:
> I think a slightly longer comment explaining why our we know our parent has
> already been processed might be useful here.

Done.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1050: void
RenderLayerCompositor::rebuildCompositingLayerTreeForLayerAndScrollParents(RenderLayer*
curLayer, Vector<GraphicsLayer*>& childList, HashSet<RenderLayer*>& visited,
HashMap<RenderLayer*, Vector<GraphicsLayer*> >& scrollParentChildLists, int
depth)
On 2013/09/06 01:46:38, hartmanng wrote:
> I think this function could use an explanation in a comment

Done.

[hartmanng, 2013-09-06 20:35:19]

rebuildCompositingLayerTreeForLayerAndScrollParents() is still a bit tough to
grasp, but I think overall it's looking pretty good.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1050: void
RenderLayerCompositor::rebuildCompositingLayerTreeForLayerAndScrollParents(RenderLayer*
curLayer, Vector<GraphicsLayer*>& childList, HashSet<RenderLayer*>& visited,
HashMap<RenderLayer*, Vector<GraphicsLayer*> >& scrollParentChildLists, int
depth)
On 2013/09/06 02:38:11, Ian Vollick wrote:
> On 2013/09/06 01:46:38, hartmanng wrote:
> > I think this function could use an explanation in a comment
> 
> Done.

Much better, I think this makes the purpose of the function clear. I still find
it a bit hard to follow the implementation though. Maybe a short description of
how a few of the collections are used (childList vs scrollParentChildList vs
scrollParentChildLists) and a sentence about each of the if blocks?

[Ian Vollick, 2013-09-06 23:00:58]

Thanks for the review, Glenn. Hopefully the comments are beefy enough now.

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1050: void
RenderLayerCompositor::rebuildCompositingLayerTreeForLayerAndScrollParents(RenderLayer*
curLayer, Vector<GraphicsLayer*>& childList, HashSet<RenderLayer*>& visited,
HashMap<RenderLayer*, Vector<GraphicsLayer*> >& scrollParentChildLists, int
depth)
On 2013/09/06 20:35:20, hartmanng wrote:
> On 2013/09/06 02:38:11, Ian Vollick wrote:
> > On 2013/09/06 01:46:38, hartmanng wrote:
> > > I think this function could use an explanation in a comment
> > 
> > Done.
> 
> Much better, I think this makes the purpose of the function clear. I still
find
> it a bit hard to follow the implementation though. Maybe a short description
of
> how a few of the collections are used (childList vs scrollParentChildList vs
> scrollParentChildLists) and a sentence about each of the if blocks?

Done.

[hartmanng, 2013-09-08 18:30:02]

On 2013/09/06 23:00:58, Ian Vollick wrote:
> Thanks for the review, Glenn. Hopefully the comments are beefy enough now.
> 

Yeah, that's a lot better, I think it's much more understandable now. LG2M.

[enne (OOO), 2013-09-10 23:45:01]

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5369: // want to scroll the root of the
layer's associate graphics layer subtree. I.e., we want it
associated?

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:552: if (compAncestor &&
m_ancestorScrollClippingLayer) {
Just as a sanity check, the ancestor scroll clipping layer is to essentially
apply the clip for the overflow scroll to scroll child.  Scroll parent / scroll
child are potentially siblings in the RenderLayer (and GraphicsLayer) tree.  So,
you need to both scroll the scroll child (which happens in cc) but also have the
correct clip (which needs to be added here, per scroll child)?  And, the clip
needs to be in the space that the clip for the scroll parent is in, so you're
compensating here?

I still feel like this patch really needs a diagram, even if it's just a comment
in outline form of what scroll parent and scroll child layer tree looks like,
what layers are optional, when they're needed, and what needs to move where and
why.

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:563: position = position +
toFloatSize(scrollAncestor->position());
Maybe also ASSERT that there's no transform applied to these layers?

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:255: OwnPtr<GraphicsLayer>
m_ancestorScrollClippingLayer; // Only used if we have a scroll parent, and our
clipping ancestor is not our scroll parent.
The code looks like it says this is used if and only if there is a scroll
parent?

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1052: // compositing layer tree
for curLayer will use values we computed for the scroll parent. Unfortunately,
I understand why you need HashSet visited for updateLayerTreeGeometry, because
of the conditional about m_ancestorScrollClippingLayer
RenderLayerBacking::updateGraphicsLayerConfiguration.  However, I don't
understand the connection to rebuildCompositingLayerTree here and why you have
to do that for the scroll parent first.  What am I missing?

[Ian Vollick, 2013-09-11 17:57:51]

Thanks for the review! I've tried to address your review and I've added some
pretty pictures. PTAL :)

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5369: // want to scroll the root of the
layer's associate graphics layer subtree. I.e., we want it
On 2013/09/10 23:45:01, enne wrote:
> associated?

Done.

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:552: if (compAncestor &&
m_ancestorScrollClippingLayer) {
On 2013/09/10 23:45:01, enne wrote:
> Just as a sanity check, the ancestor scroll clipping layer is to essentially
> apply the clip for the overflow scroll to scroll child.  Scroll parent /
scroll
> child are potentially siblings in the RenderLayer (and GraphicsLayer) tree. 
So,
> you need to both scroll the scroll child (which happens in cc) but also have
the
> correct clip (which needs to be added here, per scroll child)?  And, the clip
> needs to be in the space that the clip for the scroll parent is in, so you're
> compensating here?
> 
> I still feel like this patch really needs a diagram, even if it's just a
comment
> in outline form of what scroll parent and scroll child layer tree looks like,
> what layers are optional, when they're needed, and what needs to move where
and
> why.

I've added a detailed description of all layers in the hierarchy in RLB.h. Can
you please let me know if it's the sort of thing you're after?

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:563: position = position +
toFloatSize(scrollAncestor->position());
On 2013/09/10 23:45:01, enne wrote:
> Maybe also ASSERT that there's no transform applied to these layers?

Done.

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:255: OwnPtr<GraphicsLayer>
m_ancestorScrollClippingLayer; // Only used if we have a scroll parent, and our
clipping ancestor is not our scroll parent.
On 2013/09/10 23:45:01, enne wrote:
> The code looks like it says this is used if and only if there is a scroll
> parent?
It is. I've updated the comment.

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1052: // compositing layer tree
for curLayer will use values we computed for the scroll parent. Unfortunately,
On 2013/09/10 23:45:01, enne wrote:
> I understand why you need HashSet visited for updateLayerTreeGeometry, because
> of the conditional about m_ancestorScrollClippingLayer
> RenderLayerBacking::updateGraphicsLayerConfiguration.  However, I don't
> understand the connection to rebuildCompositingLayerTree here and why you have
> to do that for the scroll parent first.  What am I missing?

There are two recursive functions in the RLC which are in change of calling
RLB::updateGraphicsLayerConfiguration, updateLayerTreeGeometry --as you
mentioned -- and rebuildCompositingLayerTree. So I think that in both cases we
must ensure that we recur on the scroll parent before the scroll children.

[enne (OOO), 2013-09-11 23:18:29]

lgtm

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/31001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1052: // compositing layer tree
for curLayer will use values we computed for the scroll parent. Unfortunately,
On 2013/09/11 17:57:52, Ian Vollick wrote:
> On 2013/09/10 23:45:01, enne wrote:
> > I understand why you need HashSet visited for updateLayerTreeGeometry,
because
> > of the conditional about m_ancestorScrollClippingLayer
> > RenderLayerBacking::updateGraphicsLayerConfiguration.  However, I don't
> > understand the connection to rebuildCompositingLayerTree here and why you
have
> > to do that for the scroll parent first.  What am I missing?
> 
> There are two recursive functions in the RLC which are in change of calling
> RLB::updateGraphicsLayerConfiguration, updateLayerTreeGeometry --as you
> mentioned -- and rebuildCompositingLayerTree. So I think that in both cases we
> must ensure that we recur on the scroll parent before the scroll children.

Sad times.  I missed that they both call updateGraphicsLayerConfiguration.

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2080: // Now, if we reside in a normal
flow list, then we will naturally scroll with our scrolling
This comment is really great.

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:255: // The hierarchy of layers that
is maintained by the RenderLayerBacking looks like this:
This is *awesome*.  Thank you!  This makes things so much more clear.

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1162:
rebuildCompositingLayerTree(curLayer, normalFlowChildList, visited, depth + 1);
Could you just have all layers go through
rebuildCompositingLayerTreeForLayerAndScrollParents and keep the original order
of neg, normal, pos order lists rather than doing this dance of doing the normal
flow first, calling a different function, and saving off a vector?

[Ian Vollick, 2013-09-12 03:17:52]

+eseidel for public/platform

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2080: // Now, if we reside in a normal
flow list, then we will naturally scroll with our scrolling
On 2013/09/11 23:18:30, enne wrote:
> This comment is really great.

Thanks :)

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:255: // The hierarchy of layers that
is maintained by the RenderLayerBacking looks like this:
On 2013/09/11 23:18:30, enne wrote:
> This is *awesome*.  Thank you!  This makes things so much more clear.

YW! I've made a small fix to the diagram for the background layer (it's a
sibling of the frame scroll, not the root content layer).

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23903012/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1162:
rebuildCompositingLayerTree(curLayer, normalFlowChildList, visited, depth + 1);
On 2013/09/11 23:18:30, enne wrote:
> Could you just have all layers go through
> rebuildCompositingLayerTreeForLayerAndScrollParents and keep the original
order
> of neg, normal, pos order lists rather than doing this dance of doing the
normal
> flow first, calling a different function, and saving off a vector?
sgtm. Done.

[eseidel, 2013-09-12 03:24:40]

re: public/api review: Jamesr or abarth are much more versed in the WebKit API
conventions than I am.

[jamesr, 2013-09-12 19:26:31]

public/ lgtm, but it'd be even better if you landed the chromium implementation
of these functions first and landed them pure virtual to start instead of having
to change them to be pure virtual in a follow-up

[Ian Vollick, 2013-09-12 19:36:38]

On 2013/09/12 19:26:31, jamesr wrote:
> public/ lgtm, but it'd be even better if you landed the chromium
implementation
> of these functions first and landed them pure virtual to start instead of
having
> to change them to be pure virtual in a follow-up

Looks like the chromium side has landed. I've changed these to be pure virtual.

[commit-bot: I haz the power, 2013-09-12 19:37:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/23903012/59002

[commit-bot: I haz the power, 2013-09-12 22:56:26]

Change committed as 157710