Support minimum gesture bounds in GestureProvider

Support minimum gesture bounds in GestureProvider

Some touch devices report either zero or unreasonably small touch sizes. While
this doesn't present a problem for gesture detection, it does affect the
behavior of the touch-derived gestures. For example, tap disambiguation is
prevented if the tap gesture's size is sufficiently small. As a workaround,
provide a platform-specific minimum gesture bounds length in GestureProvider.
On Android, this value is the same as was used before the unified gesture
detector, and on Aura we simply use half the default touch size.

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

[jdduke (slow), 2014-06-06 16:46:16]

PTAL, thanks.

[tdresser, 2014-06-06 16:58:05]

Don't you need to switch to using GetRawTouchMajor when constructing
WebInputEvents?

Can you add a test?

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_android.h:74: float
GetRawTouchMinor() const { return GetRawTouchMinor(0); }
Hmmm, it's a little weird to have GetRawTouchMinor() == GetTouchMinor(), but I
guess it's not unreasonable.

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_android.h:124: // devices that
otherwise report zero or inappropriatley small values. Note
s/inappropriatly/inappropriately

[jdduke (slow), 2014-06-06 17:18:13]

Rick, how important is it that we report the raw values for radiusX currently? I
know we want to expose the right values, but currently we report the same value
for radiusX/radiusY and we don't report orientation, so I'm wondering how much
effort I should put into pushing the unadjusted values right now.

As for a test, we don't have any unit tests for MotionEventAndroid, and I'm not
sure the harness that would be necessary with all of the JNI bindings would be
worth it just to verify that std::max is working properly =/

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_android.h:74: float
GetRawTouchMinor() const { return GetRawTouchMinor(0); }
On 2014/06/06 16:58:04, tdresser wrote:
> Hmmm, it's a little weird to have GetRawTouchMinor() == GetTouchMinor(), but I
> guess it's not unreasonable.

There's never been a |MotionEvent::GetTouchMinor()|, so now the only reference
is to |MotionEventAndroid::GetRawTouchMinor()|.

https://codereview.chromium.org/321563002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_android.h:124: // devices that
otherwise report zero or inappropriatley small values. Note
On 2014/06/06 16:58:04, tdresser wrote:
> s/inappropriatly/inappropriately

Done.

[tdresser, 2014-06-06 17:32:05]

Yeah, if it's going to be a headache, I'm fine without a test here.

[jdduke (slow), 2014-06-06 17:59:49]

OK, I exposed GetRawTouchMajor, PTAL, thanks.

[tdresser, 2014-06-06 18:18:57]

LGTM

[Rick Byers, 2014-06-06 18:52:36]

On 2014/06/06 17:59:49, jdduke wrote:
> OK, I exposed GetRawTouchMajor, PTAL, thanks.

I'd prefer to expose the raw data directly from the OS to the application in
touch events (touch events should be low-level APIs, we should avoid getting in
the way off the app if we can), but it's hard to argue that it's really
important in this case.

Do we really need to add a separate member to MotionEvent here?  I've started
the change to add minor radius and rotation to MotionEvent and there's a fair
amount of extra plumbing (also need historical members, the extra cache, etc.). 
Having both raw and processed radius values increases that plumbing further.

I was expecting we'd apply the minimum radius downstream from MotionEvent
somewhere (that's both cross-platform and doesn't interfere with the low-level
APIs).  Jared, didn't you say you were going to do it at the point of
GestureDetection?

[jdduke (slow), 2014-06-06 19:03:56]

On 2014/06/06 18:52:36, Rick Byers wrote:
> On 2014/06/06 17:59:49, jdduke wrote:
> > OK, I exposed GetRawTouchMajor, PTAL, thanks.
> 
> I'd prefer to expose the raw data directly from the OS to the application in
> touch events (touch events should be low-level APIs, we should avoid getting
in
> the way off the app if we can), but it's hard to argue that it's really
> important in this case.
> 
> Do we really need to add a separate member to MotionEvent here?  I've started
> the change to add minor radius and rotation to MotionEvent and there's a fair
> amount of extra plumbing (also need historical members, the extra cache,
etc.). 
> Having both raw and processed radius values increases that plumbing further.
> 
> I was expecting we'd apply the minimum radius downstream from MotionEvent
> somewhere (that's both cross-platform and doesn't interfere with the low-level
> APIs).  Jared, didn't you say you were going to do it at the point of
> GestureDetection?

