[webkit-reviews] review granted: [Bug 132535] Top content inset: Margin tiles should not display in the inset area when pinned to the top of the page : [Attachment 230768] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 132535: Top content inset: Margin tiles should not display in the inset
area when pinned to the top of the page
https://bugs.webkit.org/show_bug.cgi?id=132535

Attachment 230768: Patch
https://bugs.webkit.org/attachment.cgi?id=230768&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230768&action=review


r+ but it would be nice to try to share more of the layer position math, and
fix UI-side compositing (at least encode/decode the new values).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:223
> +		   positionForInsetClipLayer = FloatPoint();

No need, it's already 0,0

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:226
> +		   positionForInsetClipLayer = FloatPoint(0, topContentInset -
scrollY);

positionForInsetClipLayer.setY(...)?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:235
> +		   if (insetClipLayer)
> +		       insetClipLayer->setPosition(positionForInsetClipLayer);

Why did we spend all that time computing positionForInsetClipLayer when we
don't have one :(

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:128
> +    // This is a clipping layer that will scroll with the page for all
y-delta scroll values between 0
> +    // and topContentInset(). Once the y-deltas get beyond the content inset
point, this layer no longer
> +    // needs to move.

Comment only applies to some configurations.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1603
> +FloatPoint RenderLayerCompositor::positionForClipLayer() const
> +{
> +    float contentInset = m_renderView.frameView().topContentInset();
> +    if (contentInset == 0)
> +	   return FloatPoint();
> +
> +    int scrollY = std::max(0,
m_renderView.frameView().scrollPosition().y());
> +
> +    if (scrollY >= contentInset)
> +	   return FloatPoint();
> +
> +    return FloatPoint(0, contentInset - scrollY);
> +}
> +
> +float RenderLayerCompositor::topContentOffsetForRootLayer() const
> +{
> +    float contentInset = m_renderView.frameView().topContentInset();
> +    if (contentInset == 0)
> +	   return 0;
> +
> +    int scrollY = std::max(0,
m_renderView.frameView().scrollPosition().y());
> +
> +    if (scrollY >= contentInset)
> +	   return contentInset;
> +
> +    return scrollY;
> +}

Doesn't this duplicate code in RenderLayerCompositor and teh scrolling tree
node? Can we factor into a static FrameView function?