Summary: | Handle more Gesture* events by performing scrolls on the correct target ScrollableArea | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Kroeger <rjkroege> | ||||||||||
Component: | UI Events | Assignee: | Robert Kroeger <rjkroege> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | gustavo, jamesr, pnormand, webkit.review.bot, wjmaclean, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 79827, 80201 | ||||||||||||
Bug Blocks: | 67822 | ||||||||||||
Attachments: |
|
Description
Robert Kroeger
2012-03-05 12:07:55 PST
Created attachment 130176 [details]
Patch
Attachment 130176 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/EventHandler.cpp', u'S..." exit_code: 1
Source/WebCore/page/EventHandler.h:340: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.h:341: The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/page/EventHandler.h:341: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/platform/ScrollTypes.h:109: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:250: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:254: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2383: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
Source/WebCore/page/EventHandler.cpp:2382: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2383: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2385: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2386: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2387: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2388: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2389: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2390: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2391: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2392: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2393: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2394: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2395: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2396: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2397: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2397: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:2398: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2398: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:2399: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2400: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2401: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2402: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2403: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2404: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2430: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2431: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2437: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/rendering/RenderBox.cpp:677: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/platform/ScrollableArea.cpp:77: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 36 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130176 [details] Patch Attachment 130176 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11817155 Comment on attachment 130176 [details] Patch Attachment 130176 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11819133 Comment on attachment 130176 [details] Patch Attachment 130176 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11808756 Created attachment 130743 [details]
Patch
jamesr@: we've discussed the previous rev of this CL. I think that this approach is more in keeping with what we discussed. Could you have a look and give me some comments on approach. I would like to think that this code is largely complete -- it works when combined with 79827 -- but I would not consider the patch committable until I add some additional tests. Comment on attachment 130743 [details]
Patch
The additional statefulness in EventHandler feels a bit weird. I see you have a FIXME for it. Perhaps fixing it would be more effort/churn than it's worth at this point, but it's worth looking in to splitting the actual scrolling logic out of handleWheelEvent() so the rest of the world doesn't have to fake out event types and call into that function.
Otherwise at a high level this looks fine.
(In reply to comment #8) > (From update of attachment 130743 [details]) > The additional statefulness in EventHandler feels a bit weird. I see you have a FIXME for it. Perhaps fixing it would be more effort/churn than it's worth at this point, but it's worth looking in to splitting the actual scrolling logic out of handleWheelEvent() so the rest of the world doesn't have to fake out event types and call into that function. I think we are in agreement with there's something "unclean" with the existing code (even without this patch I think.) I think the ScrollableArea::scroll() interface is too restrictive to support trackpad or touchscreen scrolling. And the historical solution to this would seem to be to have been to overload PlatformWheelEvent with extra state (e.g. m_phase) that tells the downstream recipient how to scroll. This could probably stand to be fixed: there could be a richer "scroll" interface that tells the ScrollableArea how to scroll. I don't really want to do this in this patch. I'm not even sure how it should be done. I added https://bugs.webkit.org/show_bug.cgi?id=80596 to track this concept. I think that overloading PlatformWheelEvent would make this patch cleaner and eliminate the additional state in EventHandler. > > Otherwise at a high level this looks fine. Excellent. I'll clean it up further. Created attachment 131095 [details]
Patch
jamesr@: perhaps you could take a look please? Comment on attachment 131095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131095&action=review I think this'll work fine for now. Left a number of nits to deal with. > Source/WebCore/page/EventHandler.cpp:2370 > + TemporaryChange<PlatformEvent::Type> baseEventType(m_baseEventType, gestureEvent.type()); can you put a FIXME here linked to the ::scroll() bug, if that is what'll resolve this? > Source/WebCore/page/EventHandler.cpp:2417 > + PlatformWheelEvent syntheticWheelEvent(point, globalPoint, gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / tickDivisor, gestureEvent.deltaY() / tickDivisor, granularity, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey()); WebKit doesn't have a fixed line limit length but I'm finding this one pretty crazy. Would you mind breaking it up into a few lines - maybe one for the point, one for all the deltas, one for granularity, and one for all the modifier keys? > Source/WebCore/platform/ScrollAnimator.cpp:102 > + scroll(VerticalScrollbar, ScrollByPixelVelocity, 0, -deltaY); this is indented too far, should be 4 spaces > Source/WebCore/platform/ScrollAnimatorNone.cpp:382 > + , m_firstVelocity(0.f) this should just be 0 (see http://www.webkit.org/coding/coding-style.html#float-suffixes) > Source/WebCore/platform/ScrollAnimatorNone.cpp:393 > +void ScrollAnimatorNone::fireUpAnAnimation(float x, float y) this might get you an unused variable warning on some platforms (I forget which set the compile flags that generate this). The standard ways to avoid this are to either omit the parameter names, comment them out, or use the UNUSED_PARAM() macro in the function body > Source/WebCore/platform/ScrollAnimatorNone.h:142 > + virtual void fireUpAnAnimation(float, float); should this take a FloatPoint? most things that take an x/y pair do > Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:85 > + float m_f1; > + float m_f2; are these an X/Y pair? if so can they be a FloatPoint or FloatSize? That will also give you zero-initialization for free > Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:145 > + scrollAnimatorNone.scroll(HorizontalScrollbar, ScrollByPixelVelocity, 111.f, -42.f); > + scrollAnimatorNone.scroll(VerticalScrollbar, ScrollByPixelVelocity, 222.f, 42.f); drop the .f's here and in the rest of the test files Comment on attachment 131095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131095&action=review >> Source/WebCore/page/EventHandler.cpp:2370 >> + TemporaryChange<PlatformEvent::Type> baseEventType(m_baseEventType, gestureEvent.type()); > > can you put a FIXME here linked to the ::scroll() bug, if that is what'll resolve this? yes. done. >> Source/WebCore/page/EventHandler.cpp:2417 >> + PlatformWheelEvent syntheticWheelEvent(point, globalPoint, gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / tickDivisor, gestureEvent.deltaY() / tickDivisor, granularity, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey()); > > WebKit doesn't have a fixed line limit length but I'm finding this one pretty crazy. Would you mind breaking it up into a few lines - maybe one for the point, one for all the deltas, one for granularity, and one for all the modifier keys? thank you. I have suffered through this since the first time I got told in review to un-break it. >> Source/WebCore/platform/ScrollAnimator.cpp:102 >> + scroll(VerticalScrollbar, ScrollByPixelVelocity, 0, -deltaY); > > this is indented too far, should be 4 spaces oops. done, next patch. >> Source/WebCore/platform/ScrollAnimatorNone.cpp:382 >> + , m_firstVelocity(0.f) > > this should just be 0 (see http://www.webkit.org/coding/coding-style.html#float-suffixes) done. >> Source/WebCore/platform/ScrollAnimatorNone.cpp:393 >> +void ScrollAnimatorNone::fireUpAnAnimation(float x, float y) > > this might get you an unused variable warning on some platforms (I forget which set the compile flags that generate this). The standard ways to avoid this are to either omit the parameter names, comment them out, or use the UNUSED_PARAM() macro in the function body thanks. done next patch. >> Source/WebCore/platform/ScrollAnimatorNone.h:142 >> + virtual void fireUpAnAnimation(float, float); > > should this take a FloatPoint? most things that take an x/y pair do done, next patch. >> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:85 >> + float m_f2; > > are these an X/Y pair? if so can they be a FloatPoint or FloatSize? That will also give you zero-initialization for free cool. done. >> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:145 >> + scrollAnimatorNone.scroll(VerticalScrollbar, ScrollByPixelVelocity, 222.f, 42.f); > > drop the .f's here and in the rest of the test files done, next patch. Created attachment 131134 [details]
Patch
Comment on attachment 131134 [details] Patch Clearing flags on attachment: 131134 Committed r110371: <http://trac.webkit.org/changeset/110371> All reviewed patches have been landed. Closing bug. (In reply to comment #16) > All reviewed patches have been landed. Closing bug. Looks like this patch broke a GTK layout test: platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r110371%20(19734)/results.html (In reply to comment #17) > (In reply to comment #16) > > All reviewed patches have been landed. Closing bug. > > Looks like this patch broke a GTK layout test: platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html > > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r110371%20(19734)/results.html See bug 80825 Now that I know more about how this system works I'm thinking this patch is wrong and at least some of it needs to be reverted: *) For wheel interactions, we need to hit test + scroll on every intermediate point, not just the beginning of the interaction. Given that, there's no point in distinguishing a begin for a scroll type that's really a wheel. That said, for gestures that really are touch interactions (like finger-dragging on a touchpad) you definitely do want to hit test only on the start of the event and retain knowledge of the hit target since that is the hit target *) Overriding GestureEnd to mean fling is an ugly ugly hack and not really compatible with what android does. We have a separate gesture type for fling start and fling cancel in the chromium WebKit API - GestureFlingStart / GestureFlingCancel. We should use those. I'll take a closer look tomorrow with a fresh set of eyes and see what should be done. (In reply to comment #19) > Now that I know more about how this system works I'm thinking this patch is wrong and at least some of it needs to be reverted: > > *) For wheel interactions, we need to hit test + scroll on every intermediate point, not just the beginning of the interaction. Given that, there's no point in distinguishing a begin for a scroll type that's really a wheel. That said, for gestures that really are touch interactions (like finger-dragging on a touchpad) you definitely do want to hit test only on the start of the event and retain knowledge of the hit target since that is the hit target > > *) Overriding GestureEnd to mean fling is an ugly ugly hack and not really compatible with what android does. We have a separate gesture type for fling start and fling cancel in the chromium WebKit API - GestureFlingStart / GestureFlingCancel. We should use those. > > I'll take a closer look tomorrow with a fresh set of eyes and see what should be done. I think we discussed. This bug is obsolete. I will clean up the code. Cleanup sufficient to close the bug is in http://trac.webkit.org/changeset/135638 |