Port of Android platform gesture detection code to C++

Port of Android platform gesture detection code to C++

This is a direct C++ port of all Java classes utilized by the current
ContentViewGestureHandler, including forks of Android C++ classes used in
gesture detection. This includes the following:

* ZoomManager - Forked from Chromium.
* SnapScrollingController - Forked from Chromium.
* GestureDetector - Forked from Chromium class originally from Android.
* ScaleGestureDetector - Forked from Android.
* VelocityTracker - Forked from Android.
* VelocityTrackerState - Forked from Android.
* BitSet32 - Forked from Android.

A MotionEvent class is introduced to abstract touch event API differences
between Chromium and Android, allowing other platforms to hook into the
detection pipeline without loss of generality.

Note that this patch is additive only, and does not affect gesture detection
for current platforms.  Wiring for Android and Aura will land in separate
patches. (see https://codereview.chromium.org/128613003/)

BUG=332418
NOTRY=true

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

[jdduke (slow), 2014-02-20 18:15:38]

I think this is ready to go.  Now that M34 has branched, we can move ahead with
minimal risk, first with this portion, then the Android hookups in content/
(perhaps in parallel with the Aura implementation).

[tdresser, 2014-02-20 19:11:19]

I've skimmed through everything, and made a few comments.

I'm not sure how it makes the most sense to get this thoroughly reviewed though.

I'd appreciate another set of eyes at least on the set of files that didn't get
explicit reviewers in the tracking bug.
(https://codereview.chromium.org/128613003/).

https://codereview.chromium.org/171773012/diff/1/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/171773012/diff/1/ui/events/events.gyp#newcode64
ui/events/events.gyp:64: }]
Trailing commas seem to be pretty standard around in .gyp files. Was this
intentional?

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

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:121: void
CheckScrollEventSequenceForendActionType(
ForendAction -> ForEndAction

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

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/velocity_tracker.h:29: enum { MAX_POINTERS = 16,
MAX_POINTER_ID = 31 };
MAX_POINTERS should probably be pulled from somewhere else. I'm not sure where
this constant should really live - it exists in a few places at the moment.

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/velocity_tracker.h:31: enum Strategy {
I'm unconvinced that it makes sense to port all strategies. Why not only port
LSQ2?
Or at least exclude methods which are known to be terrible?

[jdduke (slow), 2014-02-20 22:52:07]

On 2014/02/20 19:11:19, tdresser wrote:
> I'd appreciate another set of eyes at least on the set of files that didn't
get
> explicit reviewers in the tracking bug.

Hmm, VelocityTracker and VelocityTrackerState were already written in C++, so
the only change was stylistic and moving some code into the .cc file from the .h
file.  I added the unit test as a sanity check, and the GestureProviderTest's
also cover the two classes indirectly.

Probably the biggest piece that needs a thorough check is GestureProvider (from
ContentViewGestureHandler), for which all unit tests were ported directly and
are passing.   I'll try to find another body to take a pass over that.

[jdduke (slow), 2014-02-21 02:31:55]

https://codereview.chromium.org/171773012/diff/1/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/171773012/diff/1/ui/events/events.gyp#newcode64
ui/events/events.gyp:64: }]
On 2014/02/20 19:11:24, tdresser wrote:
> Trailing commas seem to be pretty standard around in .gyp files. Was this
> intentional?

