Added ChromeOS settings for touchpad

First pass at adding ChromeOS settings
- a mock wifi selector combobox
- touchpad settings that makes calls to synclient
- on startup, touchpad settings are initialized to what's stored in preferences

[sky, 2009-09-14 17:18:47]

http://codereview.chromium.org/193032/diff/12001/12002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/193032/diff/12001/12002#newcode4496
Line 4496: <message name="IDS_OPTIONS_SETTINGS_SECTION_TITLE_NETWORK">
These should only be defined for ChromeOS. See theme_resources.grd for an
example of defining strings for ChromeOS.

http://codereview.chromium.org/193032/diff/12001/12003
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/193032/diff/12001/12003#newcode830
Line 830: // Initialized touchpad settings to what's saved in user preferences
nit: initialized -> initialize. And end your sentence with a period.

http://codereview.chromium.org/193032/diff/12001/12005
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/193032/diff/12001/12005#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
The string story in Chrome is in transition now. At some point everything should
be string16. You should try to use string16 for everything, and only converting
to wide for existing methods.

http://codereview.chromium.org/193032/diff/12001/12005#newcode33
Line 33: class WifiSSIDComboModel : public ComboboxModel {
Description?

http://codereview.chromium.org/193032/diff/12001/12005#newcode38
Line 38: : encryption(encryption), strength(strength) { }
Style guide says that once you wrap, each member must be on a newline.

http://codereview.chromium.org/193032/diff/12001/12005#newcode43
Line 43: typedef std::map<std::wstring, NetworkData> NetworkDataMap;
include map.

http://codereview.chromium.org/193032/diff/12001/12005#newcode50
Line 50: std::wstring GetSSIDAt(int index);
Can this return const std::wstring& ?

http://codereview.chromium.org/193032/diff/12001/12005#newcode51
Line 51: bool RequiresPassword(std::wstring ssid);
const std::wstring&

http://codereview.chromium.org/193032/diff/12001/12005#newcode54
Line 54: std::vector<std::wstring> ssids_;
include vector.

http://codereview.chromium.org/193032/diff/12001/12005#newcode60
Line 60: const std::wstring& encryption, const int& strength);
nit: align with ( or indent 4.
const int& strength -> int strength

http://codereview.chromium.org/193032/diff/12001/12005#newcode67
Line 67: AddWifiNetwork(L"Wifi Combobox Mock", L"", 80);
nit: "" -> std::wstring()

http://codereview.chromium.org/193032/diff/12001/12005#newcode69
Line 69: AddWifiNetwork(L"Wifi No Encryption", L"", 90);
Should these be localized? If not, can you add a comment to that effect.

http://codereview.chromium.org/193032/diff/12001/12005#newcode69
Line 69: AddWifiNetwork(L"Wifi No Encryption", L"", 90);
nit: "" -> std::wstring()

http://codereview.chromium.org/193032/diff/12001/12005#newcode83
Line 83: it->second.encryption : L"None";
localize

http://codereview.chromium.org/193032/diff/12001/12005#newcode85
Line 85: return it->first + L" (" + encryption + L", " + strength +  L")";
Use ReplaceStringPlacholders along with a format.

http://codereview.chromium.org/193032/diff/12001/12005#newcode106
Line 106: class NetworkSection : public OptionsPageView,
Description.

http://codereview.chromium.org/193032/diff/12001/12005#newcode107
Line 107: public views::Combobox::Listener {
nit: align public views:: with previous public.

http://codereview.chromium.org/193032/diff/12001/12005#newcode133
Line 133: // Used to store the index (in combobox) of the currently connected
wifi
End with a '.'.

http://codereview.chromium.org/193032/diff/12001/12005#newcode134
Line 134: int current_wifi_ssid_index_;
The way you use this looks more like previous than current, or perhaps
last_selected_...

http://codereview.chromium.org/193032/diff/12001/12005#newcode137
Line 137: scoped_ptr<WifiSSIDComboModel> wifi_ssid_model_;
Any reason this is a scoped ptr and not by value?

http://codereview.chromium.org/193032/diff/12001/12005#newcode142
Line 142: NetworkSection::NetworkSection(Profile* profile)
Group with other NetworkSection members.
Be sure and initialize current_wifi_ssid_index_.

http://codereview.chromium.org/193032/diff/12001/12005#newcode148
Line 148: class PasswordWindowView : public views::View,
Description.

http://codereview.chromium.org/193032/diff/12001/12005#newcode175
Line 175: static const int kDialogPadding = 7;
Keep definition and declaration separate for fields.

http://codereview.chromium.org/193032/diff/12001/12005#newcode191
Line 191: Profile* profile_;
Doesn't look like you need this.

http://codereview.chromium.org/193032/diff/12001/12005#newcode214
Line 214: password_textfield_->SyncText();
You shouldn't need to invoke SyncText.

http://codereview.chromium.org/193032/diff/12001/12005#newcode229
Line 229: return gfx::Size(font.ave_char_width() * kDefaultWindowWidthChars,
Do you really want 3 rows here?

http://codereview.chromium.org/193032/diff/12001/12005#newcode237
Line 237: // a HWND to parent native child controls to.
HWND comment isn't right here.

http://codereview.chromium.org/193032/diff/12001/12005#newcode263
Line 263: ConnectToWifi(ssid, L"");
nit: "" -> std::wstring()

http://codereview.chromium.org/193032/diff/12001/12005#newcode282
Line 282: DLOG(ERROR) << "Trying to connect to '" << ssid <<
No DLOG. I think you want a TODO here.

http://codereview.chromium.org/193032/diff/12001/12005#newcode291
Line 291: const int single_column_layout_id = 0;
nit: don't use const here.

http://codereview.chromium.org/193032/diff/12001/12005#newcode324
Line 324: const int single_column_view_set_id = 0;
nit: no const

http://codereview.chromium.org/193032/diff/12001/12005#newcode334
Line 334:
////////////////////////////////////////////////////////////////////////////////
It would be nice if you used separators like this between all classes.

http://codereview.chromium.org/193032/diff/12001/12005#newcode421
Line 421: }
Spacing is off for the }, and starting void on 423.

http://codereview.chromium.org/193032/diff/12001/12005#newcode437
Line 437: const int single_column_view_set_id = 0;
nit: no const.

http://codereview.chromium.org/193032/diff/12001/12005#newcode473
Line 473: int SettingsContentsView::line_height_ = 0;
It doesn't seem like you use this.

http://codereview.chromium.org/193032/diff/12001/12005#newcode492
Line 492: const int width = parent->width();
no const.

http://codereview.chromium.org/193032/diff/12001/12005#newcode494
Line 494: SetBounds(0, 0, width, height);
Do you really need this code?

http://codereview.chromium.org/193032/diff/12001/12005#newcode531
Line 531: static bool initialized = false;
I suspect you can get rid of this whole method.

http://codereview.chromium.org/193032/diff/12001/12006
File chrome/browser/chromeos/settings_contents_view.h (right):

http://codereview.chromium.org/193032/diff/12001/12006#newcode10
Line 10: class SettingsContentsView : public OptionsPageView {
Description.

http://codereview.chromium.org/193032/diff/12001/12009
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/193032/diff/12001/12009#newcode20
Line 20: void Touchpad::SetTapToClick(bool taptoclick) {
nit: for statics we typically have
// static
before the method.

http://codereview.chromium.org/193032/diff/12001/12009#newcode28
Line 28: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
You need to make sure the io_thread is non-null.

http://codereview.chromium.org/193032/diff/12001/12009#newcode44
Line 44: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
Make sure io_thread is non-null.

http://codereview.chromium.org/193032/diff/12001/12016
File views/window/dialog_client_view.cc (right):

http://codereview.chromium.org/193032/diff/12001/12016#newcode278
Line 278: window_style->bg[GTK_STATE_NORMAL].red >> 8,
Use GdkColorToSkColor (which needs the fix too). It's in
src/skia/ext/skia_utils_gtk.

[tony, 2009-09-14 17:37:40]

drive-by-suggestions

http://codereview.chromium.org/193032/diff/12001/12005
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/193032/diff/12001/12005#newcode383
Line 383: Touchpad::SetTapToClick(enabled);
Rather than having to remember to call this, Touchpad could be a class that has
pref observers that watch for the pref to change.  When the pref changes, it
updates the setting.  The PrefMember class does a lot of this for you.

http://codereview.chromium.org/193032/diff/12001/12010
File chrome/browser/chromeos/touchpad.h (right):

http://codereview.chromium.org/193032/diff/12001/12010#newcode10
Line 10: static void SetTapToClick(bool taptoclick);
I would just make this a real class with PrefMembers watching the 2 prefs. 
It'll update state when the prefs change.  Maybe ProfileImpl can own this class?
 That has the side benefit of not adding more stuff to the already complicated
browser_main.

http://codereview.chromium.org/193032/diff/12001/12004
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/193032/diff/12001/12004#newcode145
Line 145: prefs->RegisterBooleanPref(prefs::kTapToClickEnabled, true);
Nit: Please register prefs in a file that uses the prefs.  I.e., can you add a
RegisterPrefs method to touchpad.cc?

[Charlie Lee (do not use), 2009-09-14 22:40:08]

Scott and Tony, thanks for the comments.
I made the changes and re-uploaded. I have a few questions below though.
- Charlie


On Mon, Sep 14, 2009 at 10:18 AM, <sky@chromium.org> wrote:

>
> http://codereview.chromium.org/193032/diff/12001/12002
> File chrome/app/generated_resources.grd (right):
>
> http://codereview.chromium.org/193032/diff/12001/12002#newcode4496
> Line 4496: <message name="IDS_OPTIONS_SETTINGS_SECTION_TITLE_NETWORK">
> These should only be defined for ChromeOS. See theme_resources.grd for
> an example of defining strings for ChromeOS.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12003
> File chrome/browser/browser_main.cc (right):
>
> http://codereview.chromium.org/193032/diff/12001/12003#newcode830
> Line 830: // Initialized touchpad settings to what's saved in user
> preferences
> nit: initialized -> initialize. And end your sentence with a period.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005
> File chrome/browser/chromeos/settings_contents_view.cc (right):
>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode1
> Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
> The string story in Chrome is in transition now. At some point
> everything should be string16. You should try to use string16 for
> everything, and only converting to wide for existing methods.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode33
> Line 33: class WifiSSIDComboModel : public ComboboxModel {
> Description?
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode38
> Line 38: : encryption(encryption), strength(strength) { }
> Style guide says that once you wrap, each member must be on a newline.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode43
> Line 43: typedef std::map<std::wstring, NetworkData> NetworkDataMap;
> include map.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode50
> Line 50: std::wstring GetSSIDAt(int index);
> Can this return const std::wstring& ?
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode51
> Line 51: bool RequiresPassword(std::wstring ssid);
> const std::wstring&
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode54
> Line 54: std::vector<std::wstring> ssids_;
> include vector.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode60
> Line 60: const std::wstring& encryption, const int& strength);
> nit: align with ( or indent 4.
> const int& strength -> int strength
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode67
> Line 67: AddWifiNetwork(L"Wifi Combobox Mock", L"", 80);
> nit: "" -> std::wstring()
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode69
> Line 69: AddWifiNetwork(L"Wifi No Encryption", L"", 90);
> Should these be localized? If not, can you add a comment to that effect.
>
Nope, just temporary data. Added comment.

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode69
> Line 69: AddWifiNetwork(L"Wifi No Encryption", L"", 90);
> nit: "" -> std::wstring()
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode83
> Line 83: it->second.encryption : L"None";
> localize
>
Temporary string. Added comment

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode85
> Line 85: return it->first + L" (" + encryption + L", " + strength +
> L")";
> Use ReplaceStringPlacholders along with a format.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode106
> Line 106: class NetworkSection : public OptionsPageView,
> Description.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode107
> Line 107: public views::Combobox::Listener {
> nit: align public views:: with previous public.
>
Oops. Done.

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode133
> Line 133: // Used to store the index (in combobox) of the currently
> connected wifi
> End with a '.'.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode134
> Line 134: int current_wifi_ssid_index_;
> The way you use this looks more like previous than current, or perhaps
> last_selected_...
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode137
> Line 137: scoped_ptr<WifiSSIDComboModel> wifi_ssid_model_;
> Any reason this is a scoped ptr and not by value?
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode142
> Line 142: NetworkSection::NetworkSection(Profile* profile)
> Group with other NetworkSection members.
> Be sure and initialize current_wifi_ssid_index_.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode148
> Line 148: class PasswordWindowView : public views::View,
> Description.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode175
> Line 175: static const int kDialogPadding = 7;
> Keep definition and declaration separate for fields.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode191
> Line 191: Profile* profile_;
> Doesn't look like you need this.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode214
> Line 214: password_textfield_->SyncText();
> You shouldn't need to invoke SyncText.
>
Without calling SynText, the text is not correct. Is this a bug with
textfield?

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode229
> Line 229: return gfx::Size(font.ave_char_width() *
> kDefaultWindowWidthChars,
> Do you really want 3 rows here?
>
Yes, so that the textfield and the buttons are visible.

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode237
> Line 237: // a HWND to parent native child controls to.
> HWND comment isn't right here.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode263
> Line 263: ConnectToWifi(ssid, L"");
> nit: "" -> std::wstring()
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode282
> Line 282: DLOG(ERROR) << "Trying to connect to '" << ssid <<
> No DLOG. I think you want a TODO here.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode291
> Line 291: const int single_column_layout_id = 0;
> nit: don't use const here.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode324
> Line 324: const int single_column_view_set_id = 0;
> nit: no const
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode334
> Line 334:
>
>
////////////////////////////////////////////////////////////////////////////////
> It would be nice if you used separators like this between all classes.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode421
> Line 421: }
> Spacing is off for the }, and starting void on 423.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode437
> Line 437: const int single_column_view_set_id = 0;
> nit: no const.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode473
> Line 473: int SettingsContentsView::line_height_ = 0;
> It doesn't seem like you use this.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode492
> Line 492: const int width = parent->width();
> no const.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode494
> Line 494: SetBounds(0, 0, width, height);
> Do you really need this code?
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12005#newcode531
> Line 531: static bool initialized = false;
> I suspect you can get rid of this whole method.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12006
> File chrome/browser/chromeos/settings_contents_view.h (right):
>
> http://codereview.chromium.org/193032/diff/12001/12006#newcode10
> Line 10: class SettingsContentsView : public OptionsPageView {
> Description.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12009
> File chrome/browser/chromeos/touchpad.cc (right):
>
> http://codereview.chromium.org/193032/diff/12001/12009#newcode20
> Line 20: void Touchpad::SetTapToClick(bool taptoclick) {
> nit: for statics we typically have
> // static
> before the method.
>
Done

>
> http://codereview.chromium.org/193032/diff/12001/12009#newcode28
> Line 28:
> g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
> You need to make sure the io_thread is non-null.
>
When will it ever be null? What do I do if it's null? Should I just make the
system call on the current thread?

>
> http://codereview.chromium.org/193032/diff/12001/12009#newcode44
> Line 44:
> g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
> Make sure io_thread is non-null.
>
Done.

>
> http://codereview.chromium.org/193032/diff/12001/12016
> File views/window/dialog_client_view.cc (right):
>
> http://codereview.chromium.org/193032/diff/12001/12016#newcode278
> Line 278: window_style->bg[GTK_STATE_NORMAL].red >> 8,
> Use GdkColorToSkColor (which needs the fix too). It's in
> src/skia/ext/skia_utils_gtk.

Ok. In skia_utils_gtk, I see code that multiplies by 257 for the conversion.
'>> 8' is equivalent to divide by 256. So which is right?

>
>
> http://codereview.chromium.org/193032
>

[tony, 2009-09-14 22:57:06]

http://codereview.chromium.org/193032/diff/3013/3021
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/193032/diff/3013/3021#newcode17
Line 17: prefs->RegisterBooleanPref(prefs::kTapToClickEnabled, true);
Nit: This is fine because we only create a single instance of Touchpad, but
multiple instances would cause a DCHECK (for reregistering).  Normally we just
add a static method to the class and call it from
chrome/browser/browser_prefs.cc.

http://codereview.chromium.org/193032/diff/3013/3021#newcode57
Line 57: if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
Should this use the file thread or the I/O thread?  I would think it should be
the file thread since it calls out to disk.

http://codereview.chromium.org/193032/diff/3013/3021#newcode65
Line 65: sprintf(command, "/usr/bin/synclient %s=%s", param, value);
Nit: StringPrintf from base/string_util.h

http://codereview.chromium.org/193032/diff/3013/3021#newcode66
Line 66: system(command);
Please use base::LaunchApp.  system calls out to the shell which seems
unnecessary here.

http://codereview.chromium.org/193032/diff/3013/3022
File chrome/browser/chromeos/touchpad.h (right):

http://codereview.chromium.org/193032/diff/3013/3022#newcode5
Line 5: #ifndef CHROME_BROWSER_CHROMEOS_TOUCHPAD_H__
Nit: only 1 trailing underscore (see style guide)

http://codereview.chromium.org/193032/diff/3013/3016
File chrome/browser/profile.h (right):

http://codereview.chromium.org/193032/diff/3013/3016#newcode529
Line 529: scoped_ptr<Touchpad> touchpad_;
Nit: Do we need the scoped_ptr?  It doesn't look like this pointer will ever
change.

[sky, 2009-09-14 23:29:51]

http://codereview.chromium.org/193032/diff/3013/3017
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/193032/diff/3013/3017#newcode73
Line 73: // This is just temporary data until we hook this up to real data
nit: periods at end of sentences.

http://codereview.chromium.org/193032/diff/3013/3017#newcode100
Line 100: ReplaceStringPlaceholders(ASCIIToUTF16("$1 ($2, $3)"), subst, NULL));
Shouldn't this format come from the resource files?

http://codereview.chromium.org/193032/diff/3013/3017#newcode241
Line 241: return gfx::Size(font.ave_char_width() * kDefaultWindowWidthChars,
This should actually come from resources too. See CookiesView for an example.

http://codereview.chromium.org/193032/diff/3013/3018
File chrome/browser/chromeos/settings_contents_view.h (right):

http://codereview.chromium.org/193032/diff/3013/3018#newcode13
Line 13: explicit SettingsContentsView(Profile* profile) :
OptionsPageView(profile) {};
nit: no ; here

http://codereview.chromium.org/193032/diff/3013/3018#newcode14
Line 14: virtual ~SettingsContentsView() {};
nit: no ; here either.

http://codereview.chromium.org/193032/diff/3013/3019
File chrome/browser/chromeos/settings_page_view.cc (right):

http://codereview.chromium.org/193032/diff/3013/3019#newcode48
Line 48: layout->AddView(settings_contents_view_);
Use you can FillLayout here. It's much simpler.

http://codereview.chromium.org/193032/diff/3013/3020
File chrome/browser/chromeos/settings_page_view.h (right):

http://codereview.chromium.org/193032/diff/3013/3020#newcode8
Line 8: #include "chrome/browser/chromeos/settings_contents_view.h"
Use a forward declaration rather than including the header.

http://codereview.chromium.org/193032/diff/3013/3021
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/193032/diff/3013/3021#newcode47
Line 47: if (!pref_name || *pref_name == prefs::kTapToClickEnabled) {
nit: no {} for single line ifs.

http://codereview.chromium.org/193032/diff/3013/3021#newcode64
Line 64: char command[80];
You should punt if there is no io thread. Having no io thread is more an error
state than one you really need to deal eloquently with.

http://codereview.chromium.org/193032/diff/3013/3021#newcode70
Line 70: // To disable tap-to-click (i.e. a tap on the touchpad is recognized as
a left
This comment should be in the function.

http://codereview.chromium.org/193032/diff/3013/3021#newcode82
Line 82: // To disable vertical edge scroll, we set VertEdgeScroll to 0.
Vertical edge
this comment should in the function too.

http://codereview.chromium.org/193032/diff/3013/3022
File chrome/browser/chromeos/touchpad.h (right):

http://codereview.chromium.org/193032/diff/3013/3022#newcode23
Line 23: Touchpad(PrefService* prefs);
explicit.

http://codereview.chromium.org/193032/diff/3013/3022#newcode24
Line 24: virtual ~Touchpad() {};
no ;

http://codereview.chromium.org/193032/diff/3013/3022#newcode42
Line 42: BooleanPrefMember vert_edge_scroll_enabled_;
DISALLOW_COPY_AND_ASSIGN.

[Charlie Lee (do not use), 2009-09-15 00:52:13]

Ok, I made the fixes. Please take another look. Thanks.

[tony, 2009-09-15 00:59:24]

Do you need to add the new files to the change set?  I don't see
browser_prefs.cc and I imagine there should be some locale_settings files?

[Charlie Lee (do not use), 2009-09-15 01:16:37]

I ended up calling RegisterUserPreferences from profile.cc as oppose to
browser_prefs. Calling it from browser_prefs won't work, b/c ProfileImpl is
initialized before it and we would be using these preferences before they
are registered.
As for the local_settings change, I ended up just using an existing one for
now. Once the UI is finalized, I will add an entry to local_settings. I
added a comment for the TODO.

- Charlie


On Mon, Sep 14, 2009 at 5:59 PM, <tony@chromium.org> wrote:

> Do you need to add the new files to the change set?  I don't see
> browser_prefs.cc and I imagine there should be some locale_settings files?
>
>
> http://codereview.chromium.org/193032
>

[tony, 2009-09-15 01:26:44]

touchpad.* and the prefs related code LGTM.  I defer to sky for the final LGTM.

http://codereview.chromium.org/193032/diff/9019/9027
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/193032/diff/9019/9027#newcode69
Line 69: argv.push_back("""/usr/bin/synclient""");
Nit: You probably didn't mean to put triple quotes here.

[Charlie Lee (do not use), 2009-09-15 01:37:42]

You are right. Oops, fixed.

- Charlie


On Mon, Sep 14, 2009 at 6:26 PM, <tony@chromium.org> wrote:

> touchpad.* and the prefs related code LGTM.  I defer to sky for the final
> LGTM.
>
>
> http://codereview.chromium.org/193032/diff/9019/9027
> File chrome/browser/chromeos/touchpad.cc (right):
>
> http://codereview.chromium.org/193032/diff/9019/9027#newcode69
> Line 69: argv.push_back("""/usr/bin/synclient""");
> Nit: You probably didn't mean to put triple quotes here.
>
>
> http://codereview.chromium.org/193032
>

[sky, 2009-09-15 16:01:53]

http://codereview.chromium.org/193032/diff/94/9051
File chrome/browser/chromeos/touchpad.cc (right):

http://codereview.chromium.org/193032/diff/94/9051#newcode60
Line 60: void Touchpad::SetSynclientParam(const char* param, const char* value)
{
If you invoke this on the ui thread we take the arguments and pass them to the
file thread. This means the arguments must be valid after the initial call to
the method returns. That's risky. This should take std::strings to avoid that
problem.

http://codereview.chromium.org/193032/diff/94/9052
File chrome/browser/chromeos/touchpad.h (right):

http://codereview.chromium.org/193032/diff/94/9052#newcode10
Line 10: #include "chrome/common/pref_service.h"
Forward declare PrefService so that you don't have to include it here.

http://codereview.chromium.org/193032/diff/94/9052#newcode26
Line 26: static void RegisterUserPrefs(PrefService* prefs);
Description please.

http://codereview.chromium.org/193032/diff/94/9052#newcode28
Line 28: virtual void Init(PrefService* prefs);
Description please.

http://codereview.chromium.org/193032/diff/94/9052#newcode39
Line 39: // This methods makes a system call to synclient to change touchpad
settings
End sentences with a period. Can you be sure and mention this may be invoked on
the file thread.

http://codereview.chromium.org/193032/diff/94/9045
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/193032/diff/94/9045#newcode859
Line 859: Touchpad::RegisterUserPrefs(prefs_.get());
Can you add a comment as to why this needs to be here instead of browser_prefs.

[Charlie Lee (do not use), 2009-09-15 17:21:36]

Fixes made and re-uploaded. Thanks.

[sky, 2009-09-15 17:47:04]

LGTM