Bug 73345

Summary: [chromium] Split scrollRootLayer() into scroll{Begin,By,End}()
Product: WebKit Reporter: Sami Kyostila <skyostil>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, jamesr, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
jamesr: review-
Patch
jamesr: review+, jamesr: commit-queue-
Patch none

Sami Kyostila
Reported 2011-11-29 12:01:14 PST
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.
Attachments
Patch (10.58 KB, patch)
2011-11-29 12:06 PST, Sami Kyostila
no flags
Patch (8.93 KB, patch)
2011-11-30 06:17 PST, Sami Kyostila
jamesr: review-
Patch (9.68 KB, patch)
2011-12-01 07:49 PST, Sami Kyostila
jamesr: review+
jamesr: commit-queue-
Patch (9.86 KB, patch)
2011-12-02 03:27 PST, Sami Kyostila
no flags
Sami Kyostila
Comment 1 2011-11-29 12:06:16 PST
Adrienne Walker
Comment 2 2011-11-29 13:37:47 PST
Maybe I'm just missing some context here, but I don't follow what isScrolling() or scrollEnd() are intended for.
James Robinson
Comment 3 2011-11-29 16:25:52 PST
These need some documentation in the header at least.
Sami Kyostila
Comment 4 2011-11-30 06:17:28 PST
Created attachment 117183 [details] Patch - Removed isScrolling(). - Documented new functions in CCInputHandler.h.
Adrienne Walker
Comment 5 2011-11-30 12:42:52 PST
(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 6 2011-11-30 16:40:17 PST
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
Comment 7 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.
Sami Kyostila
Comment 8 2011-12-01 07:49:57 PST
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
Comment 9 2011-12-01 11:20:05 PST
This patch has merge conflicts compared to ToT, would you mind rebaselining it?
James Robinson
Comment 10 2011-12-01 11:23:04 PST
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
Comment 11 2011-12-02 03:27:32 PST
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 12 2011-12-02 03:41:28 PST
Comment on attachment 117600 [details] Patch r=me for rebase after jamesr's r+
WebKit Review Bot
Comment 13 2011-12-02 04:35:00 PST
Comment on attachment 117600 [details] Patch Clearing flags on attachment: 117600 Committed r101785: <http://trac.webkit.org/changeset/101785>
WebKit Review Bot
Comment 14 2011-12-02 04:35:05 PST
All reviewed patches have been landed. Closing bug.
James Robinson
Comment 15 2011-12-02 11:41:22 PST
Comment on attachment 117600 [details] Patch Awesome, thanks for taking care of that Steve.
Note You need to log in before you can comment on or make changes to this bug.