Nope, completely accidental.

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

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:121: void
CheckScrollEventSequenceForendActionType(
On 2014/02/20 19:11:24, tdresser wrote:
> ForendAction -> ForEndAction

Done.

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

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/velocity_tracker.h:29: enum { MAX_POINTERS = 16,
MAX_POINTER_ID = 31 };
On 2014/02/20 19:11:24, tdresser wrote:
> MAX_POINTERS should probably be pulled from somewhere else. I'm not sure where
> this constant should really live - it exists in a few places at the moment.

Well, there's touchesLengthCap in Blink, and a runtime ui::MaxTouchPoints()
query.  Note that this is not a hard cap; the code "gracefully" handles the case
if there are more than MAX_POINTERS pointers in the provided MotionEvent. 
However, if that occurs you will lose accuracy.  I can add a comment to that
effect.

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/velocity_tracker.h:31: enum Strategy {
On 2014/02/20 19:11:24, tdresser wrote:
> I'm unconvinced that it makes sense to port all strategies. Why not only port
> LSQ2?
> Or at least exclude methods which are known to be terrible?

No reason, other than it being very little extra work to do so. The easier it is
to pull upstream changes, the better, particularly if Android decides to change
their default.

[tdresser, 2014-02-21 14:29:28]

Sounds good.

Everything except ui/events/gesture_detection/gesture_provider* LGTM.

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

https://codereview.chromium.org/171773012/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/velocity_tracker.h:31: enum Strategy {
On 2014/02/21 02:31:56, jdduke wrote:
> On 2014/02/20 19:11:24, tdresser wrote:
> > I'm unconvinced that it makes sense to port all strategies. Why not only
port
> > LSQ2?
> > Or at least exclude methods which are known to be terrible?
> 
> No reason, other than it being very little extra work to do so. The easier it
is
> to pull upstream changes, the better, particularly if Android decides to
change
> their default.
SGTM.

[jdduke (slow), 2014-02-21 15:58:18]

skyostil@: I hate to add another review to your plate, but you may be the most
familiar with ContentViewGestureHandler... Would you mind taking a glance at
ui/events/gesture_detection/gesture_provider.{h,cc} (ported directly from
ContentViewGestureHandler)? Mainly looking for glaring errors and stylistics
flaws. Thanks!

I have a plan to convert all of these classes to use proper gfx:: geometry
types, which could cut the line count cost here by 20% or so, but that will come
later.

[Xianzhu, 2014-02-21 17:45:43]

scale_gesture_detector lgtm.

[Sami, 2014-02-21 18:59:08]

gesture_provider.{h,cc} lgtm. Just some minor nits.

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

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:70: // TODO: Need to deal with
multi-touch transition
TODO(jdduke) plus a '.'

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:117: bool
IsScaleGestureDetectionInProgress() {
Could be const

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:121: void
SetIgnoreDetectorEvents(bool value) {
Could be a simple setter: set_ignore_detector_events(...)

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:210: // Return true to indicate
that we want to handle touch
Add '.'

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:225: std::sqrt(distance_x *
distance_x + distance_y * distance_y);
We could probably change this to sqrtf & floats, but that can be done later.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:258: // Because we are passing
integers to webkit, this could introduce
s/webkit/blink :)

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:296:
show_press_data.show_press.width = e.GetTouchMajor();
Is this right? width = major, height = width?

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:412: break;
NOTREACHED?

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:526: // mAlwaysInTapRegion is
not reset. So when the last finger is up,
always_in_tap_region_

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:527: // onSingleTapUp() will be
mistakenly fired.
OnSingleTapUp()

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:580: TRACE_EVENT0("input",
"GestureProvider::OnTouchEvent");
Since you're changing this, wanna add tracing for the motion event type? I've
often found that useful when looking for touch events in traces. If we want,
later we could also add more disabled-by-default trace events for the internal
state of various classes here.

[jdduke (slow), 2014-02-21 20:59:11]

Thank you all!

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

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:70: // TODO: Need to deal with
multi-touch transition
On 2014/02/21 18:59:08, Sami wrote:
> TODO(jdduke) plus a '.'

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:117: bool
IsScaleGestureDetectionInProgress() {
On 2014/02/21 18:59:08, Sami wrote:
> Could be const

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:121: void
SetIgnoreDetectorEvents(bool value) {
On 2014/02/21 18:59:08, Sami wrote:
> Could be a simple setter: set_ignore_detector_events(...)

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:210: // Return true to indicate
that we want to handle touch
On 2014/02/21 18:59:08, Sami wrote:
> Add '.'

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:258: // Because we are passing
integers to webkit, this could introduce
On 2014/02/21 18:59:08, Sami wrote:
> s/webkit/blink :)

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:258: // Because we are passing
integers to webkit, this could introduce
On 2014/02/21 18:59:08, Sami wrote:
> s/webkit/blink :)

Hmm, as a follow-up, I wonder if we still need this? GestureScrollUpdate takes
floating point deltas.  I'm not sure how that interacts with the integral coords
of the WebGestureEvent though.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:296:
show_press_data.show_press.width = e.GetTouchMajor();
On 2014/02/21 18:59:08, Sami wrote:
> Is this right? width = major, height = width?

At the moment, we have yet to discover a device that provides non-zero touch
axis rotations.  I think being generous in the width/height (using TouchMajor)
is fine.  In fact, before we were simply using a fixed 48.f pixel constant =/ 
Adding a TODO to expose additional minor axis and rotation.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:412: break;
On 2014/02/21 18:59:08, Sami wrote:
> NOTREACHED?

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:526: // mAlwaysInTapRegion is
not reset. So when the last finger is up,
On 2014/02/21 18:59:08, Sami wrote:
> always_in_tap_region_

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:527: // onSingleTapUp() will be
mistakenly fired.
On 2014/02/21 18:59:08, Sami wrote:
> OnSingleTapUp()

Done.

https://codereview.chromium.org/171773012/diff/90001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:580: TRACE_EVENT0("input",
"GestureProvider::OnTouchEvent");
On 2014/02/21 18:59:08, Sami wrote:
> Since you're changing this, wanna add tracing for the motion event type? I've
> often found that useful when looking for touch events in traces. If we want,
> later we could also add more disabled-by-default trace events for the internal
> state of various classes here.

Yes!

[jdduke (slow), 2014-02-25 17:51:01]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-25 17:53:14]

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

[commit-bot: I haz the power, 2014-02-25 19:35:21]

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

[commit-bot: I haz the power, 2014-02-25 19:35:21]

Retried try job too often on win_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jdduke (slow), 2014-02-25 20:07:17]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-25 20:07:42]

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

[commit-bot: I haz the power, 2014-02-25 21:56:08]

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

[jdduke (slow), 2014-02-25 21:57:53]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:22:04]

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

[commit-bot: I haz the power, 2014-02-25 23:27:42]

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

[commit-bot: I haz the power, 2014-02-26 00:13:45]

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

[jdduke (slow), 2014-02-26 01:24:16]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2014-02-26 01:24:38]

The CQ bit was checked by jdduke@chromium.org

[Paweł Hajdan Jr., 2014-02-26 04:41:34]

The CQ bit was unchecked by phajdan.jr@chromium.org

[jdduke (slow), 2014-02-26 04:52:10]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2014-02-26 13:02:14]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2014-02-26 13:02:32]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2014-02-26 15:10:47]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2014-02-26 16:24:00]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-26 16:24:07]

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

[commit-bot: I haz the power, 2014-02-26 17:20:18]

Change committed as 253489