Three Finger Swipe

Three Finger Swipe

Implements a three finger swipe gesture event for the aura Gesture Recognizer.

BUG=122807
TEST=SystemGestureEventFilterTest.TapOutsideRootWindow, GestureRecognizerTest.GestureEventThreeFingerSwipe


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132570

[sadrul, 2012-04-10 20:32:51]

Looks good in general. I have some questions about diagonal swipes:

http://codereview.chromium.org/10037012/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/web_input_event_aurax11.cc (right):

http://codereview.chromium.org/10037012/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/web_input_event_aurax11.cc:362: case
ui::ET_GESTURE_THREE_FINGER_SWIPE:
Do we need this here? Presumably this should never be reached?

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:263: PinchEnd(event, point,
gestures.get());
Should there also be a ScrollEnd here?

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:661: int minVelocity = 20;
min_velocity

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:669: 
I think we should check whether all points are moving in the same direction
here. Should we also check if the velocities of the three points are within a
certain ratio of each other?

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:675: return true;
return false;

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:681: return true;
return false;

Also, why this restriction?

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:704: 0, sign, gestures);
It looks like one of the velocities is always zero. Is that intentional? Do we
not foresee any use for diagonal swipes? Even if we don't, is this the correct
place to filter this?

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.h (right):

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.h:106: float xVelocity,
x_velocity, y_velocity

[tdresser, 2012-04-11 18:17:04]

We've decided to ignore diagonal swipes for now.

http://codereview.chromium.org/10037012/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/web_input_event_aurax11.cc (right):

http://codereview.chromium.org/10037012/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/web_input_event_aurax11.cc:362: case
ui::ET_GESTURE_THREE_FINGER_SWIPE:
On 2012/04/10 20:32:51, sadrul wrote:
> Do we need this here? Presumably this should never be reached?

Done.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:263: PinchEnd(event, point,
gestures.get());
On 2012/04/10 20:32:51, sadrul wrote:
> Should there also be a ScrollEnd here?
And the related ScrollStart, when a finger from a three finger swipe is
released/cancelled.

Done.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:661: int minVelocity = 20;
On 2012/04/10 20:32:51, sadrul wrote:
> min_velocity

Done.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:669: 
On 2012/04/10 20:32:51, sadrul wrote:
> I think we should check whether all points are moving in the same direction
> here. Should we also check if the velocities of the three points are within a
> certain ratio of each other?

I feel like ensuring that the points are within a certain ratio of each other is
probably overkill.

They all have to be going above some minimum speed, and in the same direction. I
can't picture any case in which this would happen and the user would not be
trying to swipe.

I'll add it in if you feel it's necessary.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:675: return true;
On 2012/04/10 20:32:51, sadrul wrote:
> return false;

Done.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:681: return true;
On 2012/04/10 20:32:51, sadrul wrote:
> return false;
> 
> Also, why this restriction?

Done.

Added TODO to consider diagonal swipes.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:704: 0, sign, gestures);
On 2012/04/10 20:32:51, sadrul wrote:
> It looks like one of the velocities is always zero. Is that intentional? Do we
> not foresee any use for diagonal swipes? Even if we don't, is this the correct
> place to filter this?

This implementation currently assumes that we aren't expecting diagonal swipes.

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.h (right):

http://codereview.chromium.org/10037012/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.h:106: float xVelocity,
On 2012/04/10 20:32:51, sadrul wrote:
> x_velocity, y_velocity

Done.

[sadrul, 2012-04-11 19:07:26]

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:66: EXPECT_EQ(true, consumed);
EXPECT_TRUE

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:74: EXPECT_EQ(false, consumed);
EXPECT_FALSE

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:275: ScrollStart(event, point,
gestures.get());
Do we want ScrollStart here? It might be better to update PinchStart to
AppendScrollGestureBegin when state is GS_PENDING_SYNTHETIC_CLICK or
GS_THREE_FINGER_SWIPE (with that done, you may not need to change ScrollStart)

[tdresser, 2012-04-12 13:36:16]

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:66: EXPECT_EQ(true, consumed);
On 2012/04/11 19:07:26, sadrul wrote:
> EXPECT_TRUE

Deja vu, sorry.
Done.

