[Tracking Patch] Unified gesture detection

[Tracking Patch] Unified gesture detection

This is the master patch for unified gesture handling, including the following:
* Port of all Android platform gesture detection code to C++
* Removal of current ContentViewGestureHandler and associated code paths
* Wiring for Android usage in content/

Wiring for Aura and (potentially) other platforms will proceed separately.

BUG=332418

[jdduke (slow), 2014-01-08 18:52:42]

This isn't really for review, I just wanted to post the results of porting.

This does not include the "glue" code that we have in ContentViewGestureHandler,
but we should be able to simplify that code a good deal, and I'm not worried
about it porting properly.

This is 2800 lines, while ui/events/gestures has about 3500 lines.  All in all,
fairly comparable, and I don't think we'll break the (Aura) bank by switching.

[tdresser, 2014-01-08 19:18:21]

This is moving into ui, correct?

My primary concern is that we're removing flexibility from the aura gesture
recognition process. If we've decided that we're not going to modify any of this
ported code, we won't be able to do some of the things we've discussed doing in
the Aura GR.

One example that comes to mind: we've discussed switching to using circular slop
boundaries in Aura, and with this model, that wouldn't be feasible.

That's a fairly trivial and unimportant example, but I'm worried there may be
other cases where we'd like behavior to differ from the Android Gesture Detector
in ways that would require modifying these ported files.

I suppose there's a good chance that any of these modifications could be
performed by subclassing GestureDetector, but that could get ugly if we really
don't want to modify GestureDetector at all.

How badly do we want to keep the ported files in sync with their java
equivalents? If we do end up in a situation where we want greater flexibility
than is offered by GestureDetector, do you think it would be reasonable to make
non-cosmetic changes?

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gestures/bitset_32.h (right):

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gestures/bitset_32.h:84: // Gets the inde of
the specified bit in the set, which is the number of
inde -> index

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gestures/snap_scroll_controller.h
(right):

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gestures/snap_scroll_controller.h:15: //
Port of SnapScrollController.java from Chromium
Will we still need the java version once this has landed?

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gestures/zoom_manager.h (right):

https://codereview.chromium.org/128613003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gestures/zoom_manager.h:14: // * Changes to
this class should be cosmetic only, easing fork maintenance.
Will we still need the java version of ZoomManager once this has landed?

[jdduke (slow), 2014-01-08 19:19:24]

On 2014/01/08 19:18:21, tdresser wrote:
> This is moving into ui, correct?
> 

Yes, I meant to put that in the preamble.  This can be moved wherever we like,
there are no content/-specific dependencies.  I put it here only as a dumping
point for code.

[jdduke (slow), 2014-01-08 19:30:22]

On 2014/01/08 19:18:21, tdresser wrote:
> This is moving into ui, correct?
> 
> My primary concern is that we're removing flexibility from the aura gesture
> recognition process. If we've decided that we're not going to modify any of
this
> ported code, we won't be able to do some of the things we've discussed doing
in
> the Aura GR.
> 
> One example that comes to mind: we've discussed switching to using circular
slop
> boundaries in Aura, and with this model, that wouldn't be feasible.
> 
> That's a fairly trivial and unimportant example, but I'm worried there may be
> other cases where we'd like behavior to differ from the Android Gesture
Detector
> in ways that would require modifying these ported files.
> 
> I suppose there's a good chance that any of these modifications could be
> performed by subclassing GestureDetector, but that could get ugly if we really
> don't want to modify GestureDetector at all.
> 
> How badly do we want to keep the ported files in sync with their java
> equivalents? If we do end up in a situation where we want greater flexibility
> than is offered by GestureDetector, do you think it would be reasonable to
make
> non-cosmetic changes?

Minor changes like introducing a circular touch slop region would be just fine. 
The warnings about "cosmetic" changes are really there to keep people from
coming in and refactoring how the classes are wired.  As long as we keep the
classes in a state recognizable to the original fork, and document functional
differences, I think we'll be OK.

[tdresser, 2014-01-08 19:34:52]

