Send TapCancel when a second finger is pressed.

Send TapCancel when a second finger is pressed.

This is required to stop the long press affordance on Aura.

BUG=371460
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270132

[tdresser, 2014-05-12 19:44:48]

jdduke@ PTAL.

[jdduke (slow), 2014-05-12 19:49:11]

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213:
CancelTapIfNecessary(packet.timestamp());
I wonder if we should be doing any validation that we don't get a GESTURE_TAP
after we synthetically cancel the tap? Maybe something to think about for the
future, perhaps as an extension of the gesture stream validator.

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:695:
TapNotSentIfTapEndingEventReceived) {
Not sure if I understand the change to this test?

[tdresser, 2014-05-12 19:58:11]

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213:
CancelTapIfNecessary(packet.timestamp());
On 2014/05/12 19:49:11, jdduke wrote:
> I wonder if we should be doing any validation that we don't get a GESTURE_TAP
> after we synthetically cancel the tap? Maybe something to think about for the
> future, perhaps as an extension of the gesture stream validator.

We could DCHECK(needs_tap_ending_event) in TDGF::SendGesture, ET_GESTURE_TAP. I
can't think of any legitimate cases in which that wouldn't pass. Can you?

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:695:
TapNotSentIfTapEndingEventReceived) {
On 2014/05/12 19:49:11, jdduke wrote:
> Not sure if I understand the change to this test?

The test previously put two touch points down (triggering a tap cancel
immediately). I attempted to keep the spirit of the test (Ensuring only one tap
ending event will be sent), but switched it to no longer use two touch points.

[jdduke (slow), 2014-05-12 20:56:06]

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213:
CancelTapIfNecessary(packet.timestamp());
On 2014/05/12 19:58:11, tdresser wrote:
> On 2014/05/12 19:49:11, jdduke wrote:
> > I wonder if we should be doing any validation that we don't get a
GESTURE_TAP
> > after we synthetically cancel the tap? Maybe something to think about for
the
> > future, perhaps as an extension of the gesture stream validator.
> 
> We could DCHECK(needs_tap_ending_event) in TDGF::SendGesture, ET_GESTURE_TAP.
I
> can't think of any legitimate cases in which that wouldn't pass. Can you?

Yeah that sounds reasonable, if we are seeing it it's probably a bug.

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:695:
TapNotSentIfTapEndingEventReceived) {
On 2014/05/12 19:58:11, tdresser wrote:
> On 2014/05/12 19:49:11, jdduke wrote:
> > Not sure if I understand the change to this test?
> 
> The test previously put two touch points down (triggering a tap cancel
> immediately). I attempted to keep the spirit of the test (Ensuring only one
tap
> ending event will be sent), but switched it to no longer use two touch points.

Ah right I missed the secondary touch.. but I'm not sure how the new test shows
that a tap is suppressed, as we don't actually queue a tap gesture? 

I think the existing test is faulty too.  Let's do this:

 PushGesture(ET_GESTURE_TAP_DOWN);
  PressTouchPoint(1, 1);
  EXPECT_TRUE(GesturesMatch(Gestures(ET_GESTURE_TAP_DOWN),
                            GetAndResetSentGestures()));

  PushGesture(ET_GESTURE_TAP);
  ReleaseTouchPoint();
  SendTouchNotConsumedAck();
  EXPECT_TRUE(GesturesMatch(Gestures(ET_GESTURE_SHOW_PRESS,
                                     ET_GESTURE_TAP),
                            GetAndResetSentGestures()));

  // The tap should not be cancelled as it was terminated by a |ET_GESTURE_TAP|.
  PressTouchPoint(2, 2);
  SendTouchConsumedAck();
  EXPECT_FALSE(GesturesSent());

[tdresser, 2014-05-13 15:13:15]

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213:
CancelTapIfNecessary(packet.timestamp());
On 2014/05/12 20:56:06, jdduke wrote:
> On 2014/05/12 19:58:11, tdresser wrote:
> > On 2014/05/12 19:49:11, jdduke wrote:
> > > I wonder if we should be doing any validation that we don't get a
> GESTURE_TAP
> > > after we synthetically cancel the tap? Maybe something to think about for
> the
> > > future, perhaps as an extension of the gesture stream validator.
> > 
> > We could DCHECK(needs_tap_ending_event) in TDGF::SendGesture,
ET_GESTURE_TAP.
> I
> > can't think of any legitimate cases in which that wouldn't pass. Can you?
> 
> Yeah that sounds reasonable, if we are seeing it it's probably a bug.

Done.

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/280593003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:695:
TapNotSentIfTapEndingEventReceived) {
On 2014/05/12 20:56:06, jdduke wrote:
> On 2014/05/12 19:58:11, tdresser wrote:
> > On 2014/05/12 19:49:11, jdduke wrote:
> > > Not sure if I understand the change to this test?
> > 
> > The test previously put two touch points down (triggering a tap cancel
> > immediately). I attempted to keep the spirit of the test (Ensuring only one
> tap
> > ending event will be sent), but switched it to no longer use two touch
points.
> 
> Ah right I missed the secondary touch.. but I'm not sure how the new test
shows
> that a tap is suppressed, as we don't actually queue a tap gesture? 
> 
> I think the existing test is faulty too.  Let's do this:
> 
>  PushGesture(ET_GESTURE_TAP_DOWN);
>   PressTouchPoint(1, 1);
>   EXPECT_TRUE(GesturesMatch(Gestures(ET_GESTURE_TAP_DOWN),
>                             GetAndResetSentGestures()));
> 
>   PushGesture(ET_GESTURE_TAP);
>   ReleaseTouchPoint();
>   SendTouchNotConsumedAck();
>   EXPECT_TRUE(GesturesMatch(Gestures(ET_GESTURE_SHOW_PRESS,
>                                      ET_GESTURE_TAP),
>                             GetAndResetSentGestures()));
> 
>   // The tap should not be cancelled as it was terminated by a
|ET_GESTURE_TAP|.
>   PressTouchPoint(2, 2);
>   SendTouchConsumedAck();
>   EXPECT_FALSE(GesturesSent());

Done.

[jdduke (slow), 2014-05-13 15:16:37]

lgtm

[tdresser, 2014-05-13 15:21:59]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-13 15:23:51]

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

[commit-bot: I haz the power, 2014-05-13 17:01:45]

Change committed as 270132