The only problem there is that the min radius is MotionEvent-dependent; Android
supports stylus and mouse-based MotionEvents, and I'm not sure we want to
enforce a minimum for those MotionEvent types. I'm not sure how big a deal that
is, but it will cause things like a mouse click on Android to trigger tap
disambiguation.

[jdduke (slow), 2014-06-06 19:24:16]

On 2014/06/06 19:03:56, jdduke wrote:
> On 2014/06/06 18:52:36, Rick Byers wrote:
> > On 2014/06/06 17:59:49, jdduke wrote:
> > > OK, I exposed GetRawTouchMajor, PTAL, thanks.
> > 
> > I'd prefer to expose the raw data directly from the OS to the application in
> > touch events (touch events should be low-level APIs, we should avoid getting
> in
> > the way off the app if we can), but it's hard to argue that it's really
> > important in this case.
> > 
> > Do we really need to add a separate member to MotionEvent here?  I've
started
> > the change to add minor radius and rotation to MotionEvent and there's a
fair
> > amount of extra plumbing (also need historical members, the extra cache,
> etc.). 
> > Having both raw and processed radius values increases that plumbing further.
> > 
> > I was expecting we'd apply the minimum radius downstream from MotionEvent
> > somewhere (that's both cross-platform and doesn't interfere with the
low-level
> > APIs).  Jared, didn't you say you were going to do it at the point of
> > GestureDetection?
> 
> The only problem there is that the min radius is MotionEvent-dependent;
Android
> supports stylus and mouse-based MotionEvents, and I'm not sure we want to
> enforce a minimum for those MotionEvent types. I'm not sure how big a deal
that
> is, but it will cause things like a mouse click on Android to trigger tap
> disambiguation.

Hmm, per discussion with Rick, I guess I'll avoid overloading the TouchMajor
interpretation and instead plumb a min gesture size in the GestureProvider.

[jdduke (slow), 2014-06-06 22:28:17]

Rick, Tim, what do you think of this approach? I went ahead and removed the
redundant "type" variable from GestureEventData as well.

[tdresser, 2014-06-09 13:36:38]

Thanks for getting rid of the redundant type.
LGTM

Nit: should we refer to the gesture bound, or the gesture bounds? I would have
thought "bounds" was correct.

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data.cc (right):

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data.cc:26: DCHECK_LE(details.type(),
ET_GESTURE_TYPE_END);
We could move these DCHECKs to the GestureEventDetails, couldn't we?

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data.h (right):

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data.h:34: EventType type() const {
return details.type(); }
Awesome, thanks.

[jdduke (slow), 2014-06-09 15:40:57]

On 2014/06/09 13:36:38, tdresser wrote:
> Thanks for getting rid of the redundant type.
> LGTM
> 
> Nit: should we refer to the gesture bound, or the gesture bounds? I would have
> thought "bounds" was correct.

Yup, that was a typo, thanks.


sadrul@: Owner review for ui/events/gesture_event_details.{cc,h}? Thanks.

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data.cc (right):

https://codereview.chromium.org/321563002/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data.cc:26: DCHECK_LE(details.type(),
ET_GESTURE_TYPE_END);
On 2014/06/09 13:36:37, tdresser wrote:
> We could move these DCHECKs to the GestureEventDetails, couldn't we?

Totally, done.

[sadrul, 2014-06-11 16:37:29]

lgtm

[jdduke (slow), 2014-06-11 16:38:39]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-06-11 16:40:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/321563002/120001

[commit-bot: I haz the power, 2014-06-11 20:52:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15558)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18699)

[commit-bot: I haz the power, 2014-06-11 20:56:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:56:40]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15625)

[jdduke (slow), 2014-06-11 21:04:46]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:08:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/321563002/140001

[commit-bot: I haz the power, 2014-06-11 22:26:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15644)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18781)

[commit-bot: I haz the power, 2014-06-11 22:29:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:29:08]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[jdduke (slow), 2014-06-11 22:30:49]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:32:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/321563002/140001

[commit-bot: I haz the power, 2014-06-11 23:49:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15725)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18849)

[commit-bot: I haz the power, 2014-06-12 00:07:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:07:14]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[jdduke (slow), 2014-06-12 01:02:37]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:04:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/321563002/140001

[commit-bot: I haz the power, 2014-06-12 06:33:06]

Change committed as 276550