Implement flick gesture in Chromium Gesture Recognizer
Created attachment 107040 [details] Patch
Comment on attachment 107040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107040&action=review Good start. Please freshen. > Source/WebCore/page/EventHandler.cpp:2213 > + // FIXME: Stop flick if one is in progress. This code is ultimately going to be dispatching events through what https://bugs.webkit.org/show_bug.cgi?id=67822 evolves into. In particular, implementing this FIXME requires dispatching the TapDownType to the target node/frame iff it has refcount > 1. (But if you wade through the review, you'll see that I have some work ahead on this.) > Source/WebCore/page/EventHandler.cpp:2227 > + case PlatformGestureEvent::FlickType: no. we don't need another type. We had decided that a flick starts on scroll end that has a velocity. You ought not to need a FlickType > Source/WebCore/platform/PlatformGestureEvent.h:44 > + FlickType, no. (I said this before.) > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:149 > + gestures->append(PlatformGestureEvent(PlatformGestureEvent::FlickType, touchPoint.pos(), touchPoint.screenPos(), m_lastTouchTime, 0.f, 0.f, m_shiftKey, m_ctrlKey, m_altKey, m_metaKey)); no. See above. > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:223 > + appendScrollGestureEnd(point, gestures); remove appendFlickGesture, add ScrollEnd with velo. > Source/WebCore/platform/chromium/PopupContainer.cpp:311 > + case PlatformGestureEvent::FlickType: Less new event types make your life easier. > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:472 > +{ More. Adjusted. With the traces for actual event sequences that I emailed out earlier.
Created attachment 107062 [details] Patch
Removed separate FlickType... PTAL (In reply to comment #2) > (From update of attachment 107040 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107040&action=review > > Good start. Please freshen. > > > Source/WebCore/page/EventHandler.cpp:2213 > > + // FIXME: Stop flick if one is in progress. > > This code is ultimately going to be dispatching events through what https://bugs.webkit.org/show_bug.cgi?id=67822 evolves into. In particular, implementing this FIXME requires dispatching the TapDownType to the target node/frame iff it has refcount > 1. (But if you wade through the review, you'll see that I have some work ahead on this.) > > > Source/WebCore/page/EventHandler.cpp:2227 > > + case PlatformGestureEvent::FlickType: > > no. we don't need another type. > > We had decided that a flick starts on scroll end that has a velocity. You ought not to need a FlickType > > > Source/WebCore/platform/PlatformGestureEvent.h:44 > > + FlickType, > > no. (I said this before.) > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:149 > > + gestures->append(PlatformGestureEvent(PlatformGestureEvent::FlickType, touchPoint.pos(), touchPoint.screenPos(), m_lastTouchTime, 0.f, 0.f, m_shiftKey, m_ctrlKey, m_altKey, m_metaKey)); > > no. See above. > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:223 > > + appendScrollGestureEnd(point, gestures); > > remove appendFlickGesture, add ScrollEnd with velo. > > > Source/WebCore/platform/chromium/PopupContainer.cpp:311 > > + case PlatformGestureEvent::FlickType: > > Less new event types make your life easier. > > > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:472 > > +{ > > More. Adjusted. With the traces for actual event sequences that I emailed out earlier.
Comment on attachment 107062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107062&action=review looks good to me. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Not so! You wrote unit tests. And if you leave this in, the commit queue will bounce the CL. > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:40 > +static const float minFlickSpeed = 1000.0; I lowered this to 300.f (and you should have the trailing f to avoid a temporary.)
Created attachment 109088 [details] Patch
(In reply to comment #5) > (From update of attachment 107062 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107062&action=review > > looks good to me. > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > Not so! You wrote unit tests. And if you leave this in, the commit queue will bounce the CL. Done > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:40 > > +static const float minFlickSpeed = 1000.0; > > I lowered this to 300.f (and you should have the trailing f to avoid a temporary.) Done
Comment on attachment 109088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109088&action=review > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:105 > + return sqrt(m_xVelocity * m_xVelocity + m_yVelocity * m_yVelocity) > minFlickSpeed; Whoa, math! > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:616 > + , new TouchPointAndEvent(256, 348, 1308336245.407, PlatformTouchPoint::TouchPressed, WebCore::TouchStart) These commas are freaky. Can me move them to the right places?
Comment on attachment 109088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109088&action=review > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:41 > +static const float minFlickSpeed = 300.f; I think recent on-device experience suggests that this should be higher. > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:105 > + return sqrt(m_xVelocity * m_xVelocity + m_yVelocity * m_yVelocity) > minFlickSpeed; velocities are float. so use sqrtf > Source/WebCore/platform/chromium/GestureRecognizerChromium.h:96 > + IntPoint m_lastTouchPosition; good hygiene would requires these to have initializers and be included in the reset. > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:580 > +struct TouchPointAndEvent { I wouldn't have done this like this. I would have just had an array of structs. But maybe this is nicer. It seems more complex than it needs to be. I would have done: static EventBlah* foo[] = { { ... }, { ... } }; for (int i = 0; i < sizeof(foo) / sizeof(EventBlah*); i++) { /* do stuff */ } > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:583 > + : m_point(x, y, state), the , goes under the : I think
Created attachment 109167 [details] Patch
(In reply to comment #8) > (From update of attachment 109088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109088&action=review > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:105 > > + return sqrt(m_xVelocity * m_xVelocity + m_yVelocity * m_yVelocity) > minFlickSpeed; > > Whoa, math! > > > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:616 > > + , new TouchPointAndEvent(256, 348, 1308336245.407, PlatformTouchPoint::TouchPressed, WebCore::TouchStart) > > These commas are freaky. Can me move them to the right places? Done
Created attachment 109172 [details] Patch
(In reply to comment #9) > (From update of attachment 109088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109088&action=review > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:41 > > +static const float minFlickSpeed = 300.f; > > I think recent on-device experience suggests that this should be higher. > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:105 > > + return sqrt(m_xVelocity * m_xVelocity + m_yVelocity * m_yVelocity) > minFlickSpeed; > > velocities are float. so use sqrtf removed sqrt > > > Source/WebCore/platform/chromium/GestureRecognizerChromium.h:96 > > + IntPoint m_lastTouchPosition; > > good hygiene would requires these to have initializers and be included in the reset. > done > > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:580 > > +struct TouchPointAndEvent { > > I wouldn't have done this like this. I would have just had an array of structs. But maybe this is nicer. It seems more complex than it needs to be. > > I would have done: > static EventBlah* foo[] = { { ... }, { ... } }; > for (int i = 0; i < sizeof(foo) / sizeof(EventBlah*); i++) { /* do stuff */ } > > > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:583 > > + : m_point(x, y, state), I think I am missing something. What we have is an array of touch sequences where each sequence is composed of touch events. Each event is defined as a touchPoint and TouchEvent... so thats what the data structures represent. Do you have a better data structure? > > the , goes under the : I think Done
OK. You convinced me on the tests. Looks good to me.
Comment on attachment 109172 [details] Patch Clearing flags on attachment: 109172 Committed r96365: <http://trac.webkit.org/changeset/96365>
All reviewed patches have been landed. Closing bug.