Implement flick gesture in Chromium Gesture Recognizer

• Component: New Bugs
• Status: RESOLVED FIXED

[Varun Jain]

Implement flick gesture in Chromium Gesture Recognizer

[Varun Jain]

Created attachment 107040 [details]
Patch

[Robert Kroeger]

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.

[Varun Jain]

Created attachment 107062 [details]
Patch

[Varun Jain]

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.

[Robert Kroeger]

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.)

[Varun Jain]

Created attachment 109088 [details]
Patch

[Varun Jain]

(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

[Dimitri Glazkov (Google)]

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?

[Robert Kroeger]

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

[Varun Jain]

Created attachment 109167 [details]
Patch

[Varun Jain]

(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

[Varun Jain]

Created attachment 109172 [details]
Patch

[Varun Jain]

(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

[Robert Kroeger]

OK. You convinced me on the tests. Looks good to me.

[WebKit Review Bot]

Comment on attachment 109172 [details]
Patch

Clearing flags on attachment: 109172

Committed r96365: <http://trac.webkit.org/changeset/96365>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.