Expose native, desktop and mobile gesture detection defaults

Expose native, desktop and mobile gesture detection defaults

Provide generic desktop and mobile gesture configurations, in addition
to the native platform configuration. This will allow devtools touch
emulation to more faithfully emulate a particular device.

Note that this change involved decoupling the TouchEventQueue from
platform-specific slop region constants, afforded by a WebTouchEvent
flag indicating whether the event may cause scrolling as a default
action.

BUG=425586
Committed: https://crrev.com/11e45a87addf6fa39f6a7d505ff318236b7b82f4
Cr-Commit-Position: refs/heads/master@{#308429}

[jdduke (slow), 2014-10-27 21:55:21]

jdduke@chromium.org changed reviewers:
+ tdresser@chromium.org

[jdduke (slow), 2014-10-27 21:55:21]

tdresser@: I haven't added any tests, but I wanted to get your thoughts on this
change.

The trickiest bit was removing the TouchEventQueue dependency on the slop region
size. This patch's approach is more flexible, at the cost of a bit that needs to
be plumbed with each WebTouchEvent.

A simpler version would be breaking this patch into two: 
1) Exposing the different GestureProviderConfigTypes, but enforcing that the
slop region of the result matches the platform slop region with a giant TODO to
remove.
2) Plumbing the "will cause scrolling" bit and removing the TODO and slop
enforcement.

Thoughts?

[jdduke (slow), 2014-10-27 21:56:16]

On 2014/10/27 21:55:21, jdduke wrote:
> tdresser@: I haven't added any tests, but I wanted to get your thoughts on
this
> change.
> 
> The trickiest bit was removing the TouchEventQueue dependency on the slop
region
> size. This patch's approach is more flexible, at the cost of a bit that needs
to
> be plumbed with each WebTouchEvent.
> 
> A simpler version would be breaking this patch into two: 
> 1) Exposing the different GestureProviderConfigTypes, but enforcing that the
> slop region of the result matches the platform slop region with a giant TODO
to
> remove.
> 2) Plumbing the "will cause scrolling" bit and removing the TODO and slop
> enforcement.
> 
> Thoughts?

Oh, I should note that in its current form, this is dependent on eager GR and
won't be able to land until then (unless we made an exception on Aura and do a
separate slop test for each touchmove).

[jdduke (slow), 2014-10-27 22:00:15]

https://codereview.chromium.org/679633005/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/679633005/diff/1/ui/events/event.h#newcode519
ui/events/event.h:519: void set_may_cause_scrolling(bool causes) {
may_cause_scrolling_ = causes; }
This isn't my favorite... I wonder if it would be simplest to duplicate the slop
region check for Aura separately in RWHVAura for now?

[dgozman, 2014-10-28 11:25:20]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2014-10-28 11:25:22]

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:34:
ui::GestureProviderConfigType::GENERIC_MOBILE);
Great!

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:423:
touch_event_.defaultActionMayCauseScrolling =
So, this function should be called exactly after
|gesture_provider_.OnTouchEvent| now? Maybe add a comment mentioning this
restriction?

[tdresser, 2014-10-28 13:41:41]

One possibility I've thrown around is to do a small pre-processing phase before
gesture recognition really starts. This separate phase could calculate a little
bit of data per pointer, like whether it has exceeded its slop region, and
potentially also the pointer velocity.

(I was mainly thinking about this because I was contemplating how we'd approach
things if we wanted to expose finger velocity through touch events.)

That likely isn't the right solution now, but I'd be interested in hearing your
thoughts on it.

It would prevent us from depending on eager GR, and would mean we could continue
passing immutable touch events through the majority of the gesture detection
stack.

I'm not sure whether it would be a long term increase or decrease in
complexity...

https://codereview.chromium.org/679633005/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/679633005/diff/1/ui/events/event.h#newcode519
ui/events/event.h:519: void set_may_cause_scrolling(bool causes) {
may_cause_scrolling_ = causes; }
On 2014/10/27 22:00:15, jdduke wrote:
> This isn't my favorite... I wonder if it would be simplest to duplicate the
slop
> region check for Aura separately in RWHVAura for now?

