Adopt ui::GestureConfiguration on Android

Adopt ui::GestureConfiguration on Android

Making GestureConfiguration as an interface to serve all the platforms,
so that Aura and Android push to and pull from the same data source,
GestureConfiguration, where gesture configuring parameters are.

BUG=339203
Committed: https://crrev.com/db8214afd49f75000a2bad0f16e0756bfcf54315
Cr-Commit-Position: refs/heads/master@{#301393}

[lanwei, 2014-10-02 19:32:41]

Patchset #1 (id:1) has been deleted

[lanwei, 2014-10-03 20:21:05]

Patchset #1 (id:20001) has been deleted

[lanwei, 2014-10-03 20:21:17]

Patchset #1 (id:40001) has been deleted

[lanwei, 2014-10-03 20:21:30]

Patchset #1 (id:60001) has been deleted

[lanwei, 2014-10-03 20:44:27]

Patchset #1 (id:80001) has been deleted

[lanwei, 2014-10-05 18:44:50]

Patchset #1 (id:100001) has been deleted

[lanwei, 2014-10-05 18:44:59]

Patchset #1 (id:120001) has been deleted

[lanwei, 2014-10-05 20:47:53]

Patchset #1 (id:140001) has been deleted

[lanwei, 2014-10-05 20:48:21]

Patchset #1 (id:160001) has been deleted

[lanwei, 2014-10-05 21:41:24]

lanwei@chromium.org changed reviewers:
+ jdduke@chromium.org, sadrul@chromium.org, tdresser@chromium.org

[jdduke (slow), 2014-10-06 15:50:51]

https://codereview.chromium.org/613373004/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:43: virtual float
min_scroll_velocity();
Mixing virtual and inline is a little confusing, particularly when you have
setters for all of these values but the setters may not take effect with any
given implementation.

We can always keep the inline setters/getters (as with the original
GestureConfiguration), and then, for Android, we would populate the initial
Instance() value with the values from Android's ViewConfiguration. What do you
think? Then we don't even need virtual getters.

[lanwei, 2014-10-07 21:22:33]

Patchset #1 (id:180001) has been deleted

[lanwei, 2014-10-21 15:04:44]

Sorry for the confusion, I closed the issue 654203008, and I upload to this one.

[jdduke (slow), 2014-10-21 15:17:13]

Thanks, this is a nice improvement!

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:14: class
GestureConfigurationAndroid : public GestureConfiguration {
Nit: This class can be put in an anonymous namespace, 

namespace {

class GestureConfigurationAndroid : ...

}  // namespace

GestureConfiguration* GestureConfiguration::GetInstance() {

...

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:26:
config.min_gesture_bounds_length = kMinGestureBoundsLengthDips;
Nit: Prefer non-member constants, so |k{Min,Max|GestureBoundsLengthDips| should
go above in an anonymous namespace.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:60: const
gfx::Display GetDisplay() {
Nit: I would get rid of both of these helper functions, instead, just cache a
|pixel_to_dip| constant at the top of the GestureConfigurationAndroid()
constructor.

[tdresser, 2014-10-21 15:21:58]

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:9:
GestureConfiguration::GestureConfiguration()
How many of these parameters are actually set in the same way for Android as for
Aura?
Anything that isn't duplicated, or isn't used in both, should probably be set in
the subclass constructors.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:30: // associated list of
prefs in gesture_prefs_aura.cc.
gesture_prefs_aura.cc doesn't exist anymore, so we should remove this reference
to it.
We could mention gesture_prefs_observer_factory_aura.cc, but the mapping between
these two sets of variables is much looser now, so I don't think its worth it.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:151: //
gesture_prefs_aura.cc. These two lists should be kept in sync.
Remove reference to gesture_prefs_aura.cc.

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_default.cc (right):

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_default.cc:11:
GestureConfiguration* GestureConfiguration::GetInstance() {
Why do we need a default which is different from both Android and Aura?

[jdduke (slow), 2014-10-21 16:02:06]

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_default.cc (right):

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_default.cc:11:
GestureConfiguration* GestureConfiguration::GetInstance() {
On 2014/10/21 15:21:58, tdresser wrote:
> Why do we need a default which is different from both Android and Aura?

This is used on Mac (devtools touch emulation).

[tdresser, 2014-10-21 16:04:43]

On 2014/10/21 16:02:06, jdduke wrote:
>
https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration_default.cc (right):
> 
>
https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_default.cc:11:
> GestureConfiguration* GestureConfiguration::GetInstance() {
> On 2014/10/21 15:21:58, tdresser wrote:
> > Why do we need a default which is different from both Android and Aura?
> 
> This is used on Mac (devtools touch emulation).

Hmmm.
That seems a little odd to me - shouldn't we use the Android config if we're
emulating mobile, and the Aura config otherwise?

Leaving it is definitely fine for now at least.

[jdduke (slow), 2014-10-21 16:15:12]

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_default.cc (right):

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_default.cc:11:
GestureConfiguration* GestureConfiguration::GetInstance() {
On 2014/10/21 16:02:06, jdduke wrote:
> On 2014/10/21 15:21:58, tdresser wrote:
> > Why do we need a default which is different from both Android and Aura?
> 
> This is used on Mac (devtools touch emulation).

Absolutely. There's a TODO in touch_emulator.cc to do just that, but I went
ahead and filed crbug.com/425586.

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

Oops, context for that reply...

On 2014/10/21 16:04:43, tdresser wrote:
> Hmmm.
> That seems a little odd to me - shouldn't we use the Android config if we're
> emulating mobile, and the Aura config otherwise?
> 
> Leaving it is definitely fine for now at least.

On 2014/10/21 16:15:12, jdduke wrote:
> Absolutely. There's a TODO in touch_emulator.cc to do just that, but I went
> ahead and filed crbug.com/425586.

[lanwei, 2014-10-21 20:52:32]

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:9:
GestureConfiguration::GestureConfiguration()
On 2014/10/21 15:21:57, tdresser wrote:
> How many of these parameters are actually set in the same way for Android as
for
> Aura?
> Anything that isn't duplicated, or isn't used in both, should probably be set
in
> the subclass constructors.

Done.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:30: // associated list of
prefs in gesture_prefs_aura.cc.
On 2014/10/21 15:21:57, tdresser wrote:
> gesture_prefs_aura.cc doesn't exist anymore, so we should remove this
reference
> to it.
> We could mention gesture_prefs_observer_factory_aura.cc, but the mapping
between
> these two sets of variables is much looser now, so I don't think its worth it.

Done.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:151: //
gesture_prefs_aura.cc. These two lists should be kept in sync.
On 2014/10/21 15:21:57, tdresser wrote:
> Remove reference to gesture_prefs_aura.cc.

Done.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:14: class
GestureConfigurationAndroid : public GestureConfiguration {
On 2014/10/21 15:17:12, jdduke wrote:
> Nit: This class can be put in an anonymous namespace, 
> 
> namespace {
> 
> class GestureConfigurationAndroid : ...
> 
> }  // namespace
> 
> GestureConfiguration* GestureConfiguration::GetInstance() {
> 
> ...

Done.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:26:
config.min_gesture_bounds_length = kMinGestureBoundsLengthDips;
On 2014/10/21 15:17:12, jdduke wrote:
> Nit: Prefer non-member constants, so |k{Min,Max|GestureBoundsLengthDips|
should
> go above in an anonymous namespace.

Done.

https://codereview.chromium.org/613373004/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:60: const
gfx::Display GetDisplay() {
On 2014/10/21 15:17:12, jdduke wrote:
> Nit: I would get rid of both of these helper functions, instead, just cache a
> |pixel_to_dip| constant at the top of the GestureConfigurationAndroid()
> constructor.

Done.

[jdduke (slow), 2014-10-22 00:45:12]

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:26: virtual
GestureDetector::Config DefaultGestureDetectorConfig();
See my other note. I think we can avoid having these as member methods if we add
a couple variables to GestureConfiguration, letting us use just one path for
creating the GestureProvider::Config from a GestureConfiguration instance.

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_aura.cc (right):

https://codereview.chromium.org/613373004/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_aura.cc:37:
config.swipe_enabled = true;
If we add |swipe_enabled|, |two_finger_tap_enabled| and
|gesture_begin_end_types_enabled| as members of GestureConfiguration, can we
then have just a single implementation of DefaultGestureProviderConfig(), 
DefaultGestureDetectorConfig() and  DefaultScaleGestureDetectorConfig()? I think
one of the primary motivations in this patch was allowing
GestureProvider::Config creation directly from a GestureConfiguration instance.

[lanwei, 2014-10-22 01:00:32]

Patchset #8 (id:340001) has been deleted

[lanwei, 2014-10-22 15:03:10]

Patchset #8 (id:360001) has been deleted

[tdresser, 2014-10-22 17:25:18]

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_aura.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_aura.cc:51: };
We use a comment to indicate ending namespaces, and put a blank line above the
brace.

"}  // namespace"

[jdduke (slow), 2014-10-22 17:31:34]

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:21: #if
defined(USE_AURA)
Could you add a

TODO(jdduke): Consolidate router configuration paths using
ui::GestureConfiguration.

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:27:
base::TimeDelta::FromMilliseconds(ui::GestureConfiguration::GetInstance()
To cut down on noise I would just cache the GestureConfiguration instance at the
top, then re-use it below.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:21:
min_gesture_bounds_length_(0),
It looks like we're missing a number of initializers, e.g.,
semi_long_press_time_in_ms_?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:22:
min_pinch_update_distance_in_pixels_(5),
min_pinch_update_distance_in_pixels_ should default to 0, and should be set
explicitly in Aura.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:60:
config.minimum_fling_velocity = min_scroll_velocity();
Can we rename min_scroll_velocity and fling_velocity_cap to make them more
consistent, e.g., min_fling_velocity and max_fling_velocity?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:63:
config.minimum_swipe_velocity = min_swipe_speed();
We use velocity elsewhere, can we use it here too instead of speed?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:22: GestureConfiguration();
I believe this constructor should be protected?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:168: gfx::Display display()
{ return display_; }
Display probably does not belong here.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:34:
set_max_distance_between_taps_for_double_tap(
Should we also explicitly disable two_finger_tap and swipe?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:51:
set_semi_long_press_time_in_ms(
Hmm, it seems a little odd to use the DoubleTap timeout for the semi_long_press,
what exactly is a semi_long_press on Aura?

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:56:
set_display(gfx::Screen::GetNativeScreen()->GetPrimaryDisplay());
Yeah, I think we should avoid caching the Display, instead we can fetch it on
demand using gfx::Screen::GetNativeScreen()->GetPrimaryDisplay() when creating
the GestureProvider::Config.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:59: float
raw_pixel_to_dip_ratio_;
Looks like this doesn't need to be a member variable.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:63: const float
kMinGestureBoundsLengthDips = 24.f;
These constants should be pulled out of the class (you can just put them above
the class definition).

[jdduke (slow), 2014-10-22 17:56:19]

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:64: float
max_distance_for_two_finger_tap_in_pixels() {
One last question. All of these properties have setters, but in theory we could
make the variables (or the setters) protected and the subclasses can set them.
How many of the properties can change dynamically? Ideally we would only expose
the ones that have to be manipulated at runtime (after GestureConfiguration
creation).

[tdresser, 2014-10-22 17:58:29]

On 2014/10/22 17:56:19, jdduke wrote:
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration.h (right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.h:64: float
> max_distance_for_two_finger_tap_in_pixels() {
> One last question. All of these properties have setters, but in theory we
could
> make the variables (or the setters) protected and the subclasses can set them.
> How many of the properties can change dynamically? Ideally we would only
expose
> the ones that have to be manipulated at runtime (after GestureConfiguration
> creation).

We might need to add an additional configuration for testing purposes, because
there are a bunch of tests that modify the parameters.
It shouldn't be too hard to just override the instance temporarily.

I don't think many of these should be modified outside of tests.

[lanwei, 2014-10-22 19:11:05]

Patchset #2 (id:220001) has been deleted

[lanwei, 2014-10-22 21:53:23]

Patchset #5 (id:300001) has been deleted

[lanwei, 2014-10-23 01:16:30]

Patchset #8 (id:410001) has been deleted

[lanwei, 2014-10-23 01:26:18]

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:21: #if
defined(USE_AURA)
On 2014/10/22 17:31:33, jdduke wrote:
> Could you add a
> 
> TODO(jdduke): Consolidate router configuration paths using
> ui::GestureConfiguration.

Done.

https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:27:
base::TimeDelta::FromMilliseconds(ui::GestureConfiguration::GetInstance()
On 2014/10/22 17:31:33, jdduke wrote:
> To cut down on noise I would just cache the GestureConfiguration instance at
the
> top, then re-use it below.

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:21:
min_gesture_bounds_length_(0),
On 2014/10/22 17:31:34, jdduke wrote:
> It looks like we're missing a number of initializers, e.g.,
> semi_long_press_time_in_ms_?

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:22:
min_pinch_update_distance_in_pixels_(5),
On 2014/10/22 17:31:33, jdduke wrote:
> min_pinch_update_distance_in_pixels_ should default to 0, and should be set
> explicitly in Aura.

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:60:
config.minimum_fling_velocity = min_scroll_velocity();
On 2014/10/22 17:31:33, jdduke wrote:
> Can we rename min_scroll_velocity and fling_velocity_cap to make them more
> consistent, e.g., min_fling_velocity and max_fling_velocity?

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:63:
config.minimum_swipe_velocity = min_swipe_speed();
On 2014/10/22 17:31:34, jdduke wrote:
> We use velocity elsewhere, can we use it here too instead of speed?

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:22: GestureConfiguration();
On 2014/10/22 17:31:34, jdduke wrote:
> I believe this constructor should be protected?

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:64: float
max_distance_for_two_finger_tap_in_pixels() {
On 2014/10/22 17:56:18, jdduke wrote:
> One last question. All of these properties have setters, but in theory we
could
> make the variables (or the setters) protected and the subclasses can set them.
> How many of the properties can change dynamically? Ideally we would only
expose
> the ones that have to be manipulated at runtime (after GestureConfiguration
> creation).

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:168: gfx::Display display()
{ return display_; }
On 2014/10/22 17:31:34, jdduke wrote:
> Display probably does not belong here.

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:34:
set_max_distance_between_taps_for_double_tap(
On 2014/10/22 17:31:34, jdduke wrote:
> Should we also explicitly disable two_finger_tap and swipe?

I feel if we already set false in the superclass, maybe it is fine that we do
not explicitly disable again.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:51:
set_semi_long_press_time_in_ms(
On 2014/10/22 17:31:34, jdduke wrote:
> Hmm, it seems a little odd to use the DoubleTap timeout for the
semi_long_press,
> what exactly is a semi_long_press on Aura?

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:56:
set_display(gfx::Screen::GetNativeScreen()->GetPrimaryDisplay());
On 2014/10/22 17:31:34, jdduke wrote:
> Yeah, I think we should avoid caching the Display, instead we can fetch it on
> demand using gfx::Screen::GetNativeScreen()->GetPrimaryDisplay() when creating
> the GestureProvider::Config.

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:59: float
raw_pixel_to_dip_ratio_;
On 2014/10/22 17:31:34, jdduke wrote:
> Looks like this doesn't need to be a member variable.

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:63: const float
kMinGestureBoundsLengthDips = 24.f;
On 2014/10/22 17:31:34, jdduke wrote:
> These constants should be pulled out of the class (you can just put them above
> the class definition).

Done.

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_aura.cc (right):

https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_aura.cc:51: };
On 2014/10/22 17:25:18, tdresser wrote:
> We use a comment to indicate ending namespaces, and put a blank line above the
> brace.
> 
> "}  // namespace"

Done.

[tdresser, 2014-10-23 14:08:22]

On 2014/10/23 01:26:18, lanwei wrote:
>
https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
> File content/browser/renderer_host/input/input_router_config_helper.cc
(right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
> content/browser/renderer_host/input/input_router_config_helper.cc:21: #if
> defined(USE_AURA)
> On 2014/10/22 17:31:33, jdduke wrote:
> > Could you add a
> > 
> > TODO(jdduke): Consolidate router configuration paths using
> > ui::GestureConfiguration.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/content/browser/rendere...
> content/browser/renderer_host/input/input_router_config_helper.cc:27:
> base::TimeDelta::FromMilliseconds(ui::GestureConfiguration::GetInstance()
> On 2014/10/22 17:31:33, jdduke wrote:
> > To cut down on noise I would just cache the GestureConfiguration instance at
> the
> > top, then re-use it below.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration.cc (right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.cc:21:
> min_gesture_bounds_length_(0),
> On 2014/10/22 17:31:34, jdduke wrote:
> > It looks like we're missing a number of initializers, e.g.,
> > semi_long_press_time_in_ms_?
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.cc:22:
> min_pinch_update_distance_in_pixels_(5),
> On 2014/10/22 17:31:33, jdduke wrote:
> > min_pinch_update_distance_in_pixels_ should default to 0, and should be set
> > explicitly in Aura.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.cc:60:
> config.minimum_fling_velocity = min_scroll_velocity();
> On 2014/10/22 17:31:33, jdduke wrote:
> > Can we rename min_scroll_velocity and fling_velocity_cap to make them more
> > consistent, e.g., min_fling_velocity and max_fling_velocity?
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.cc:63:
> config.minimum_swipe_velocity = min_swipe_speed();
> On 2014/10/22 17:31:34, jdduke wrote:
> > We use velocity elsewhere, can we use it here too instead of speed?
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration.h (right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.h:22:
GestureConfiguration();
> On 2014/10/22 17:31:34, jdduke wrote:
> > I believe this constructor should be protected?
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.h:64: float
> max_distance_for_two_finger_tap_in_pixels() {
> On 2014/10/22 17:56:18, jdduke wrote:
> > One last question. All of these properties have setters, but in theory we
> could
> > make the variables (or the setters) protected and the subclasses can set
them.
> > How many of the properties can change dynamically? Ideally we would only
> expose
> > the ones that have to be manipulated at runtime (after GestureConfiguration
> > creation).
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.h:168: gfx::Display
display()
> { return display_; }
> On 2014/10/22 17:31:34, jdduke wrote:
> > Display probably does not belong here.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration_android.cc (right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_android.cc:34:
> set_max_distance_between_taps_for_double_tap(
> On 2014/10/22 17:31:34, jdduke wrote:
> > Should we also explicitly disable two_finger_tap and swipe?
> 
> I feel if we already set false in the superclass, maybe it is fine that we do
> not explicitly disable again.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_android.cc:51:
> set_semi_long_press_time_in_ms(
> On 2014/10/22 17:31:34, jdduke wrote:
> > Hmm, it seems a little odd to use the DoubleTap timeout for the
> semi_long_press,
> > what exactly is a semi_long_press on Aura?
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_android.cc:56:
> set_display(gfx::Screen::GetNativeScreen()->GetPrimaryDisplay());
> On 2014/10/22 17:31:34, jdduke wrote:
> > Yeah, I think we should avoid caching the Display, instead we can fetch it
on
> > demand using gfx::Screen::GetNativeScreen()->GetPrimaryDisplay() when
creating
> > the GestureProvider::Config.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_android.cc:59: float
> raw_pixel_to_dip_ratio_;
> On 2014/10/22 17:31:34, jdduke wrote:
> > Looks like this doesn't need to be a member variable.
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_android.cc:63: const float
> kMinGestureBoundsLengthDips = 24.f;
> On 2014/10/22 17:31:34, jdduke wrote:
> > These constants should be pulled out of the class (you can just put them
above
> > the class definition).
> 
> Done.
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration_aura.cc (right):
> 
>
https://codereview.chromium.org/613373004/diff/390001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration_aura.cc:51: };
> On 2014/10/22 17:25:18, tdresser wrote:
> > We use a comment to indicate ending namespaces, and put a blank line above
the
> > brace.
> > 
> > "}  // namespace"
> 
> Done.

LGTM with nits.

[tdresser, 2014-10-23 14:26:22]

Oops, missed a couple of nits.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:31: // See
Formatting.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:30: float
raw_pixel_to_dip_ratio = 1.f /
Looks like this could use re-formatting.

[jdduke (slow), 2014-10-23 15:01:27]

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:25: GestureProvider::Config
DefaultGestureProviderConfig();
I'd prefer if this method could remain in gesture_config_helper.h (actually,
maybe we should rename that file to gesture_provider_config_helper.h). Then its
implementation would just query the singleton to fetch all the appropriate
values (caching the singleton so you don't have GetInstance() everywhere). That
would avoid the need for this class to know anything about a GestureProvider or
GestureDetector.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:104:
min_pinch_update_span_delta_ =
tdresser@: Is this really logic that we want here?

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:147: GestureDetector::Config
DefaultGestureDetectorConfig();
These |DefaultFooConfig()| methods could also go back into
gesture_provider_config_helper.cc.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:196: float
max_distance_for_two_finger_tap_in_pixels_;
It's a shame all of these references are in |pixels| when they're really DIPs...
maybe that's the convention on Aura (this comment is not really related to your
changes :)

[tdresser, 2014-10-23 15:14:07]

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:104:
min_pinch_update_span_delta_ =
On 2014/10/23 15:01:27, jdduke wrote:
> tdresser@: Is this really logic that we want here?

Doesn't look like we ever use min_pinch_update_distance_in_pixels, so we can
probably just get rid of it, and add a setter for min_pinch_update_span_delta.

We could then just initialize min_pinch_update_span_delta correctly in
GestureConfiguration's constructor based on this flag.

Does that sound reasonable?

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:196: float
max_distance_for_two_finger_tap_in_pixels_;
On 2014/10/23 15:01:26, jdduke wrote:
> It's a shame all of these references are in |pixels| when they're really
DIPs...
> maybe that's the convention on Aura (this comment is not really related to
your
> changes :)

Yeah, that definitely is a shame. Not a change I'd want to see in this patch
though.

[jdduke (slow), 2014-10-23 15:30:10]

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:104:
min_pinch_update_span_delta_ =
On 2014/10/23 15:14:07, tdresser wrote:
> On 2014/10/23 15:01:27, jdduke wrote:
> > tdresser@: Is this really logic that we want here?
> 
> Doesn't look like we ever use min_pinch_update_distance_in_pixels, so we can
> probably just get rid of it, and add a setter for min_pinch_update_span_delta.
> 
> We could then just initialize min_pinch_update_span_delta correctly in
> GestureConfiguration's constructor based on this flag.
> 
> Does that sound reasonable?
> 
> 

Sounds great.

[jdduke (slow), 2014-10-23 15:30:10]

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:104:
min_pinch_update_span_delta_ =
On 2014/10/23 15:14:07, tdresser wrote:
> On 2014/10/23 15:01:27, jdduke wrote:
> > tdresser@: Is this really logic that we want here?
> 
> Doesn't look like we ever use min_pinch_update_distance_in_pixels, so we can
> probably just get rid of it, and add a setter for min_pinch_update_span_delta.
> 
> We could then just initialize min_pinch_update_span_delta correctly in
> GestureConfiguration's constructor based on this flag.
> 
> Does that sound reasonable?
> 
> 

Sounds great.

[lanwei, 2014-10-23 19:04:07]

Patchset #9 (id:450001) has been deleted

[jdduke (slow), 2014-10-23 19:11:41]

https://codereview.chromium.org/613373004/diff/470001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider_config_helper.cc (right):

https://codereview.chromium.org/613373004/diff/470001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_config_helper.cc:12:
GestureConfiguration* gesture_config;
No need to cache this, just pass it as an arg to DefaultGestureDetectorConfig(),
etc...

[lanwei, 2014-10-23 19:54:19]

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:31: // See
On 2014/10/23 14:26:22, tdresser wrote:
> Formatting.

Done.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:25: GestureProvider::Config
DefaultGestureProviderConfig();
On 2014/10/23 15:01:27, jdduke wrote:
> I'd prefer if this method could remain in gesture_config_helper.h (actually,
> maybe we should rename that file to gesture_provider_config_helper.h). Then
its
> implementation would just query the singleton to fetch all the appropriate
> values (caching the singleton so you don't have GetInstance() everywhere).
That
> would avoid the need for this class to know anything about a GestureProvider
or
> GestureDetector.

Done.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:104:
min_pinch_update_span_delta_ =
On 2014/10/23 15:14:07, tdresser wrote:
> On 2014/10/23 15:01:27, jdduke wrote:
> > tdresser@: Is this really logic that we want here?
> 
> Doesn't look like we ever use min_pinch_update_distance_in_pixels, so we can
> probably just get rid of it, and add a setter for min_pinch_update_span_delta.
> 
> We could then just initialize min_pinch_update_span_delta correctly in
> GestureConfiguration's constructor based on this flag.
> 
> Does that sound reasonable?
> 
> 

Done.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:147: GestureDetector::Config
DefaultGestureDetectorConfig();
On 2014/10/23 15:01:26, jdduke wrote:
> These |DefaultFooConfig()| methods could also go back into
> gesture_provider_config_helper.cc.

Done.

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/430001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:30: float
raw_pixel_to_dip_ratio = 1.f /
On 2014/10/23 14:26:22, tdresser wrote:
> Looks like this could use re-formatting.

Done.

[jdduke (slow), 2014-10-23 20:17:01]

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:12: #include
"ui/events/gesture_detection/gesture_provider.h"
You an remove the gesture_detector/provider/scale_gesture_detector includes.

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:132: GestureDetector::Config
DefaultGestureDetectorConfig();
You can remove these declarations.

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:67: return
GestureConfigurationAndroid::GetInstance();
Can this just be Singleton<GestureConfigurationAndroid>::get();?

[lanwei, 2014-10-23 20:50:48]

Sorry that I did not clean up thoroughly.

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:12: #include
"ui/events/gesture_detection/gesture_provider.h"
On 2014/10/23 20:17:00, jdduke wrote:
> You an remove the gesture_detector/provider/scale_gesture_detector includes.

Done.

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:132: GestureDetector::Config
DefaultGestureDetectorConfig();
On 2014/10/23 20:17:00, jdduke wrote:
> You can remove these declarations.

Done.

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/490001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:67: return
GestureConfigurationAndroid::GetInstance();
On 2014/10/23 20:17:00, jdduke wrote:
> Can this just be Singleton<GestureConfigurationAndroid>::get();?

I tried before, it gave me this error
 error: 'get' is a private member of 'Singleton<ui::(anonymous
namespace)::GestureConfigurationAura, DefaultSingletonTraits<ui::(anonymous
namespace)::GestureConfigurationAura>, ui::(anonymous
namespace)::GestureConfigurationAura>'

I feel it can be only used inside methods in its own class.

[jdduke (slow), 2014-10-23 21:02:42]

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

[jdduke (slow), 2014-10-23 21:02:44]

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:15:
long_press_time_in_ms_(1000),
My only remaining concern is devtools touch emulation on Mac. As it stands, Mac
emulation would now be using these constants (instead of the
GestureDetector::Config defaults, which were taken from Android defaults) for
touch emulation. They're actually quite different for a number of these values
(e.g., long press on Android is 500ms, the double-tap distance is 100 instead of
20, etc...).

Maybe it's not as big an issue as now we have parity on desktop, but it's
something to keep in mind. dgozman@: Thoughts?

[dgozman, 2014-10-24 10:40:36]

On 2014/10/23 21:02:44, jdduke wrote:
>
https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_configuration.cc (right):
> 
>
https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_configuration.cc:15:
> long_press_time_in_ms_(1000),
> My only remaining concern is devtools touch emulation on Mac. As it stands,
Mac
> emulation would now be using these constants (instead of the
> GestureDetector::Config defaults, which were taken from Android defaults) for
> touch emulation. They're actually quite different for a number of these values
> (e.g., long press on Android is 500ms, the double-tap distance is 100 instead
of
> 20, etc...).
> 
> Maybe it's not as big an issue as now we have parity on desktop, but it's
> something to keep in mind. dgozman@: Thoughts?

I think it's important for devtools emulation to be as close as possible to
android when needed.
Can we instantiate different gesture detector configs in touch emulator (and
switch on the fly)?
We do have the "mobile/desktop" flag in RWHI when requesting touch emulation.
And the default config could be selected with ifdef (in
ui/events/gesture_detection/gesture_provider_config_helper.cc). What do you
think?

[jdduke (slow), 2014-10-24 15:21:04]

On 2014/10/24 10:40:36, dgozman wrote:
> On 2014/10/23 21:02:44, jdduke wrote:
> >
>
https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
> > File ui/events/gesture_detection/gesture_configuration.cc (right):
> > 
> >
>
https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
> > ui/events/gesture_detection/gesture_configuration.cc:15:
> > long_press_time_in_ms_(1000),
> > My only remaining concern is devtools touch emulation on Mac. As it stands,
> Mac
> > emulation would now be using these constants (instead of the
> > GestureDetector::Config defaults, which were taken from Android defaults)
for
> > touch emulation. They're actually quite different for a number of these
values
> > (e.g., long press on Android is 500ms, the double-tap distance is 100
instead
> of
> > 20, etc...).
> > 
> > Maybe it's not as big an issue as now we have parity on desktop, but it's
> > something to keep in mind. dgozman@: Thoughts?
> 
> I think it's important for devtools emulation to be as close as possible to
> android when needed.
> Can we instantiate different gesture detector configs in touch emulator (and
> switch on the fly)?
> We do have the "mobile/desktop" flag in RWHI when requesting touch emulation.
> And the default config could be selected with ifdef (in
> ui/events/gesture_detection/gesture_provider_config_helper.cc). What do you
> think?

Something like that should work. We actually have everything we need for
desktop/mobile defaults: Desktop defaults are in the GestureConfiguration
constructor, and mobile defaults are in the GestureProvider::Config constructor,
so it shouldn't be too crazy to support, say,

enum GestureProviderConfigType {
  CURRENT_PLATFORM_DEFAULT,
  DESKTOP_DEFAULT,
  MOBILE_DEFAULT
};
GetGestureProviderConfig(GestureProviderConfigType):

I'd be happy to implement that as an immediate follow-up to this patch.

[dgozman, 2014-10-24 15:38:22]

> Something like that should work. We actually have everything we need for
> desktop/mobile defaults: Desktop defaults are in the GestureConfiguration
> constructor, and mobile defaults are in the GestureProvider::Config
constructor,
> so it shouldn't be too crazy to support, say,
> 
> enum GestureProviderConfigType {
>   CURRENT_PLATFORM_DEFAULT,
>   DESKTOP_DEFAULT,
>   MOBILE_DEFAULT
> };
> GetGestureProviderConfig(GestureProviderConfigType):
> 
> I'd be happy to implement that as an immediate follow-up to this patch.

Great! I'll post a patch to add |mobile| parameter to |enableTouchEmulation|.

[jdduke (slow), 2014-10-24 15:50:00]

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:49: float
max_distance_for_two_finger_tap_in_pixels() {
Nit: All of the methods that return a value should be const, e.g., "float
max_distance_for_two_finger_tap_in_pixels() const {"

[lanwei, 2014-10-24 17:40:33]

lanwei@chromium.org changed reviewers:
- sadrul@chromium.org

[lanwei, 2014-10-24 17:43:42]

lanwei@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[lanwei, 2014-10-24 17:43:44]

sky@chromium.org: Please review changes in 

ash/
chrome/
content/
ui/aura

Thank you very much.

[sky, 2014-10-24 19:38:36]

https://codereview.chromium.org/613373004/diff/530001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/530001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:16: virtual
~GestureConfiguration();
Shouldn't this be protected? You don't want random people deleting the value
returned from GetInstance(). In fact you should really document ownership of
GetInstance().

[lanwei, 2014-10-26 17:51:48]

Patchset #13 (id:550001) has been deleted

[lanwei, 2014-10-26 17:51:54]

Patchset #13 (id:550001) has been deleted

[lanwei, 2014-10-27 14:00:34]

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/510001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:49: float
max_distance_for_two_finger_tap_in_pixels() {
On 2014/10/24 15:50:00, jdduke wrote:
> Nit: All of the methods that return a value should be const, e.g., "float
> max_distance_for_two_finger_tap_in_pixels() const {"

Done.

https://codereview.chromium.org/613373004/diff/530001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/530001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:16: virtual
~GestureConfiguration();
On 2014/10/24 19:38:36, sky wrote:
> Shouldn't this be protected? You don't want random people deleting the value
> returned from GetInstance(). In fact you should really document ownership of
> GetInstance().

Done.

[sky, 2014-10-27 14:47:44]

LGTM

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
2014 and no (c).

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:16: // Create an instance of
GestureConfiguration's subclass, depending on
Your comment here is confusing. This doesn't always create. I would like this
at:
"Returns the singleton GestureConfiguration."

[jdduke (slow), 2014-10-27 15:05:01]

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
For any new files the copyright notice should start with:

// Copyright 2014 ...

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:32:
min_scaling_touch_major_(0),
Let's give this a default of 50 (2 * default_radius_).

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:43: span_slop_(0),
Let's give this a default of 30 (2 *  max_touch_move_in_pixels_for_click_).

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:24:
~GestureConfigurationAndroid() {
~GestureConfigurationAndroid() override {
}

(also for other subclass destructors).

[lanwei, 2014-10-27 16:20:17]

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.cc (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2014/10/27 15:05:01, jdduke wrote:
> For any new files the copyright notice should start with:
> 
> // Copyright 2014 ...
 I thought that this file is moved from other place, maybe do not need to change
the copyright. I guess this situation also consider as new files. 
Done.

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:32:
min_scaling_touch_major_(0),
On 2014/10/27 15:05:01, jdduke wrote:
> Let's give this a default of 50 (2 * default_radius_).

Done.

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.cc:43: span_slop_(0),
On 2014/10/27 15:05:01, jdduke wrote:
> Let's give this a default of 30 (2 *  max_touch_move_in_pixels_for_click_).

Done.

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration.h (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/10/27 14:47:43, sky wrote:
> 2014 and no (c).

Done.

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration.h:16: // Create an instance of
GestureConfiguration's subclass, depending on
On 2014/10/27 14:47:43, sky wrote:
> Your comment here is confusing. This doesn't always create. I would like this
> at:
> "Returns the singleton GestureConfiguration."

Done.

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_configuration_android.cc (right):

https://codereview.chromium.org/613373004/diff/570001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_configuration_android.cc:24:
~GestureConfigurationAndroid() {
On 2014/10/27 15:05:01, jdduke wrote:
> ~GestureConfigurationAndroid() override {
> }
> 
> (also for other subclass destructors).

Done.

[jdduke (slow), 2014-10-27 16:25:06]

ui/events/gesture_detection lgtm.

[sadrul, 2014-10-27 16:27:21]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[sadrul, 2014-10-27 16:27:23]

All files I own already have other owner reviews. Removing myself

[lanwei, 2014-10-27 17:00:13]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2014-10-27 17:01:32]

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

[commit-bot: I haz the power, 2014-10-27 17:58:57]

Committed patchset #14 (id:590001)

[commit-bot: I haz the power, 2014-10-27 17:59:33]

Patchset 14 (id:??) landed as
https://crrev.com/db8214afd49f75000a2bad0f16e0756bfcf54315
Cr-Commit-Position: refs/heads/master@{#301393}