Add touchpad speed factor setting to Chrome OS touchpad settings page...

Add touchpad speed factor setting to Chrome OS touchpad settings page
Created slider widget with native gtk widget

[sky, 2009-09-16 23:52:19]

http://codereview.chromium.org/211002/diff/6001/6002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/211002/diff/6001/6002#newcode5052
Line 5052: Speed factor
As commented in person this term is a bit nebulous to me. Would be nice to get
something more descriptive, but that needn't be done now.

http://codereview.chromium.org/211002/diff/6001/6004
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/211002/diff/6001/6004#newcode476
Line 476: this);
Where do you set the slider to the current pref value?

http://codereview.chromium.org/211002/diff/6001/6005
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/211002/diff/6001/6005#newcode109
Line 109: // The value of MinSpeed should be a double between 0.01 and 1.00.
This comment is confusing.

http://codereview.chromium.org/211002/diff/6001/6005#newcode115
Line 115: SetSynclientParam("VertEdgeScroll", StringPrintf("%f", value));
Shouldn't this be something other than VertEdgeScroll?

http://codereview.chromium.org/211002/diff/6001/6013
File views/controls/slider/slider.h (right):

http://codereview.chromium.org/211002/diff/6001/6013#newcode8
Line 8: #if defined (OS_LINUX)
nit: no space between defined and the (

http://codereview.chromium.org/211002/diff/6001/6013#newcode17
Line 17: #ifdef UNIT_TEST
ifdefs for UNIT_TEST like this don't work.

http://codereview.chromium.org/211002/diff/6001/6013#newcode51
Line 51: explicit Slider(double min, double max, double step, StyleFlags style,
nit: no explicit.

http://codereview.chromium.org/211002/diff/6001/6013#newcode52
Line 52: SliderListener* listener);
align with ( on previous line.

http://codereview.chromium.org/211002/diff/6001/6013#newcode96
Line 96: NativeSliderWrapper* native_wrapper_;
make this private.

http://codereview.chromium.org/211002/diff/6001/6013#newcode99
Line 99: SliderListener* listener_;
can this be private?

http://codereview.chromium.org/211002/diff/6001/6009
File views/views.gyp (right):

http://codereview.chromium.org/211002/diff/6001/6009#newcode173
Line 173: 'controls/slider/slider.cc',
As we haven't implemented slider on windows, you should exclude slider and
native_slider_wrapper on windows.

[Charlie Lee (do not use), 2009-09-17 00:08:30]

Please take another look. Thanks.

http://codereview.chromium.org/211002/diff/6001/6002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/211002/diff/6001/6002#newcode5052
Line 5052: Speed factor
On 2009/09/16 23:52:19, sky wrote:
> As commented in person this term is a bit nebulous to me. Would be nice to get
> something more descriptive, but that needn't be done now.

Done.

http://codereview.chromium.org/211002/diff/6001/6004
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/211002/diff/6001/6004#newcode476
Line 476: this);
On 2009/09/16 23:52:19, sky wrote:
> Where do you set the slider to the current pref value?

We add the this as a listener to the pref. So on Init below, we get a pref
change notification which will set the value.

http://codereview.chromium.org/211002/diff/6001/6005
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/211002/diff/6001/6005#newcode109
Line 109: // The value of MinSpeed should be a double between 0.01 and 1.00.
On 2009/09/16 23:52:19, sky wrote:
> This comment is confusing.

Done.

http://codereview.chromium.org/211002/diff/6001/6005#newcode115
Line 115: SetSynclientParam("VertEdgeScroll", StringPrintf("%f", value));
On 2009/09/16 23:52:19, sky wrote:
> Shouldn't this be something other than VertEdgeScroll?

Done.

http://codereview.chromium.org/211002/diff/6001/6013
File views/controls/slider/slider.h (right):

http://codereview.chromium.org/211002/diff/6001/6013#newcode8
Line 8: #if defined (OS_LINUX)
On 2009/09/16 23:52:19, sky wrote:
> nit: no space between defined and the (

Done.

http://codereview.chromium.org/211002/diff/6001/6013#newcode17
Line 17: #ifdef UNIT_TEST
On 2009/09/16 23:52:19, sky wrote:
> ifdefs for UNIT_TEST like this don't work.

Done.

http://codereview.chromium.org/211002/diff/6001/6013#newcode51
Line 51: explicit Slider(double min, double max, double step, StyleFlags style,
On 2009/09/16 23:52:19, sky wrote:
> nit: no explicit.

Done.

http://codereview.chromium.org/211002/diff/6001/6013#newcode52
Line 52: SliderListener* listener);
On 2009/09/16 23:52:19, sky wrote:
> align with ( on previous line.

Done.

http://codereview.chromium.org/211002/diff/6001/6013#newcode96
Line 96: NativeSliderWrapper* native_wrapper_;
On 2009/09/16 23:52:19, sky wrote:
> make this private.

Done.

http://codereview.chromium.org/211002/diff/6001/6013#newcode99
Line 99: SliderListener* listener_;
On 2009/09/16 23:52:19, sky wrote:
> can this be private?

Done.

http://codereview.chromium.org/211002/diff/6001/6009
File views/views.gyp (right):

http://codereview.chromium.org/211002/diff/6001/6009#newcode173
Line 173: 'controls/slider/slider.cc',
On 2009/09/16 23:52:19, sky wrote:
> As we haven't implemented slider on windows, you should exclude slider and
> native_slider_wrapper on windows.

Done.

[sky, 2009-09-17 00:20:08]

LGTM with the following changes. If you upload another patch I'll land it for
you.

http://codereview.chromium.org/211002/diff/1025/60
File views/views.gyp (right):

http://codereview.chromium.org/211002/diff/1025/60#newcode332
Line 332: 'controls/slider/native_slider_gtk.cc',
No need to exclude the gtk files here, that is done automatically for you.

[Charlie Lee (do not use), 2009-09-17 01:40:44]

Thanks. Please land this for me.

http://codereview.chromium.org/211002/diff/1025/60
File views/views.gyp (right):

http://codereview.chromium.org/211002/diff/1025/60#newcode332
Line 332: 'controls/slider/native_slider_gtk.cc',
On 2009/09/17 00:20:09, sky wrote:
> No need to exclude the gtk files here, that is done automatically for you.

Done.