Your goal is to keep ui::TouchEvent from knowing anything about whether or not
it causes scrolling? That approach sounds reasonable to me. Eager GR will allow
a lot of cleanup of event conversion, which should make this a little clearer.

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:34:
ui::GestureProviderConfigType::GENERIC_MOBILE);
Can't you also emulate a touch screen laptop?

Also, aren't the lines below redundant now?

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (left):

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:62: // Determines the
bounds of the (square) touchmove slop suppression region.
That "square" is pretty out of date. Oops.

[jdduke (slow), 2014-10-28 22:08:53]

On 2014/10/28 13:41:41, tdresser wrote:
> One possibility I've thrown around is to do a small pre-processing phase
before
> gesture recognition really starts. This separate phase could calculate a
little
> bit of data per pointer, like whether it has exceeded its slop region, and
> potentially also the pointer velocity.
> 
> (I was mainly thinking about this because I was contemplating how we'd
approach
> things if we wanted to expose finger velocity through touch events.)
> 
> That likely isn't the right solution now, but I'd be interested in hearing
your
> thoughts on it.
> 
> It would prevent us from depending on eager GR, and would mean we could
continue
> passing immutable touch events through the majority of the gesture detection
> stack.
> 
> I'm not sure whether it would be a long term increase or decrease in
> complexity...

Yeah, seems reasonable enough, particularly when we're more unified in our
TouchEvent/MotionEvent->WebTouchEvent conversions.

[jdduke (slow), 2014-10-30 22:06:56]

It's probably worth deferring this patch until eager GR lands.

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:34:
ui::GestureProviderConfigType::GENERIC_MOBILE);
On 2014/10/28 13:41:41, tdresser wrote:
> Can't you also emulate a touch screen laptop?
> 

I guess I would consider desktop as PC/laptop, and mobile as phone/tablet, but
if you have some better names for that dichotomy I'm all ears :)

> Also, aren't the lines below redundant now?

Yeah, I left them as a safeguard in case |PLATFORM_DEFAULT| is ever a supplied
type.

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:423:
touch_event_.defaultActionMayCauseScrolling =
On 2014/10/28 11:25:22, dgozman wrote:
> So, this function should be called exactly after
> |gesture_provider_.OnTouchEvent| now? Maybe add a comment mentioning this
> restriction?

Yeah, this was pretty unclear. I reworked the logic a bit, and made the scroll
causality an explicit part of the GestureProvider::OnTouchEvent API.

[dgozman, 2014-10-31 06:47:41]

>
https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/touch_emulator.cc:423:
> touch_event_.defaultActionMayCauseScrolling =
> On 2014/10/28 11:25:22, dgozman wrote:
> > So, this function should be called exactly after
> > |gesture_provider_.OnTouchEvent| now? Maybe add a comment mentioning this
> > restriction?
> 
> Yeah, this was pretty unclear. I reworked the logic a bit, and made the scroll
> causality an explicit part of the GestureProvider::OnTouchEvent API.

Thank you, this is much cleaner now. Touch emulator lgtm.

[tdresser, 2014-10-31 12:34:12]

Nits.

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:34:
ui::GestureProviderConfigType::GENERIC_MOBILE);
On 2014/10/30 22:06:54, jdduke wrote:
> On 2014/10/28 13:41:41, tdresser wrote:
> > Can't you also emulate a touch screen laptop?
> > 
> 
> I guess I would consider desktop as PC/laptop, and mobile as phone/tablet, but
> if you have some better names for that dichotomy I'm all ears :)
> 
> > Also, aren't the lines below redundant now?
> 
> Yeah, I left them as a safeguard in case |PLATFORM_DEFAULT| is ever a supplied
> type.

Oh nevermind, I was confused.

https://codereview.chromium.org/679633005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/679633005/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:713: auto
result = gesture_provider_.OnTouchEvent(event);
See other comment on auto.

https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/filtered_gesture_provider.h (right):

https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/filtered_gesture_provider.h:32: // |event| and cease
furthe propagation.
furthe -> further

https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/filtered_gesture_provider_unittest.cc (right):

