| Summary: | [chromium] CCLayerTreeHostImpl::scrollBegin() should return ScrollFailed for CCInputHandlerClient::Gesture type when wheel handlers found. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | W. James MacLean <wjmaclean> | ||||||
| Component: | New Bugs | Assignee: | W. James MacLean <wjmaclean> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | backer, cc-bugs, jamesr, nduca, rjkroege, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
W. James MacLean
2012-03-23 17:39:08 PDT
Created attachment 133597 [details]
Patch
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 Created attachment 133604 [details]
Patch
Removed extraneous FIXME. 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. Touchpad flings should be generating wheel scrolls. This seems wrong. What sort of input is causing you problems? Comment on attachment 133604 [details] Patch Clearing flags on attachment: 133604 Committed r111971: <http://trac.webkit.org/changeset/111971> All reviewed patches have been landed. Closing bug. (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. 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. 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... 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. (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. (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 |