Chrome OS: make language per user.

Chrome OS: make language per user.

BUG=http://crosbug.com/3873
TEST=Manual

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

[Denis Lagno, 2010-10-06 17:46:48]

please take a look

[Nikita (slow), 2010-10-07 12:30:11]

As discussed,

- kLoginLocale is not needed since login locale always matches app_local (locale
of last logged in user).

- user locale should be part of user preferences so that once user saves his
locale, it will be synced and user gets this locale on whatever new device login
occurs.
However tricky part is that user prefs are synced after short period of time and
need to make sure that app locale is used from there.

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

http://codereview.chromium.org/3579011/diff/12001/13001#newcode553
chrome/browser/browser_main.cc:553:
local_state->RegisterDictionaryPref(prefs::kPerUserLocale);
These settings registration should be placed in some class RegisterPrefs static
method and called 

in chrome/browser/prefs/browser_prefs.h
RegisterLocalState

http://codereview.chromium.org/3579011/diff/12001/13002
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3579011/diff/12001/13002#newcode303
chrome/browser/chromeos/dom_ui/language_options_handler.cc:303:
per_user_locale->SetWithoutPathExpansion(username,
Value::CreateStringValue(language_code));
split into 2 lines.

http://codereview.chromium.org/3579011/diff/12001/13009
File chrome/browser/sync/glue/synchronized_preferences.h (right):

http://codereview.chromium.org/3579011/diff/12001/13009#newcode75
chrome/browser/sync/glue/synchronized_preferences.h:75:
prefs::kApplicationLocale,
I'm not sure why kApplicationLocale is marked as synchronizable here because
it's a global one.

http://codereview.chromium.org/3579011/diff/12001/13009#newcode77
chrome/browser/sync/glue/synchronized_preferences.h:77: prefs::kPerUserLocale,
User Locale should synced only.

[Denis Lagno, 2010-11-11 13:29:58]

reworked/simplified.

please wield your LGTM seals.

[tfarina, 2010-11-11 16:54:20]

http://codereview.chromium.org/3579011/diff/24001/chrome/browser/chromeos/dom...
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3579011/diff/24001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/language_options_handler.cc:296: for (size_t i =
0; i < sizeof(prefs) / sizeof(prefs[0]); ++i) {
use arraysize for this from base/basictypes.h, please.

[Denis Lagno, 2010-11-11 17:07:12]

http://codereview.chromium.org/3579011/diff/24001/chrome/browser/chromeos/dom...
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3579011/diff/24001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/language_options_handler.cc:296: for (size_t i =
0; i < sizeof(prefs) / sizeof(prefs[0]); ++i) {
On 2010/11/11 16:54:25, tfarina wrote:
> use arraysize for this from base/basictypes.h, please.

Done.

[glotov, 2010-11-11 18:16:22]

LGTM

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/dom...
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/language_options_handler.cc:293: PrefService*
prefs[] = {
Please comment why there is 2 PrefServices.

[Denis Lagno, 2010-11-11 19:48:03]

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/language_switch_menu.cc (right):

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/language_switch_menu.cc:85:
DCHECK(g_browser_process);
move this DCHECK before anything else

[Denis Lagno, 2010-11-12 11:51:34]

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/dom...
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/language_options_handler.cc:293: PrefService*
prefs[] = {
On 2010/11/11 18:16:23, glotov wrote:
> Please comment why there is 2 PrefServices.

Done.

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/language_switch_menu.cc (right):

http://codereview.chromium.org/3579011/diff/29001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/language_switch_menu.cc:85:
DCHECK(g_browser_process);
On 2010/11/11 19:48:03, Denis Lagno wrote:
> move this DCHECK before anything else

Done.

[Nikita (slow), 2010-11-13 10:36:59]

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:214: std::string pref_locale =
profile->GetPrefs()->GetString(
Please extract into a separate method.
CompleteLogin now is 140 lines long.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:217: if (first_login) {
How do you intend to bootstrap this option to the existing users?
They would always have an empty preference?

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:218:
profile->GetPrefs()->SetString(
As I understand we're always forcing language selected at Sign In pod to a user
profile.

What happens if user has this option set in his sync profile?
TODO/comment here.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
You're not introducing a new preference but use existing one, though introducing
confusion. Probably it's OK, but should be clearly stated where
kApplicationLocale is defined.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
Not marking this new profile preference as synchronized?

[Denis Lagno, 2010-11-13 12:48:22]

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:214: std::string pref_locale =
profile->GetPrefs()->GetString(
On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> Please extract into a separate method.
> CompleteLogin now is 140 lines long.

Done.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:217: if (first_login) {
On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> How do you intend to bootstrap this option to the existing users?
> They would always have an empty preference?

yes, I think check for first login does not resolve anything here.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:218:
profile->GetPrefs()->SetString(
On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> As I understand we're always forcing language selected at Sign In pod to a
user
> profile.
> 
> What happens if user has this option set in his sync profile?
> TODO/comment here.

Done.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> Not marking this new profile preference as synchronized?

kApplicationLocale is already present in
chrome/browser/sync/glue/synchronized_preferences.h

Apparently, initial intention was to have it as a synced user's preference but
later it was moved to Local State.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> You're not introducing a new preference but use existing one, though
introducing
> confusion. Probably it's OK, but should be clearly stated where
> kApplicationLocale is defined.

Done.

[Nikita (slow), 2010-11-15 07:54:41]

LGTM with few comments

Don't merge it to 552 branch, let's test how it works on TOT.

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/3579011/diff/39001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:217: if (first_login) {
On 2010/11/13 12:48:23, Denis Lagno wrote:
> On 2010/11/13 10:36:59, Nikita Kostylev wrote:
> > How do you intend to bootstrap this option to the existing users?
> > They would always have an empty preference?
> 
> yes, I think check for first login does not resolve anything here.

Agree, there's one more thing remaining - conflicting with sync preference but
let's iterate in the future CLs.

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:271: }
That would be always the case because profile is not synced at this point yet.

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/profile.cc
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
Please put it in the CHROME_OS ifdef with the TODO because we haven't
tested/used this preference on the other platforms yet.

[Denis Lagno, 2010-11-15 11:50:05]

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:271: // but still not synced yet.
On 2010/11/15 07:54:41, Nikita Kostylev wrote:
> That would be always the case because profile is not synced at this point yet.

Done.

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/profile.cc
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/3579011/diff/45001/chrome/browser/profile.cc#n...
chrome/browser/profile.cc:99:
prefs->RegisterStringPref(prefs::kApplicationLocale, "");
On 2010/11/15 07:54:41, Nikita Kostylev wrote:
> Please put it in the CHROME_OS ifdef with the TODO because we haven't
> tested/used this preference on the other platforms yet.

Done.