https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/filtered_gesture_provider_unittest.cc:44: //
Exceeding the slop should triggering scrolling and be reflected in the API.
triggering -> trigger

https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/filtered_gesture_provider_unittest.cc:44: //
Exceeding the slop should triggering scrolling and be reflected in the API.
"and be reflected in the API"
Does this statement add any useful information? The vast majority of tests have
an implicit "and be reflected in the API" don't they?

https://codereview.chromium.org/679633005/diff/60001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/679633005/diff/60001/ui/events/gestures/gestu...
ui/events/gestures/gesture_provider_aura.cc:53: auto result =
filtered_gesture_provider_.OnTouchEvent(pointer_state_);
I'm not entirely convinced that this is a legitimate use of "auto", but I don't
have a particularly good feel for what we consider "type names that are just
clutter."

I'd lean towards using an explicit type here, but I'm fine either way.

[tdresser, 2014-10-31 12:34:44]

On 2014/10/31 12:34:12, tdresser wrote:
> Nits.
> 
>
https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_emulator.cc (right):
> 
>
https://codereview.chromium.org/679633005/diff/20001/content/browser/renderer...
> content/browser/renderer_host/input/touch_emulator.cc:34:
> ui::GestureProviderConfigType::GENERIC_MOBILE);
> On 2014/10/30 22:06:54, jdduke wrote:
> > On 2014/10/28 13:41:41, tdresser wrote:
> > > Can't you also emulate a touch screen laptop?
> > > 
> > 
> > I guess I would consider desktop as PC/laptop, and mobile as phone/tablet,
but
> > if you have some better names for that dichotomy I'm all ears :)
> > 
> > > Also, aren't the lines below redundant now?
> > 
> > Yeah, I left them as a safeguard in case |PLATFORM_DEFAULT| is ever a
supplied
> > type.
> 
> Oh nevermind, I was confused.
> 
>
https://codereview.chromium.org/679633005/diff/60001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/679633005/diff/60001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:713: auto
> result = gesture_provider_.OnTouchEvent(event);
> See other comment on auto.
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
> File ui/events/gesture_detection/filtered_gesture_provider.h (right):
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
> ui/events/gesture_detection/filtered_gesture_provider.h:32: // |event| and
cease
> furthe propagation.
> furthe -> further
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
> File ui/events/gesture_detection/filtered_gesture_provider_unittest.cc
(right):
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
> ui/events/gesture_detection/filtered_gesture_provider_unittest.cc:44: //
> Exceeding the slop should triggering scrolling and be reflected in the API.
> triggering -> trigger
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gesture_detect...
> ui/events/gesture_detection/filtered_gesture_provider_unittest.cc:44: //
> Exceeding the slop should triggering scrolling and be reflected in the API.
> "and be reflected in the API"
> Does this statement add any useful information? The vast majority of tests
have
> an implicit "and be reflected in the API" don't they?
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gestures/gestu...
> File ui/events/gestures/gesture_provider_aura.cc (right):
> 
>
https://codereview.chromium.org/679633005/diff/60001/ui/events/gestures/gestu...
> ui/events/gestures/gesture_provider_aura.cc:53: auto result =
> filtered_gesture_provider_.OnTouchEvent(pointer_state_);
> I'm not entirely convinced that this is a legitimate use of "auto", but I
don't
> have a particularly good feel for what we consider "type names that are just
> clutter."
> 
> I'd lean towards using an explicit type here, but I'm fine either way.

ui/events LGTM (with above nits).

[jdduke (slow), 2014-11-12 19:10:20]

Per Rick's suggestion I'm pulling out the touch/scroll related changes into a
separate patch: https://codereview.chromium.org/718153002

This patch will then simply be exposing the different gesture config types and
hooking them up to the touch emulator.

[dgozman, 2014-11-20 15:17:52]

On 2014/11/12 19:10:20, jdduke wrote:
> Per Rick's suggestion I'm pulling out the touch/scroll related changes into a
> separate patch: https://codereview.chromium.org/718153002
> 
> This patch will then simply be exposing the different gesture config types and
> hooking them up to the touch emulator.

Is this patch blocked on something?

[tdresser, 2014-11-20 15:28:16]

