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

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


Thanks for adding all the new tests.  I think we need a little more refinement
on this before landing, but it's not in bad shape.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:49
> +    virtual void didScroll(const IntSize&) = 0;

After having more time to think about it, I really think that scrolling should
be a separate delegate type and hang off of the base LayerChromium class (and
be optional).  Even in this patch you can see that you had to modify a number
of ContentLayerDelegate implementations that aren't scrollable at all.	It's
also a bit wrong that this patch only lets us scroll content layers - other
types (like plugins) definitely should be scrollable as well.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:70
> +    virtual void scrollBy(const IntSize&) OVERRIDE;

if you move the scroll delegate up to LayerChromium, this should move up as
well (and become non-virtual)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:190
> +	   TRACE_EVENT("tryScroll Failed shouldScrollOnMainThread", this, 0);

please use:

TRACE_EVENT0("cc", "....");

style trace macros here and everywhere else inside cc so we get a useful
component

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:397
> +    m_mostRecentRenderSurfaceLayerList = frame.renderSurfaceLayerList;

Hmmm, a deep copy on every pass?  If we're making the renderSurfaceLayerList
something persistent instead of something frame-transient, can we just generate
the list into a member variable of CCLayerTreeHostImpl in the first place,
instead of pretending it's part of FrameData (since it's no longer isolated to
being used only on this frame)?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
> +PassOwnPtr<CCLayerImpl> CCLayerTreeHostImpl::releaseRootLayer()
> +{
> +    m_scrollingLayerIdFromPreviousTree = m_currentlyScrollingLayerImpl ?
m_currentlyScrollingLayerImpl->id() : -1;
> +    m_currentlyScrollingLayerImpl = 0;

I think it's ugly to make this seemingly normal getter have strange side
effects like this.  I assume you're doing this because currently the only
caller to releaseRootLayer() is in the TreeSynchronizer call, right? I think
it'd be much better to have an explicit call to reset state like this or to
rename this function to make it clearer what is going on.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:714
> +    if (m_mostRecentRenderSurfaceLayerList.size() &&
m_rootLayerImpl->renderSurface())

I'm not sure I understand both of the checks here - why are we checking both? 
If we haven't calculated a renderSurfaceLayerList I can understand that we need
to calculate one, but what does the rootLayerImpl having a render surface have
to do with it?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:739
> +    // First find out which layer was hit by walking the saved list of
visible layers from the most recent frame.
> +    CCLayerImpl* layerImpl = 0;

this looks like a generic hit testing function - could you extract it out into
one and provide dedicated tests, etc?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:745
> +    for (CCLayerIteratorType it =
CCLayerIteratorType::begin(&m_mostRecentRenderSurfaceLayerList); it != end;
++it) {
> +	   CCLayerImpl* renderSurfaceLayerImpl = (*it);

I think we should only care about the case where it.representsItself() == true,
right? (see comments at the top of CCLayerIterator.h to see what this means).