On 2014/01/08 19:30:22, jdduke wrote:
> On 2014/01/08 19:18:21, tdresser wrote:
> > This is moving into ui, correct?
> > 
> > My primary concern is that we're removing flexibility from the aura gesture
> > recognition process. If we've decided that we're not going to modify any of
> this
> > ported code, we won't be able to do some of the things we've discussed doing
> in
> > the Aura GR.
> > 
> > One example that comes to mind: we've discussed switching to using circular
> slop
> > boundaries in Aura, and with this model, that wouldn't be feasible.
> > 
> > That's a fairly trivial and unimportant example, but I'm worried there may
be
> > other cases where we'd like behavior to differ from the Android Gesture
> Detector
> > in ways that would require modifying these ported files.
> > 
> > I suppose there's a good chance that any of these modifications could be
> > performed by subclassing GestureDetector, but that could get ugly if we
really
> > don't want to modify GestureDetector at all.
> > 
> > How badly do we want to keep the ported files in sync with their java
> > equivalents? If we do end up in a situation where we want greater
flexibility
> > than is offered by GestureDetector, do you think it would be reasonable to
> make
> > non-cosmetic changes?
> 
> Minor changes like introducing a circular touch slop region would be just
fine. 
> The warnings about "cosmetic" changes are really there to keep people from
> coming in and refactoring how the classes are wired.  As long as we keep the
> classes in a state recognizable to the original fork, and document functional
> differences, I think we'll be OK.

Sounds good.

[aelias_OOO_until_Jul13, 2014-01-08 19:35:03]

Our "fork" already looks quite different from the original Android class just by
virtue of being rewritten in C++ rather than Java.  I don't think it would
significantly make things worse to also introduce a bit of additional logic that
is only turned on on ChromeOS.  The important thing is for the behavior on
Android to remain the same as other platform apps.

[jdduke (slow), 2014-02-19 16:31:34]

tdresser@: I'd like to start review of ui/events/gesture_detection (per
sadrul@'s preference I opted not to move the original /gestures folder).  I'll
keep this as the tracking patch, and split it off when it's in landable shape.

As this is a Java port, there are many things that may seem peculiar about the
code (e.g., lack of gfx::Vector2dF/gfx::PointF usage).  I'm not really looking
for any structural comments here, more so glaring errors and/or explicit
Chromium naming style violations.  There will be ample time to clean up the code
into a proper C++ port (particularly with respect to gfx::Vector2dF usage
instead of x/y components littered everywhere...), but I'd prefer such cleanup
be done in a follow-up patch.

[jdduke (slow), 2014-02-19 16:53:22]

This patch is a beast, so I'm adding some additional reviewers.  As noted above,
I'd like this review to be primarily for glaring errors and/or Chromium style
violations in the porting process.  We'll be tidying it up to be more "C++-ish
(stack friendly)" as a follow-up.

skyostil@: ui/events/gesture_detection/gesture_detector.{h,cc}
wangxianzhu@:  ui/events/gesture_detection/scale_gesture_detector.{h,cc},
ui/events/gesture_detection/snap_scroll_controller.{h,cc}
tdresser@: ui/events/gesture_detection/gesture_config_helper*

BitSet32 and VelocityTracker{State} were already native Android code, so there's
less of a need for review there.

[tdresser, 2014-02-19 17:44:12]

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper.h (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014, throughout.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_android.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:22:
ViewConfiguration::GetTapTimeoutInMs());;
Getting a little ; happy in here.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:24:
ViewConfiguration::GetDoubleTapTimeoutInMs());;;
;;;

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:31:
ViewConfiguration::GetMaximumFlingVelocityInPixelsPerSecond();;
;;

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:46:
ViewConfiguration::GetMinScalingSpanInPixels();;
;;

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_aura.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:18:
GestureConfiguration::long_press_time_in_seconds());
Could be wrapped onto two lines, as could a few others below.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:19: config.tap_timeout
= ;
That is a scary ;.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:22:
config.double_tap_timeout = ;
;

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:28: int scaled_touch_slop;
I'm not too keen on the use of "scaled" in these names. Would a suffix
indicating the units be clearer?

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:15: // Double tap drag zoom
sensitive (speed).
sensitive -> sensitivity

[jdduke (slow), 2014-02-19 18:51:58]

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper.h (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/02/19 17:44:12, tdresser wrote:
> 2014, throughout.

Done.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_android.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:22:
ViewConfiguration::GetTapTimeoutInMs());;
On 2014/02/19 17:44:12, tdresser wrote:
> Getting a little ; happy in here.

Done.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:24:
ViewConfiguration::GetDoubleTapTimeoutInMs());;;
On 2014/02/19 17:44:12, tdresser wrote:
> ;;;

Done.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:31:
ViewConfiguration::GetMaximumFlingVelocityInPixelsPerSecond();;
On 2014/02/19 17:44:12, tdresser wrote:
> ;;

Oh dear...

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:46:
ViewConfiguration::GetMinScalingSpanInPixels();;
On 2014/02/19 17:44:12, tdresser wrote:
> ;;

Done.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_aura.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:19: config.tap_timeout
= ;
On 2014/02/19 17:44:12, tdresser wrote:
> That is a scary ;.

