Gesture recognition constants should all be stored in the GestureConfiguration object.

Gesture recognition constants should all be stored in the GestureConfiguration object.

BUG=119086
TEST=GestureRecognizerTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129649

[girard, 2012-03-20 18:30:59]

Looks good, except for the buffer size variable.  (I don't think there's any
useful purpose for the user modifying that value.... is there?)
[I'm not a reviewer.]

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/gesture_prefs_aura.cc (right):

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:47: prefs::kLongPressTimeInMs,
It's unfortunate to have some values measured in seconds, and others measured in
milliseconds.  Since we're using a double, we have the option of being
consistent.

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:57:
prefs::kPointsBufferedForVelocity,
I don't think kPointsBufferedForVelocity is a "user configurable" value. 
Changing it breaks unit tests (I've tried this!), and will probably damage our
algorithms/logic.

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:132: local_state_->GetInteger(
Surprising that this is an integer.

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/gestures/ges...
File ui/aura/gestures/gesture_point.cc (right):

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/gestures/ges...
ui/aura/gestures/gesture_point.cc:21:
GestureConfiguration::points_buffered_for_velocity()),
As per my other comment.  This should not be configurable.

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/test/aura_te...
File ui/aura/test/aura_test_base.cc (right):

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/test/aura_te...
ui/aura/test/aura_test_base.cc:29: // Changing the parameters for gesture
recognition shouldn't cause
Is this necessary? Are the configured parameters used in these unit tests?
Maybe we could modify the specific unit tests to set these as needed, or modify
their logic to use the current value?

[tdresser, 2012-03-20 19:34:10]

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/gesture_prefs_aura.cc (right):

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:47: prefs::kLongPressTimeInMs,
On 2012/03/20 18:30:59, girard wrote:
> It's unfortunate to have some values measured in seconds, and others measured
in
> milliseconds.  Since we're using a double, we have the option of being
> consistent.

Done.

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:57:
prefs::kPointsBufferedForVelocity,
On 2012/03/20 18:30:59, girard wrote:
> I don't think kPointsBufferedForVelocity is a "user configurable" value. 
> Changing it breaks unit tests (I've tried this!), and will probably damage our
> algorithms/logic.

kPointsBufferedForVelocity affects the feel of the fling gesture, so I think it
should be configurable.

Unit tests will use a separate value, so modifying it won't break any tests.

https://chromiumcodereview.appspot.com/9751011/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:132: local_state_->GetInteger(
On 2012/03/20 18:30:59, girard wrote:
> Surprising that this is an integer.
Switched to a storing in seconds, as a double.

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/test/aura_te...
File ui/aura/test/aura_test_base.cc (right):

https://chromiumcodereview.appspot.com/9751011/diff/8001/ui/aura/test/aura_te...
ui/aura/test/aura_test_base.cc:29: // Changing the parameters for gesture
recognition shouldn't cause
On 2012/03/20 18:30:59, girard wrote:
> Is this necessary? Are the configured parameters used in these unit tests?
> Maybe we could modify the specific unit tests to set these as needed, or
modify
> their logic to use the current value?

It seems beneficial to isolate changes to GestureConfiguration as much as
possible. Although this may be overkill, it guarantees that changes to
GestureConfiguration will never break unit tests.

It would be easy for a future unit test author to forget to specify all relevant
attributes of GestureConfiguration, causing future modifications to
GestureConfiguration to break unit tests.

[sadrul, 2012-03-26 19:56:59]

LGTM

http://codereview.chromium.org/9751011/diff/14001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/gesture_prefs_aura.cc (right):

http://codereview.chromium.org/9751011/diff/14001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/gesture_prefs_aura.cc:169:
prefs::kRailStartProportion));
General comment: this is fairly icky. Updating all the configs when just one may
have changed is ugly. But that's not an issue with this CL. Regardless, this
should probably be cleaned up in some future CL.

[sky, 2012-03-28 20:52:05]

LGTM

[commit-bot: I haz the power, 2012-03-29 15:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/9751011/31001

[commit-bot: I haz the power, 2012-03-29 18:19:20]

Change committed as 129649