Support GestureBegin and GestureEnd in ui::GestureProvider

Support GestureBegin and GestureEnd in ui::GestureProvider

Only enabled for aura, so this will be unused until the unified
GR lands.

BUG=357272
TEST=TouchDispositionGestureFilterTest.BasicGestures*,
  a bunch of tests in GestureProviderTest.

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

[jdduke (slow), 2014-04-03 20:17:16]

https://codereview.chromium.org/223673006/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/223673006/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:374: if (event.type !=
WebInputEvent::Undefined)
Shouldn't this be a DCHECK? We won't use the Gesture{Begin,End} on Android?

https://codereview.chromium.org/223673006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/223673006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/web_input_event_util.cc:334: case
ui::ET_GESTURE_BEGIN:
I'd really prefer this  be NOTREACHED(), unless there are many places where Aura
calls into this method.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.cc:623:
SendGestureBeginOrEndIfNecessary(event);
But don't we want the GestureEnd to come after events that might be generated by
the ACTION_UP or ACTION_POINTER_UP?  I think you'l need to have a call before
handling and after handling.

Ideally, we'd move code that comes before into a |OnTouchEventHandlingBegin(e)|,
and that would also do the ACTION_DOWN stuff below.  Then we'd have a
|OnTouchEventHandlingEnd(e)|, doing the ACTION_UP/ACTION_CANCEL cleanup (in
addition to the begin/end sending).

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.cc:830: switch(event.GetAction()) {
Might as well move the check for the condition here, i.e., |if
(!gesture_begin_end_enabled_) return|, and just always call this depending on
the action.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.h:36: bool
send_aura_specific_gestures;
Until we have more gestures, let's be specific with the variable name here, i.e.
|gesture_begin_end_types_enabled_|, or something like that.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:44:
sConfig.send_aura_specific_gestures = true;
I'd prefer if this default'ed to false, and we had a standalone test for
enabling it.

[tdresser, 2014-04-03 21:14:40]

https://codereview.chromium.org/223673006/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/223673006/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:374: if (event.type !=
WebInputEvent::Undefined)
On 2014/04/03 20:17:16, jdduke wrote:
> Shouldn't this be a DCHECK? We won't use the Gesture{Begin,End} on Android?

Done.

https://codereview.chromium.org/223673006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/223673006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/web_input_event_util.cc:334: case
ui::ET_GESTURE_BEGIN:
On 2014/04/03 20:17:16, jdduke wrote:
> I'd really prefer this  be NOTREACHED(), unless there are many places where
Aura
> calls into this method.

Done.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.cc:623:
SendGestureBeginOrEndIfNecessary(event);
On 2014/04/03 20:17:16, jdduke wrote:
> But don't we want the GestureEnd to come after events that might be generated
by
> the ACTION_UP or ACTION_POINTER_UP?  I think you'l need to have a call before
> handling and after handling.
> 
> Ideally, we'd move code that comes before into a
|OnTouchEventHandlingBegin(e)|,
> and that would also do the ACTION_DOWN stuff below.  Then we'd have a
> |OnTouchEventHandlingEnd(e)|, doing the ACTION_UP/ACTION_CANCEL cleanup (in
> addition to the begin/end sending).

