Bug 82106 - [chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found.
Summary: [chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: W. James MacLean
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 17:39 PDT by W. James MacLean
Modified: 2012-03-24 15:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2012-03-23 17:43 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (3.77 KB, patch)
2012-03-23 18:06 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description W. James MacLean 2012-03-23 17:39:08 PDT
[chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found.
Comment 1 W. James MacLean 2012-03-23 17:43:09 PDT
Created attachment 133597 [details]
Patch
Comment 2 Nat Duca 2012-03-23 17:56:59 PDT
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
Comment 3 W. James MacLean 2012-03-23 18:06:00 PDT
Created attachment 133604 [details]
Patch
Comment 4 W. James MacLean 2012-03-23 18:06:47 PDT
Removed extraneous FIXME.
Comment 5 Adrienne Walker 2012-03-23 18:25:38 PDT
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.
Comment 6 James Robinson 2012-03-23 19:06:05 PDT
Touchpad flings should be generating wheel scrolls. This seems wrong. What sort of input is causing you problems?
Comment 7 WebKit Review Bot 2012-03-23 19:24:04 PDT
Comment on attachment 133604 [details]
Patch

Clearing flags on attachment: 133604

Committed r111971: <http://trac.webkit.org/changeset/111971>
Comment 8 WebKit Review Bot 2012-03-23 19:24:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 W. James MacLean 2012-03-23 19:33:09 PDT
(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.
Comment 10 James Robinson 2012-03-23 21:19:17 PDT
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.
Comment 11 Nat Duca 2012-03-23 21:21:04 PDT
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...
Comment 12 James Robinson 2012-03-23 21:40:31 PDT
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.
Comment 13 W. James MacLean 2012-03-24 04:57:45 PDT
(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.
Comment 14 W. James MacLean 2012-03-24 15:06:08 PDT
(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