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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73345





--- Comment #7 from Sami Kyostila <skyostil at google.com>  2011-12-01 07:46:44 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.