Issue 22419002: Set up clip and scroll parents on the blink side.

Issue 22419002: Set up clip and scroll parents on the blink side. (Closed)

Created:
7 years, 4 months ago by hartmanng

Modified:
7 years, 3 months ago

Reviewers:
enne (OOO)

CC:
blink-reviews, kenneth.christiansen, jamesr, eae+blinkwatch, leviw+renderwatch, abarth-chromium, danakj, dglazkov+blink, Rik, jchaffraix+rendering, Stephen Chennney, jeez, pdr.

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

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 and was written by vollick@chromium.org. BUG=249349, 249357

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+681 lines, -61 lines)			Patch
A	LayoutTests/compositing/overflow/ancestor-scroll-clipping-layer.html	View	1 2 3 4 5 6 7	1 chunk	+101 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/ancestor-scroll-clipping-layer-expected.txt	View	1 2 3 4 5 6 7	1 chunk	+284 lines, -0 lines	0 comments	Download
M	Source/core/page/scrolling/ScrollingCoordinator.h	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/page/scrolling/ScrollingCoordinator.cpp	View	1 2 3 4 5 6	1 chunk	+32 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderLayer.h	View	1 2 3 4 5 6 7	3 chunks	+5 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayer.cpp	View	1 2 3 4 5 6 7	3 chunks	+56 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayerBacking.h	View	1 2 3 4 5 6 7	5 chunks	+8 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderLayerBacking.cpp	View	1 2 3 4 5 6 7	12 chunks	+91 lines, -3 lines	0 comments	Download
M	Source/core/rendering/RenderLayerCompositor.h	View	1 2 3 4 5 6 7	1 chunk	+8 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderLayerCompositor.cpp	View	1 2 3 4 5 6 7	10 chunks	+83 lines, -50 lines	0 comments	Download
M	public/platform/WebLayer.h	View	1 2 3 4	1 chunk	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 2 (0 generated)

Expand Messages | Collapse Messages

enne (OOO)

This is probably just my own lack of understanding about RenderLayer and RenderLayerBacking, but RenderLayerBacking::updateGraphicsLayerGeometry() ...

7 years, 4 months ago (2013-08-22 20:54:30 UTC) #1

Ian Vollick

7 years, 3 months ago (2013-09-05 03:01:57 UTC) #2

On 2013/08/22 20:54:30, enne wrote:
> This is probably just my own lack of understanding about RenderLayer and
> RenderLayerBacking, but RenderLayerBacking::updateGraphicsLayerGeometry() is
> quite dense and hard to follow.  There's a RenderLayer, GraphicsLayer, clip
> parent, and scroll parent hierarchy to understand here all with different
spaces
> and optional layers.
> 
> Could I get some diagrams?
> 
> I have a bunch of smaller comments that I made as I was trying to digest this,
> but I didn't quite get to the impenetrable geometry updating core.
> 
>
https://codereview.chromium.org/22419002/diff/9001/Source/core/page/scrolling...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/9001/Source/core/page/scrolling...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:425:
> TRACE_EVENT_INSTANT2("comp-scroll2",
> "ScrollingCoordinator::updateScrollParentForLayer",
> Would this trace event be better on the cc side where you could only do it
when
> the scroll parent changed? Same question for a bunch of other places.
> 
> Also, comp-scroll2??
> 
>
https://codereview.chromium.org/22419002/diff/9001/Source/core/page/scrolling...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:427: "ancestorId",
> scrollParentWebLayer ? scrollParentWebLayer->id() : -1);
> Ids start at 1, so you could say 0.
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/page/scrollin...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/page/scrollin...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:502: if (!child ||
> !child->backing())
> Shouldn't these always be true?
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/platform/grap...
> File Source/core/platform/graphics/GraphicsLayer.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/platform/grap...
> Source/core/platform/graphics/GraphicsLayer.cpp:362:
> TRACE_EVENT_INSTANT2("core,painting",
> "GraphicsLayer::paintGraphicsLayerContents",
> I think we already are tracing this from the compositor side.
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:4971: // we may ignor the cilp here. If
> we've been told to ignore the overflow clip (via
> I won't ignor this cilp.
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerBacking.cpp:590: scrollToCompAncestorOffset =
> -compToScrollAncestorOffset;
> Is this the equivalent of the "scroll buddy" scroll delta adjustment for the
> clip layer? (Just trying to understand.)
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerBacking.cpp:1196: if (RenderObject*
> containingBlock = m_owningLayer->renderer()->containingBlock())
> Can you help me understand the containing block logic here?
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerBacking.cpp:1218: if
> (m_owningLayer->compositingReasons() &
CompositingReasonOverflowScrollingParent)
> Use your shiny new unused m_owningLayer->hasScrollParent helper?
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerCompositor.cpp:1755: RenderLayer*
> compositingAncestor = clippedByScrollingAncestor(layer)
> clippedByScrollingAncestor seems like a bunch of work to repeat in
> clippedByAncestor and then immediately again by the caller of
clippedByAncestor.
>  Can this be simplified?
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayerCompositor.h (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerCompositor.h:108: bool
> clippedByScrollingAncestor(RenderLayer*) const;
> I see.  This is your solution to the ordering issue that you talked about
> previously.  By adding an additional clipping layer that's an ancestor to all
of
> the scroll children you're guaranteed that it comes before them in the tree. 
> Neat!
> 
> https://codereview.chromium.org/22419002/diff/12001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/web/WebViewImpl.cp...
> Source/web/WebViewImpl.cpp:3973: TRACE_EVENT1("impl-scroll",
> Should this go after the early outs?
> 
>
https://codereview.chromium.org/22419002/diff/12001/Source/web/WebViewImpl.cp...
> Source/web/WebViewImpl.cpp:3996: TRACE_EVENT_INSTANT2("webkit,impl-scroll",
> "WebViewImpl::applyScrollAndScale::scrollBy", "x", scrollDelta.width, "y",
> scrollDelta.height);
> Aren't you tracing updateMainFrameScrollPosition too? There's lots of trace
> events here that all seem to be doing the same thing.  I'm not sure what this
is
> gaining, other than really deep redundant traces.
> 
> https://codereview.chromium.org/22419002/diff/12001/public/platform/WebLayer.h
> File public/platform/WebLayer.h (right):
> 
>
https://codereview.chromium.org/22419002/diff/12001/public/platform/WebLayer....
> public/platform/WebLayer.h:166: // delta and offset even though it will not be
a
> descendant of the scroll
> "its parent's" => "its scroll parent's"?
> "of the scroll" => "of the scroll parent"?
> "will not" => "may not"?
> 
> Doesn't it just inherit the scroll parent's scroll delta and assumes that its
> position already includes its scroll parent's offset?

Patch has been drastically simplified and reposted here under my ownership:
https://codereview.chromium.org/23903012/. I see that I've missed a comment here
that still applies to the new patch. I will have an update for that one shortly.
Closing this for now

Expand Messages | Collapse Messages