Summary: | [chromium] Split scrollRootLayer() into scroll{Begin,By,End}() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sami Kyostila <skyostil> | ||||||||||
Component: | UI Events | Assignee: | 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
Sami Kyostila
2011-11-29 12:01:14 PST
Created attachment 117015 [details]
Patch
Maybe I'm just missing some context here, but I don't follow what isScrolling() or scrollEnd() are intended for. These need some documentation in the header at least. Created attachment 117183 [details]
Patch
- Removed isScrolling().
- Documented new functions in CCInputHandler.h.
(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. :) 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. (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. Created attachment 117421 [details]
Patch
- scrollBegin() now returns one of {ScrollStarted, ScrollIgnored, ScrollFailed}, allowing us to drop ignored scroll events.
- Improved header comments.
This patch has merge conflicts compared to ToT, would you mind rebaselining it? 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? Created attachment 117600 [details]
Patch
Thanks James! Here's a rebased patch with an updated comment about when scrollEnd() and scrollBy() should be called.
Comment on attachment 117600 [details]
Patch
r=me for rebase after jamesr's r+
Comment on attachment 117600 [details] Patch Clearing flags on attachment: 117600 Committed r101785: <http://trac.webkit.org/changeset/101785> All reviewed patches have been landed. Closing bug. Comment on attachment 117600 [details]
Patch
Awesome, thanks for taking care of that Steve.
|