Bug 73345 – [chromium] Split scrollRootLayer() into scroll{Begin,By,End}()

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Sami Kyostila]

CCInputHandler::scrollRootLayer() should be split into scroll{Begin,By,End}() to allow the compositor thread to either process scroll events or delegate them to the main thread based on hit testing against scrollable layers.

[Sami Kyostila]

Created attachment 117015 [details]
Patch

[Adrienne Walker]

Maybe I'm just missing some context here, but I don't follow what isScrolling() or scrollEnd() are intended for.

[James Robinson]

These need some documentation in the header at least.

[Sami Kyostila]

Created attachment 117183 [details]
Patch

- Removed isScrolling().
- Documented new functions in CCInputHandler.h.

[Adrienne Walker]

(In reply to comment #2)
> Maybe I'm just missing some context here, but I don't follow what isScrolling() or scrollEnd() are intended for.

I had clarification off-thread, and these make a lot more sense in the context of gestures creating multiple input events and needing to track which layer is the current scrolling target.  I was just missing some context, sorry.  :)

[James Robinson]

Comment on attachment 117183 [details]
Patch

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.

[Sami Kyostila]

(In reply to comment #6)
> (From update of attachment 117183 [details])
> 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?

Yes, this is an important aspect of scrolling and the API is meant to support it. The comment here is misleading in suggesting that scrolling only affects the the selected layer. I'll update it to be more accurate.

> 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?

Great point. I hadn't considered this. I don't think we can simply start sending events to the main thread in this case, because the main thread also needs to know which element the events should be directed at. The reason is that with gesture scroll events the scrolled element should be determined by the initial touch coordinates instead the most recent ones.

When we detect this situation, it's already too late to perform hit testing to find the scrolling element in the main thread, since it might have been scrolled offscreen or there might an entirely unrelated element under the touch point.

I think one way to implement this would be to also inform the main thread about the scroll scrollBegin/End transaction. Then we could send the residual scroll deltas to the main thread, which would apply them to the chosen element instead of looking at the event coordinates.

While this should work, I'm not sure if it's worth the added complexity given that the situation should be relatively rare. Perhaps it would be simpler just to refuse scrolling a layer in the impl thread if any of its ancestor layers or elements need to be scrolled in the main thread? I don't think this situation should arise very often with common sites.

> > 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.

Right. I chose to fix this by returning status code from scrollBegin() to avoid the need to do multiple hit tests.

[Sami Kyostila]

Created attachment 117421 [details]
Patch

- scrollBegin() now returns one of {ScrollStarted, ScrollIgnored, ScrollFailed}, allowing us to drop ignored scroll events.
- Improved header comments.

[James Robinson]

This patch has merge conflicts compared to ToT, would you mind rebaselining it?

[James Robinson]

Comment on attachment 117421 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117421&action=review

This looks great. R=me but I can't set cq+ since this patch has merge conflicts.

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:63
> +    // Stop scrolling the layer selected with scrollBegin().

It appears from the logic that you only need to call this when scrollBegin() returns ScrollStarted, which makes sense to me. Could you document this requirement in the header?

[Sami Kyostila]

Created attachment 117600 [details]
Patch

Thanks James! Here's a rebased patch with an updated comment about when scrollEnd() and scrollBy() should be called.

[Steve Block]

Comment on attachment 117600 [details]
Patch

r=me for rebase after jamesr's r+

[WebKit Review Bot]

Comment on attachment 117600 [details]
Patch

Clearing flags on attachment: 117600

Committed r101785: <http://trac.webkit.org/changeset/101785>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Comment on attachment 117600 [details]
Patch

Awesome, thanks for taking care of that Steve.