http://codereview.chromium.org/10037012/diff/9001/ash/wm/system_gesture_event...
ash/wm/system_gesture_event_filter_unittest.cc:74: EXPECT_EQ(false, consumed);
On 2012/04/11 19:07:26, sadrul wrote:
> EXPECT_FALSE

Done.

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:275: ScrollStart(event, point,
gestures.get());
On 2012/04/11 19:07:26, sadrul wrote:
> Do we want ScrollStart here? It might be better to update PinchStart to
> AppendScrollGestureBegin when state is GS_PENDING_SYNTHETIC_CLICK or
> GS_THREE_FINGER_SWIPE (with that done, you may not need to change ScrollStart)

It's clearer what's happening if the scroll begin gesture and scroll end gesture
are appended in the same way. We need the fling logic from ScrollEnd, which
makes me think that calling ScrollStart is the right thing to do. It does make
ScrollStart a little uglier, but following the logic in
ProcessTouchEventForGesture is easier this way.

Though, I did notice that the rail logic in ScrollStart should also be
conditional on the current state.

Do you have an idea that keeps ProcessTouchEventForGesture this readable while
not having to mess with ScrollStart?

[sadrul, 2012-04-13 11:34:20]

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:275: ScrollStart(event, point,
gestures.get());
On 2012/04/12 13:36:16, tdresser wrote:
> On 2012/04/11 19:07:26, sadrul wrote:
> > Do we want ScrollStart here? It might be better to update PinchStart to
> > AppendScrollGestureBegin when state is GS_PENDING_SYNTHETIC_CLICK or
> > GS_THREE_FINGER_SWIPE (with that done, you may not need to change
ScrollStart)
> 
> It's clearer what's happening if the scroll begin gesture and scroll end
gesture
> are appended in the same way. We need the fling logic from ScrollEnd, which
> makes me think that calling ScrollStart is the right thing to do. It does make
> ScrollStart a little uglier, but following the logic in
> ProcessTouchEventForGesture is easier this way.
> 
> Though, I did notice that the rail logic in ScrollStart should also be
> conditional on the current state.
> 
> Do you have an idea that keeps ProcessTouchEventForGesture this readable while
> not having to mess with ScrollStart?

Since PinchStart already appends scroll-start for some cases, I think it makes
sense to do it for some other cases too. PinchEnd never does a scroll-end, so if
we do want to end scroll, explicitly calling ScrollEnd sounds OK to me.

Having said that ... what happens if we don't pinch-end/scroll-end in ~263/264
at all (and then don't start either here either)?

[tdresser, 2012-04-16 14:09:24]

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10037012/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:275: ScrollStart(event, point,
gestures.get());
On 2012/04/13 11:34:20, sadrul wrote:
> On 2012/04/12 13:36:16, tdresser wrote:
> > On 2012/04/11 19:07:26, sadrul wrote:
> > > Do we want ScrollStart here? It might be better to update PinchStart to
> > > AppendScrollGestureBegin when state is GS_PENDING_SYNTHETIC_CLICK or
> > > GS_THREE_FINGER_SWIPE (with that done, you may not need to change
> ScrollStart)
> > 
> > It's clearer what's happening if the scroll begin gesture and scroll end
> gesture
> > are appended in the same way. We need the fling logic from ScrollEnd, which
> > makes me think that calling ScrollStart is the right thing to do. It does
make
> > ScrollStart a little uglier, but following the logic in
> > ProcessTouchEventForGesture is easier this way.
> > 
> > Though, I did notice that the rail logic in ScrollStart should also be
> > conditional on the current state.
> > 
> > Do you have an idea that keeps ProcessTouchEventForGesture this readable
while
> > not having to mess with ScrollStart?
> 
> Since PinchStart already appends scroll-start for some cases, I think it makes
> sense to do it for some other cases too. PinchEnd never does a scroll-end, so
if
> we do want to end scroll, explicitly calling ScrollEnd sounds OK to me.
> 
> Having said that ... what happens if we don't pinch-end/scroll-end in ~263/264
> at all (and then don't start either here either)?

You're right, there's no reason to call pinch/scroll start or end. I've removed
the calls.

[sadrul, 2012-04-16 20:29:17]

LGTM

[sky, 2012-04-16 21:26:26]

LGTM

[commit-bot: I haz the power, 2012-04-17 12:29:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/10037012/27001

[commit-bot: I haz the power, 2012-04-17 13:52:47]

Change committed as 132570