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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> 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 134627: Patch
https://bugs.webkit.org/attachment.cgi?id=134627&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134627&action=review


Sorry for the long delay in getting back to this.  Some high-level comments:

Syncing offsets back into WebCore is something to consider, but I don't think
we need to do it here and I honestly don't think it'll be that difficult. 
We're trying to get back to some scrollable thing.  The GraphicsLayerClient
implementations are:

- NonCompositedContentHost - scrolls via WebViewImpl
- RenderLayerCompositor - associated with a FrameView, which is a
ScrollableArea
- RenderLayerBacking - associated with a RenderLayer, which is a ScrollableArea


but we can deal with that later.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:614
> +static LayerChromium* findLayerById(LayerChromium* layer, int id)

why does this function skip mask/replica layers? that may make sense in the
context of this caller but it's not clear from the name

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:373
> +void
CCLayerTreeHostImpl::calculateVisibleLayers(CCLayerTreeHostImpl::FrameData&
frame, CCLayerGeometryList& visibleLayers)

I'm not sure I understand why this intermediate representation is useful

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:520
> +    if (m_currentlyScrollingLayerImpl) {
> +	   if (!m_rootLayerImpl ||
!m_rootLayerImpl->findLayerInSubtree(m_currentlyScrollingLayerImpl->id()))

webkit style would prefer a few early returns here instead of nested if()s. the
second is a bit hard to read and the extra nesting level isn't helping much

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:721
> +    for (CCLayerImpl* layerImpl = m_currentlyScrollingLayerImpl; layerImpl
&& !pendingDelta.isZero(); layerImpl = layerImpl->parent()) {

For wheel scrolls we need to stop the scroll as soon as we move the target
layer at all.  IOW, if we get a scrollBy(50) and the scroll target has 30px
left of scrollable area, then the desired behavior is to scroll that layer by
30px and do nothing else.  This is so you can scroll content to the end of a
scrollable area without moving it.

Touch is definitely different, so we need to map the scroll type into this
function somehow.  Perhaps we could track the scroll type between the
scrollBegin / scrollEnd calls?	The sequence for wheel is always a
scrollBegin(), exactly one scrollBy(), then a scrollEnd().

we should also have test coverage for this (currently since only the root is
scrollable there's no way to scroll past the end, so it hasn't come up)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:207
> +    // List of visible layers in the most recent frame in front-to-back
order.

I don't understand why we are keeping this list instead of retaining the last
frame's CCLayerList, since we have to calculate the latter anyway.  Why not
just keep that and iterate front-to-back?