[webkit-reviews] review denied: [Bug 73345] [chromium] Split scrollRootLayer() into scroll{Begin, By, End}() : [Attachment 117183] 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 73345: [chromium] Split scrollRootLayer() into scroll{Begin,By,End}()
https://bugs.webkit.org/show_bug.cgi?id=73345

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

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


Comments inline. I like the shape of this, just have some questions about the
details.

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:55
> +    // Scroll the layer selected with scrollBegin().
> +    virtual void scrollBy(const IntSize&) = 0;

If the layer being scrolled runs out of scrollable area, then in at least some
UIs we start scrolling the nearest ancestor that can accept the scrolls (even
if it isn't the same scroll). I believe this API is compatible with this
interaction but this comment seems to indicate that all scrolls will have to be
on the same layer. Can you update the comment for this case?

What happens if we run out of scrollable area on the initially targeted layer
and we need to start scrolling some ancestor that we can't scroll on the
thread? It seems like in that case we need to start punting the scrolls to the
main thread - or is that considered a separate scrollBegin/scrollBy/scrollEnd
interaction at that point?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:122
> +	   if (m_inputHandlerClient->scrollBegin(IntPoint(wheelEvent.x,
wheelEvent.y))) {

I think this logic is subtly wrong in the case where the main thread has no
wheel event handlers and the root content is not scrollable - in that case we
should drop the wheel on the floor, not send it to the main thread.

I think you need to distinguish between the "input handler cannot handle the
scroll" and "input handler knows that nobody cares about this scroll" cases to
handle this, either by having separate APIs to query whether the handler can do
a scroll vs whether there's anything to scroll at all or by having multiple
return values from this call.