[webkit-reviews] review denied: [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread : [Attachment 134303] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Sami Kyostila
<skyostil at google.com>'s request for review:
Bug 73350: [chromium] Allow scrolling non-root layers in the compositor thread
https://bugs.webkit.org/show_bug.cgi?id=73350

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

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=134303&action=review


Can you walk me through the data flow here? Scrolling a layer sets its scroll
delta.	The scroll delta is used temporarily on the impl thread to affect the
position of a layer.  It's then synced back and merged into the scroll position
on the main thread which...does what? For the root layer, we call out to
FrameView, scroll the frame, and then update layer positions.  I think you're
going to have to do the same thing here--scrolling a non-root layer on the main
thread actually adjusts the layer position (GraphicsLayer::position).  You're
also going to have to figure out how to get the correct maximum scroll for
non-root layers and set layers as properly being scrollable.

Also, can you hook this up more so that some non-example page can be tested
locally? I feel like that would help to clear this up immensely.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:147
> +void ContentLayerChromium::scrollBy(const IntSize& scrollDelta)

Only content layers get to scroll?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> +    if (m_delegate)
> +	   m_delegate->didScroll(scrollDelta);

This delegate needs to do something real.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:389
> +void LayerChromium::setMaxScrollPosition(const IntSize& maxScrollPosition)
> +{
> +    if (m_maxScrollPosition == maxScrollPosition)
> +	   return;
> +    m_maxScrollPosition = maxScrollPosition;
> +    setNeedsCommit();
> +}

You need to set this from somewhere other than tests.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:675
> +	   if (layerImpl->tryScroll(viewportPoint, type) == ScrollFailed &&
false)

...?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:686
> +	   // If any layer wants to divert the scroll event to the main thread,
abort.
> +	   if (status == ScrollFailed)
> +	       return ScrollFailed;

I don't follow this.  If I have a scrollable child layer contained within a
non-fast scrollable parent layer, this code appears to not scroll the child
layer, because the parent layer will bail out? Is this because you don't want
to partially scroll one layer and then have to transfer it to a non-fast layer?