Haha, yeah, you'll notice I never bothered compiling this... also ran
clang-format on these files.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:22:
config.double_tap_timeout = ;
On 2014/02/19 17:44:12, tdresser wrote:
> ;

Done.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:28: int scaled_touch_slop;
On 2014/02/19 17:44:12, tdresser wrote:
> I'm not too keen on the use of "scaled" in these names. Would a suffix
> indicating the units be clearer?

Yeah, all of the scaled prefixes are remnants of the Android naming.  The
gesture detection is mostly scale agnostic; there are a few int's here and there
that we'll probably want to change if we want to properly support DIPs, but the
client is responsible for converting the output to the desired coordinate space.

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/128613003/diff/710001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:15: // Double tap drag zoom
sensitive (speed).
On 2014/02/19 17:44:12, tdresser wrote:
> sensitive -> sensitivity

Done.

[tdresser, 2014-02-19 19:42:27]

ui/events/gesture_detection/gesture_config_helper* LGTM.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_android.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:21:
config.tap_timeout =
That is some spectacularly inconsistent line breaking from clang-format.
Not that I think it's worth fussing with.

[Xianzhu, 2014-02-19 22:01:55]

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:19: const TimeDelta
TOUCH_STABILIZE_TIME = TimeDelta::FromMilliseconds(128);
Global variables of class type are forbidden
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...).
Could be 'int kTouchStabilizeTimeMs'.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:21: const float
SCALE_FACTOR = .5f;
s/SCALE_FACTOR/kScaleFactor/

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:343: float newAccepted =
(touch_upper_ + touch_lower_ + avg) / 3;
s/newAccepted/new_accepted/

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.h:50: virtual void
OnScaleEnd(const ScaleGestureDetector&) OVERRIDE;
DISALLOW_COPY_AND_ASSIGN?

[Sami, 2014-02-20 14:59:04]

