[webkit-reviews] review denied: [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread : [Attachment 127318] 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 127318: Patch
https://bugs.webkit.org/attachment.cgi?id=127318&action=review

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


I think this looks pretty great.  I think that we might want to tweak the way
we sync changes back from LayerChromium->GraphicsLayer->beyond, but I think we
should move forward with the rest of this code first.

I would suggest that you first break out the Int->Float change for scrollDeltas
into a separate patch, since I think that's pretty separable and would be
generally pretty useful, and take out the GraphicsLayerChromium stuff from this
patch (just leave a stub for didScroll).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:62
> +    bool isLayerInDescendants(int layerId) const;

this doesn't appear to consider mask and reflection layers at all. for
scrolling, i believe that's right, but as a generic function this might
surprised some callers. can you update the function name and/or documentation
(ideally both) describing exactly what this does?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.h:58
> +    IntSize rootScrollDelta;

why does this have to be separate and not simply an entry in 'scrolls'?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:191
> +void CCLayerTreeHostImpl::calculateRenderSurfaces(CCLayerList&
renderSurfaceLayerList)

this looks like a bad merge - did you mean to change this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:478
> +CCInputHandlerClient::ScrollStatus
CCLayerTreeHostImpl::beginScrollingLayer(CCLayerImpl* layerImpl, const
IntPoint& viewportPoint)

it feels like most of this belongs on CCLayerImpl and the host can take care of
setting m_currentlyScrollingLayerImpl if appropriate.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:507
> +    CCLayerList renderSurfaceLayerList;
> +    calculateRenderSurfaces(renderSurfaceLayerList);

yuck yuck! we should definitely not need to recalculate all our transforms, etc
on every scrollBegin. Just use the transforms from the most recent frame.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:513
> +	   // A non-composited content layer should be scrolled via the root
scroll layer.
> +	   if (layerImpl->isNonCompositedContent())

this feels fishy. in general, we need to support scrolling a layer that isn't
the content in order to be able to scroll any FrameViews, not just the NCCH.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:538
> +	       // Since scrollDelta is in window coordinates, it already has
the page scale applied.
> +	       // This matches what the root scroll layer expects, but child
layers are scrolled using
> +	       // unscaled content coordinates instead, so we have to undo the
scaling for them. The
> +	       // page scale delta needs to be unapplied with both layer types
since the scroll
> +	       // coordinates do not respect it.

this is really hard to maintain. it's a bit beyond this patch, but can we
normalize the scales somewhere else (LayerChromium? GraphicsLayerChromium?) and
not have to deal with it throughout the compositor code?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:642
> +    bool didMove = m_scrollLayerImpl &&
(didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1);

we're doing a recursive walk through layers here to check if anything has a
scrollDelta...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:648
> +    collectScrollDeltas(scrollInfo.get(), m_scrollLayerImpl.get());

and then we do another walk here. seems unnecessary - can't we just do one walk
and if we don't have any scroll deltas just send an empty scrollInfo over?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:179
> +    initializeLayerRenderer();

would you mind converting the other CCLayerTreeHostImplTest tests that
initialize the layer renderer to using this helper as well?