Done.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.cc:830: switch(event.GetAction()) {
On 2014/04/03 20:17:16, jdduke wrote:
> Might as well move the check for the condition here, i.e., |if
> (!gesture_begin_end_enabled_) return|, and just always call this depending on
> the action.

Done.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider.h:36: bool
send_aura_specific_gestures;
On 2014/04/03 20:17:16, jdduke wrote:
> Until we have more gestures, let's be specific with the variable name here,
i.e.
> |gesture_begin_end_types_enabled_|, or something like that.

Done.

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/223673006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:44:
sConfig.send_aura_specific_gestures = true;
On 2014/04/03 20:17:16, jdduke wrote:
> I'd prefer if this default'ed to false, and we had a standalone test for
> enabling it.

Done.

[jdduke (slow), 2014-04-03 22:03:28]

https://codereview.chromium.org/223673006/diff/20001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/223673006/diff/20001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:374: DCHECK_NE(event.type,
WebInputEvent::Undefined)
Hmm, I think your change to CreateWebGestureEventFromGestureEventData should be
sufficient here, you can just change this back to how it was.

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

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:609:
gesture_begin_end_types_enabled_(false) {
gesture_begin_end_types_enabled_(config.gesture_begin_end_types_enabled) {

The double-tap/multi-touch toggles can all occur at different points in the
provider lifetime, so we need explicit setters, but this flag should be set only
once.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:629: bool handled =
gesture_listener_->OnTouchEvent(event, in_scale_gesture);
I guess we'll no longer need the |handled| variable (see below).

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:727: gesture.type ==
ET_GESTURE_BEGIN || gesture.type == ET_GESTURE_END);
If we send GESTURE_END before resetting the down event in
OnTouchEventHandlingEnd, we shouldn't need these extra exceptions.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:816: needs_show_press_event_ =
true;
Hmm, this show press assignment looks funky (pretty sure that's my bad).  Could
you change this to |needs_show_press_event_ = false;|, and then when we send a
TAP_DOWN set it to true?

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:842:
EndTouchScrollIfNecessary(event.GetEventTime(), true);
This will probably fail during rebase after
https://codereview.chromium.org/200623003/.  In short, you shouldn't need the
|handled| or |was_touch_scrolling| arguments.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:850: if
(gesture_begin_end_types_enabled_)
This should come before resetting the down event (see comment above).

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.h:36: bool
gesture_begin_end_types_enabled;
Maybe a short comment explaining what this does?  I should have added one for
|disable_click_delay| as well.

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

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1099:
gesture_provider_->SetBeginEndTypesEnabled(true);
If possible, I'd prefer not to expose this flag except at provider creation. 
Let's just recreate the provider but with the config adjusted accordingly.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1149: 
I guess add an ACTION_CANCEL in here somewhere for completeness?

[tdresser, 2014-04-04 16:39:31]

Thanks for the feedback, that's a significant improvement.

https://codereview.chromium.org/223673006/diff/20001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/223673006/diff/20001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:374: DCHECK_NE(event.type,
WebInputEvent::Undefined)
On 2014/04/03 22:03:28, jdduke wrote:
> Hmm, I think your change to CreateWebGestureEventFromGestureEventData should
be
> sufficient here, you can just change this back to how it was.

Done.

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

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:609:
gesture_begin_end_types_enabled_(false) {
On 2014/04/03 22:03:28, jdduke wrote:
> gesture_begin_end_types_enabled_(config.gesture_begin_end_types_enabled) {
> 
> The double-tap/multi-touch toggles can all occur at different points in the
> provider lifetime, so we need explicit setters, but this flag should be set
only
> once.

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:629: bool handled =
gesture_listener_->OnTouchEvent(event, in_scale_gesture);
On 2014/04/03 22:03:28, jdduke wrote:
> I guess we'll no longer need the |handled| variable (see below).

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:727: gesture.type ==
ET_GESTURE_BEGIN || gesture.type == ET_GESTURE_END);
On 2014/04/03 22:03:28, jdduke wrote:
> If we send GESTURE_END before resetting the down event in
> OnTouchEventHandlingEnd, we shouldn't need these extra exceptions.

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:816: needs_show_press_event_ =
true;
On 2014/04/03 22:03:28, jdduke wrote:
> Hmm, this show press assignment looks funky (pretty sure that's my bad). 
Could
> you change this to |needs_show_press_event_ = false;|, and then when we send a
> TAP_DOWN set it to true?

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:842:
EndTouchScrollIfNecessary(event.GetEventTime(), true);
On 2014/04/03 22:03:28, jdduke wrote:
> This will probably fail during rebase after
> https://codereview.chromium.org/200623003/.  In short, you shouldn't need the
> |handled| or |was_touch_scrolling| arguments.

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:850: if
(gesture_begin_end_types_enabled_)
On 2014/04/03 22:03:28, jdduke wrote:
> This should come before resetting the down event (see comment above).

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.h:36: bool
gesture_begin_end_types_enabled;
On 2014/04/03 22:03:28, jdduke wrote:
> Maybe a short comment explaining what this does?  I should have added one for
> |disable_click_delay| as well.

Done. Please double check, particularly the comment I added for
disable_click_delay.

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

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1099:
gesture_provider_->SetBeginEndTypesEnabled(true);
On 2014/04/03 22:03:28, jdduke wrote:
> If possible, I'd prefer not to expose this flag except at provider creation. 
> Let's just recreate the provider but with the config adjusted accordingly.

Done.

https://codereview.chromium.org/223673006/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1149: 
On 2014/04/03 22:03:28, jdduke wrote:
> I guess add an ACTION_CANCEL in here somewhere for completeness?

Done.

[jdduke (slow), 2014-04-04 19:08:36]

lgtm with a couple nits

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:773: // We shouldn't necessarily
cancel a tap on ACTION_UP, as the double-tap
Let's add a line break before "// We shouldn't".

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:777:
UpdateDoubleTapDetectionSupport();
Also line break before |UpdateDouble...|.

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.h:36: // There is normally a delay
before a tap event is fired, in order to allow
Nit: |When double-tap support is enabled, there will always be a delay before a
tap event is fired...|.

[tdresser, 2014-04-07 13:42:55]

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:773: // We shouldn't necessarily
cancel a tap on ACTION_UP, as the double-tap
On 2014/04/04 19:08:36, jdduke wrote:
> Let's add a line break before "// We shouldn't".

Done.

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:777:
UpdateDoubleTapDetectionSupport();
On 2014/04/04 19:08:36, jdduke wrote:
> Also line break before |UpdateDouble...|.

Done.

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/223673006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.h:36: // There is normally a delay
before a tap event is fired, in order to allow
On 2014/04/04 19:08:36, jdduke wrote:
> Nit: |When double-tap support is enabled, there will always be a delay before
a
> tap event is fired...|.

Done.

[tdresser, 2014-04-07 13:42:59]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-04-07 13:43:08]

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

[commit-bot: I haz the power, 2014-04-07 15:35:58]

Change committed as 262132