Add GestureTapCancel gesture type

Add GestureTapCancel gesture type

GestureTapCancel fires whenever a TapDown will not result in a Tap.  This can be used, for example, to cancel a pressed state triggered by TapDown.

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

[Rick Byers, 2012-09-08 23:28:55]

Rob,
Can you please review this CL to add GestureTapCancel on the Aura side?

Once https://bugs.webkit.org/show_bug.cgi?id=96183 lands, I'll put up my next CL
which plumbs GestureTapCancel through to WebKit including updating
gesture_event_filter for it.

Thanks,
   Rick

[rjkroege, 2012-09-09 17:29:56]

lgtm

[sadrul, 2012-09-09 18:33:02]

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

http://codereview.chromium.org/10928066/diff/3002/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:378: AppendTapCancelGestureEvent(point,
gestures.get());
This will add TAP_CANCEL after SCROLL_START. I think TAP_CANCEL should be before
SCROLL_START (and similarly, before PINCH_START). So AppendTapCancelGestureEvent
should be in ScrollStart, PinchStart (when state_ == PENDING_TAP), and
TwoFingerTouchDown (when state_ == PENDING_TAP)

[Rick Byers, 2012-09-10 00:56:25]

Thanks guys.  I'll work on updating the CL tomorrow.

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

http://codereview.chromium.org/10928066/diff/3002/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:378: AppendTapCancelGestureEvent(point,
gestures.get());
On 2012/09/09 18:33:02, sadrul wrote:
> This will add TAP_CANCEL after SCROLL_START. I think TAP_CANCEL should be
before
> SCROLL_START (and similarly, before PINCH_START). So
AppendTapCancelGestureEvent
> should be in ScrollStart, PinchStart (when state_ == PENDING_TAP), and
> TwoFingerTouchDown (when state_ == PENDING_TAP)

Yep, you're right - thanks.  I don't think it makes much difference in practice
which order they occur in, but I agree the order should be reversed.  I'll do
that.  

Since I was trying to make it clear that we send TapCancel (or Tap) on every
outgoing edge from the 'pending' state I wanted the sending of the event to be
next to the set_state calls (eg. it's not necessarily they case that we'll
always want to send this on every ScrollStart - in fact it gets more complicated
in this way in my fix for bug 146397), I'll come up with something.

BTW, this is something I don't like about our existing gesture recognizer unit
test infrastructure - it's too loose on what it validates (eg. makes it hard to
validate the order of events like this).  Tim's reworked log-based
infrastructure (that got reverted) was much better if I recall.  I'll see if
there's a small change I can make to validate this in the current infrastructure
without doing a major overhaul here.

[sadrul, 2012-09-10 13:18:45]

[snip]
> BTW, this is something I don't like about our existing gesture recognizer unit
> test infrastructure - it's too loose on what it validates (eg. makes it hard
to
> validate the order of events like this).  Tim's reworked log-based
> infrastructure (that got reverted) was much better if I recall.  I'll see if
> there's a small change I can make to validate this in the current
infrastructure
> without doing a major overhaul here.

There are some tests in root_window_unittests that check the order of the events
sent to a window (HoldMouseMove). There used to be some gesture-related tests
there as well (to check that unhandled gestures turned into the correct sequence
of synthetic mouse events). We probably can take the same approach for the
gesture-tests.

[Rick Byers, 2012-09-10 19:38:01]

On 2012/09/10 13:18:45, sadrul wrote:
> [snip]
> > BTW, this is something I don't like about our existing gesture recognizer
unit
> > test infrastructure - it's too loose on what it validates (eg. makes it hard
> to
> > validate the order of events like this).  Tim's reworked log-based
> > infrastructure (that got reverted) was much better if I recall.  I'll see if
> > there's a small change I can make to validate this in the current
> infrastructure
> > without doing a major overhaul here.
> 
> There are some tests in root_window_unittests that check the order of the
events
> sent to a window (HoldMouseMove). There used to be some gesture-related tests
> there as well (to check that unhandled gestures turned into the correct
sequence
> of synthetic mouse events). We probably can take the same approach for the
> gesture-tests.

Thanks.  I look a similar approach here (wrote it up before your comment), the
big difference being that I decided against converting event types to strings -
that is a little nicer for understanding failures though.  Do we have a common
test-specific place that would make sense to stick code like that?  Anyway maybe
we can take merging these approaches / cleaning up the tests further as a
separate work item after this CL...

[Rick Byers, 2012-09-10 19:38:55]

+sky for owners now that this is mostly settled

[sky, 2012-09-10 21:17:44]

LGTM

http://codereview.chromium.org/10928066/diff/2003/content/browser/renderer_ho...
File content/browser/renderer_host/gesture_event_filter.cc (right):

http://codereview.chromium.org/10928066/diff/2003/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:142:
DCHECK(coalesced_gesture_events_.front().type == type);
DCHECK_EQ

[Rick Byers, 2012-09-11 14:04:54]

Thanks Scott.

Sadrul, are you happy with this now?

http://codereview.chromium.org/10928066/diff/2003/content/browser/renderer_ho...
File content/browser/renderer_host/gesture_event_filter.cc (right):

http://codereview.chromium.org/10928066/diff/2003/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:142:
DCHECK(coalesced_gesture_events_.front().type == type);
On 2012/09/10 21:17:45, sky wrote:
> DCHECK_EQ

Done.

[sadrul, 2012-09-11 14:28:11]

On 2012/09/11 14:04:54, Rick Byers wrote:
> Thanks Scott.
> 
> Sadrul, are you happy with this now?

Oh yeah. This LGTM (I meant to just drive-by ... forgot to uncheck the
add-to-reviewer box :) )

[commit-bot: I haz the power, 2012-09-11 14:30:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/10928066/3004

[commit-bot: I haz the power, 2012-09-11 16:38:00]

Change committed as 156026