Bug 82106 – [chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

[chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found.

[W. James MacLean]

Created attachment 133597 [details]
Patch

[Nat Duca]

Comment on attachment 133597 [details]
Patch

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

Nice catch. LGTM.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:338
>          // For now, just abort the fling.
> +        // FIXME: for now, should this write a warning to a log file?

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

[W. James MacLean]

Created attachment 133604 [details]
Patch

[W. James MacLean]

Removed extraneous FIXME.

[Adrienne Walker]

Comment on attachment 133604 [details]
Patch

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

Thanks for the quick fix.  R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:596
> -    if (type == CCInputHandlerClient::Wheel && m_scrollLayerImpl->haveWheelEventHandlers()) {
> +    if ((type == CCInputHandlerClient::Wheel || type == CCInputHandlerClient::Gesture) && m_scrollLayerImpl->haveWheelEventHandlers()) {

I think this is not going to be true once when we have touch flings, but we can burn that bridge when we come to it.

[James Robinson]

Touchpad flings should be generating wheel scrolls. This seems wrong. What sort of input is causing you problems?

[WebKit Review Bot]

Comment on attachment 133604 [details]
Patch

Clearing flags on attachment: 133604

Committed r111971: <http://trac.webkit.org/changeset/111971>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[W. James MacLean]

(In reply to comment #6)
> Touchpad flings should be generating wheel scrolls. This seems wrong. What sort of input is causing you problems?

Try fling scrolling on a page with wheel handlers (e.g. vimeo.org on a page with a video).

The fling is started, but then on the first wheel event it fails, but by now there's no way to punt back to the main thread, so the scroll gets aborted. This will change eventually, but for now it seems better not to start a fling scroll that is destined to fail immediately.

[James Robinson]

In that case GestureFlingStart should be using type wheel instead of gesture if the gesture is intended to produce a wheel fling. This patch is the wrong way to patch the bug.

[Nat Duca]

James, I'm still up working on what I understand to be the real patch that you handed off to me. What do you want to do in the interrim ~half day? This was pushed in to hit canary, but I think tomorrow's canary will pick up the proper solution...

[James Robinson]

We absolutely need the transfer stuff regardless, that's a separate issue.

I think the better way to address this particular issue is to pass the right value to scrollBegin, not to distrust the value in the implementation. We know if a gestureFlingStart means wheel or not (currently it always does) so we should take advantage of that.

[W. James MacLean]

(In reply to comment #10)
> In that case GestureFlingStart should be using type wheel instead of gesture if the gesture is intended to produce a wheel fling. This patch is the wrong way to patch the bug.

I'm OK with this idea for sure ... I'd be happy to upload a patch to switch to this solution, but I can't do it before 2pm PST today. If that's OK just leave it and I'll do it.

[W. James MacLean]

(In reply to comment #13)
> (In reply to comment #10)
> > In that case GestureFlingStart should be using type wheel instead of gesture if the gesture is intended to produce a wheel fling. This patch is the wrong way to patch the bug.
> 
> I'm OK with this idea for sure ... I'd be happy to upload a patch to switch to this solution, but I can't do it before 2pm PST today. If that's OK just leave it and I'll do it.

Patch submitted at https://bugs.webkit.org/show_bug.cgi?id=82133