Keep those scary patches coming Jared :) I had look at
ui/events/gesture_detection/gesture_detector.{h,cc} and while no bugs jumped
out, I added some stylistic comments.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:23:
GestureDetector::Config::Config()
Could you add a comment saying which device these defaults are from?

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:82: typedef void
(GestureDetector::*ReceiverMethod)();
This could be private, right?

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:95: 
Should we have a destructor that calls Stop() automatically?

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:158: // Determine focal point
nit: Comments should end with a period.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:177: // Cancel long press and
taps
Ditto.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:189: const int upIndex =
ev.GetActionIndex();
up_index

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:212: bool hadTapMessage =
timeout_handler_->HasTimeout(TAP);
had_tap_message

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:218: // This is a second tap
Add '.'

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:220: // Give a callback with the
first tap of the double-tap
Ditto.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:222: // Give a callback with
down event of the double-tap
Ditto.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:225: // This is a first tap
Ditto.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:254: // Give the move events of
the double-tap
Add '.'

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:257: const int delta_x =
(int)(focus_x - down_focus_x_);
static_cast<int> instead of C-casts.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:283: // Finally, give the up
event of the double-tap
Add '.'

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:295: // A fling must travel the
minimum tap distance
Add '.'

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:389: int delta_x =
(int)first_down.GetX() - (int)second_down.GetX();
Use static_cast<int> instead of C-style casts.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:25: base::TimeDelta
longpress_timeout;
We're a little inconsistent whether long press is two words (LongPress) or one
(longpress). I'm happy with either but not both :)

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:34: class OnGestureListener {
nit: OnXxxListener sounds weird in C++-land. I found only two such instances
currently in Chrome. How about calling these just XxxListener?

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:60: public OnDoubleTapListener {
public:?

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:61: // OnGestureListener
nit: Chrome tends to use "// Overridden from SomeClass:".

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:71: // OnDoubleTapListener
Ditto.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:95: void Init(Config config);
const Config&

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:103: const MotionEvent&
second_down);
Could be a const method?

[jdduke (slow), 2014-02-20 18:01:08]

Thanks Sami and Xianzhu! I went ahead and cleaned up some more
non-period-terminated comments and C-style casts.  I'll split of the
gesture_detection/ portion and land in a separate patch.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_android.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_android.cc:21:
config.tap_timeout =
On 2014/02/19 19:42:28, tdresser wrote:
> That is some spectacularly inconsistent line breaking from clang-format.
> Not that I think it's worth fussing with.

Hmm, you're right, and it's worth being consistent here (even if clang-format
disagrees.. there's not always a single correct answer =/).

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:23:
GestureDetector::Config::Config()
On 2014/02/20 14:59:05, Sami wrote:
> Could you add a comment saying which device these defaults are from?

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:82: typedef void
(GestureDetector::*ReceiverMethod)();
On 2014/02/20 14:59:05, Sami wrote:
> This could be private, right?

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:95: 
On 2014/02/20 14:59:05, Sami wrote:
> Should we have a destructor that calls Stop() automatically?

I believe the |timeout_timer_[i]| destructor will take care of that, but it's
probably good to be explicit.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:158: // Determine focal point
On 2014/02/20 14:59:05, Sami wrote:
> nit: Comments should end with a period.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:177: // Cancel long press and
taps
On 2014/02/20 14:59:05, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:189: const int upIndex =
ev.GetActionIndex();
On 2014/02/20 14:59:05, Sami wrote:
> up_index

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:212: bool hadTapMessage =
timeout_handler_->HasTimeout(TAP);
On 2014/02/20 14:59:05, Sami wrote:
> had_tap_message

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:218: // This is a second tap
On 2014/02/20 14:59:05, Sami wrote:
> Add '.'

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:220: // Give a callback with the
first tap of the double-tap
On 2014/02/20 14:59:05, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:222: // Give a callback with
down event of the double-tap
On 2014/02/20 14:59:05, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:225: // This is a first tap
On 2014/02/20 14:59:05, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:254: // Give the move events of
the double-tap
On 2014/02/20 14:59:05, Sami wrote:
> Add '.'

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:257: const int delta_x =
(int)(focus_x - down_focus_x_);
On 2014/02/20 14:59:05, Sami wrote:
> static_cast<int> instead of C-casts.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:283: // Finally, give the up
event of the double-tap
On 2014/02/20 14:59:05, Sami wrote:
> Add '.'

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:295: // A fling must travel the
minimum tap distance
On 2014/02/20 14:59:05, Sami wrote:
> Add '.'

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:389: int delta_x =
(int)first_down.GetX() - (int)second_down.GetX();
On 2014/02/20 14:59:05, Sami wrote:
> Use static_cast<int> instead of C-style casts.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:25: base::TimeDelta
longpress_timeout;
On 2014/02/20 14:59:05, Sami wrote:
> We're a little inconsistent whether long press is two words (LongPress) or one
> (longpress). I'm happy with either but not both :)

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:34: class OnGestureListener {
On 2014/02/20 14:59:05, Sami wrote:
> nit: OnXxxListener sounds weird in C++-land. I found only two such instances
> currently in Chrome. How about calling these just XxxListener?

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:60: public OnDoubleTapListener {
On 2014/02/20 14:59:05, Sami wrote:
> public:?

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:61: // OnGestureListener
On 2014/02/20 14:59:05, Sami wrote:
> nit: Chrome tends to use "// Overridden from SomeClass:".

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:71: // OnDoubleTapListener
On 2014/02/20 14:59:05, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:95: void Init(Config config);
On 2014/02/20 14:59:05, Sami wrote:
> const Config&

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:103: const MotionEvent&
second_down);
On 2014/02/20 14:59:05, Sami wrote:
> Could be a const method?

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:19: const TimeDelta
TOUCH_STABILIZE_TIME = TimeDelta::FromMilliseconds(128);
On 2014/02/19 22:01:56, Xianzhu wrote:
> Global variables of class type are forbidden
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...).
> Could be 'int kTouchStabilizeTimeMs'.

OK, it's a shame we can't use constexpr like the rest of Google =/

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:21: const float
SCALE_FACTOR = .5f;
On 2014/02/19 22:01:56, Xianzhu wrote:
> s/SCALE_FACTOR/kScaleFactor/

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.cc:343: float newAccepted =
(touch_upper_ + touch_lower_ + avg) / 3;
On 2014/02/19 22:01:56, Xianzhu wrote:
> s/newAccepted/new_accepted/

Done.

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/128613003/diff/790001/ui/events/gesture_detec...
ui/events/gesture_detection/scale_gesture_detector.h:50: virtual void
OnScaleEnd(const ScaleGestureDetector&) OVERRIDE;
On 2014/02/19 22:01:56, Xianzhu wrote:
> DISALLOW_COPY_AND_ASSIGN?

Hmm, as an empty class it's nice to leave that off (there's no harm in copying),
as otherwise I'd need to define a regular constructor for a class that in
general will be subclassed.

[jdduke (slow), 2014-02-20 18:14:12]

ui/events/gesture_detection changes have been split into
https://codereview.chromium.org/171773012/, so if there are additional comments
(for that portion of the code) please post them there.  Thanks.