On 2014/11/20 15:17:52, dgozman wrote:
> On 2014/11/12 19:10:20, jdduke wrote:
> > Per Rick's suggestion I'm pulling out the touch/scroll related changes into
a
> > separate patch: https://codereview.chromium.org/718153002
> > 
> > This patch will then simply be exposing the different gesture config types
and
> > hooking them up to the touch emulator.
> 
> Is this patch blocked on something?

It's currently blocked on:
https://codereview.chromium.org/680413006/, 
which I'm actively working on.

[jdduke (slow), 2014-12-11 19:06:38]

+sadrul@ for ui/events/event.* and the base file change sin 
content/browser/renderer_host.

https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h#newco...
ui/events/event.h:525: void set_may_cause_scrolling(bool causes) {
may_cause_scrolling_ = causes; }
I'm open to different naming here, or event a different way of propagating this
bit.

[jdduke (slow), 2014-12-11 19:32:54]

https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h#newco...
ui/events/event.h:576: bool may_cause_scrolling_;
Awfully tempting to make this mutable... Would avoid the need to restructure
event-related APIs, and emphasizes that the value is likely to change without
affecting the core event, but it might be somewhat confusing...

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

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

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

Oops, actually adding sadrul@.

On 2014/12/11 19:06:38, jdduke wrote:
> +sadrul@ for ui/events/event.* and the base file change sin 
> content/browser/renderer_host.
> 
> https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/679633005/diff/100001/ui/events/event.h#newco...
> ui/events/event.h:525: void set_may_cause_scrolling(bool causes) {
> may_cause_scrolling_ = causes; }
> I'm open to different naming here, or event a different way of propagating
this
> bit.

[sadrul, 2014-12-12 16:24:25]

The try failures look related. Mind taking a look at those?

[jdduke (slow), 2014-12-15 16:11:09]

On 2014/12/12 16:24:25, sadrul wrote:
> The try failures look related. Mind taking a look at those?

Yup, fixed, the remaining failures look like flakes.

[sadrul, 2014-12-15 17:40:46]

LGTM

https://codereview.chromium.org/679633005/diff/140001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/679633005/diff/140001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:897: 
Can you add a test to make sure this bit is set correctly in the correct event?
(e.g. generate a touch-press event, then a touch-move with enough delta to make
sure this gets set, and verify that this indeed gets set).

https://codereview.chromium.org/679633005/diff/140001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/679633005/diff/140001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:154:
static_cast<aura::Window*>(nullptr));
Can you just do: mutable_touch_event = touch_event;?

[jdduke (slow), 2014-12-15 18:37:36]

jdduke@chromium.org changed reviewers:
+ sievers@chromium.org

[jdduke (slow), 2014-12-15 18:37:38]

+sievers@ for content/browser/frame_host/render_widget_host_view_guest.cc owner
review.

https://codereview.chromium.org/679633005/diff/140001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/679633005/diff/140001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:897: 
On 2014/12/15 17:40:46, sadrul wrote:
> Can you add a test to make sure this bit is set correctly in the correct
event?
> (e.g. generate a touch-press event, then a touch-move with enough delta to
make
> sure this gets set, and verify that this indeed gets set).

Done.

https://codereview.chromium.org/679633005/diff/140001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/679633005/diff/140001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:154:
static_cast<aura::Window*>(nullptr));
On 2014/12/15 17:40:46, sadrul wrote:
> Can you just do: mutable_touch_event = touch_event;?

Hmm, much better, I guess assignment works but not the copy constructor.

[no sievers, 2014-12-15 19:47:06]

On 2014/12/15 18:37:38, jdduke wrote:
> +sievers@ for content/browser/frame_host/render_widget_host_view_guest.cc
owner
> review.

lgtm

[jdduke (slow), 2014-12-15 20:57:24]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:58:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679633005/160001

[commit-bot: I haz the power, 2014-12-15 21:49:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-15 21:50:55]

Patchset 9 (id:??) landed as
https://crrev.com/11e45a87addf6fa39f6a7d505ff318236b7b82f4
Cr-Commit-Position: refs/heads/master@{#308429}