Do not use local override for language settings: always sync.

Do not use local override for language settings: always store it into synchronized preference.

BUG=chromium-os:11147
TEST=Manual

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

[Denis Lagno, 2011-01-25 14:16:15]

please take a look

[Nikita (slow), 2011-01-25 14:35:31]

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:69:
tab_contents_->profile()->ChangeApplicationLocale(from_locale_);
In this case should we clear LocaleBackup setting?
Why it's needed?

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:113: if
(prefs->GetString(prefs::kApplicationLocaleAccepted) == to_locale)
Could you describe case which we're optimizing here?

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:489: // "Keep local" means that changes should
not be propagated to other devices.
nit: update comment

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1375:
GetPrefs()->SetString(prefs::kApplicationLocaleBackup, locale);
Why kApplicationLocaleBackup / Accepted prefs state is different after local
change if compared with local change guard operations on these prefs.

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:24: // For OS_CHROMEOS we maintain kApplicationLocale
property in both local state
nit: update comment

[Nikita (slow), 2011-01-25 14:38:28]

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:29: extern const char kApplicationLocaleBackup[];
Is it possible to create a table somewhere that will look like
                AppLocale Backup Accepted
Initial login      A        -       - 
Sync               B        A       -
Accept             B        A       A (?)
Revert             A        ?       ?
Local change       C        ?       ?

[Denis Lagno, 2011-01-25 15:46:43]

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:69:
tab_contents_->profile()->ChangeApplicationLocale(from_locale_);
On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> In this case should we clear LocaleBackup setting?
> Why it's needed?

ChangeApplicationLocale will set both kApplicationLocaleBackup and
kApplicationLocale to current locale.

value of kApplicationLocaleBackup steps into significance only in 2 cases:

(1) Backup is non-empty and kApplicationLocale is empty (during initial
bootstrap of kApplicationLocale on first logins).

(2) both Backup and kApplicationLocale are non-empty and differ (in that case
notification may be shown).

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:113: if
(prefs->GetString(prefs::kApplicationLocaleAccepted) == to_locale)
On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> Could you describe case which we're optimizing here?

it is not optimization, it is double check.
This code is here to make it clear (and ensure) that we do not show notification
if notification (with the same language) was already accepted.

It is double check because login_utils.cc contains code:

  std::string pref_locale = prefs->GetString(prefs::kApplicationLocale);
  if (!pref_locale.empty()) {
    if (prefs->GetString(prefs::kApplicationLocaleAccepted) == pref_locale) {
      // If locale is accepted then we do not want to show LocaleChange
      // notification.  This notification is triggered by different values of
      // kApplicationLocaleBackup and kApplicationLocale preferences,
      // so make them identical.
      prefs->SetString(prefs::kApplicationLocaleBackup, pref_locale);


so, if current language was accepted then Backup == kApplicationLocale, so
previous check will return.


(optimization is the first check:
  if (cur_profile_id == profile_id_)
    return;  // We have already checked this profile, exiting fast.

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:489: // "Keep local" means that changes should
not be propagated to other devices.
On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> nit: update comment

Done.

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1375:
GetPrefs()->SetString(prefs::kApplicationLocaleBackup, locale);
On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> Why kApplicationLocaleBackup / Accepted prefs state is different after local
> change if compared with local change guard operations on these prefs.

Backup must not be cleared.  Backup should be kept as a reference.  In case
kApplicationLocale would change due to sync from other device then this Backup
value will allow us to show notification.
Added comment on that.

OTOH kApplicationLocaleAccepted can be cleared to make it clear that we allow
notifications again; or it can be just left equal to old value, I don't think it
matters.

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:24: // For OS_CHROMEOS we maintain kApplicationLocale
property in both local state
On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> nit: update comment

but..but..but cannot see what changed in this respect?

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:29: extern const char kApplicationLocaleBackup[];
On 2011/01/25 14:38:28, Nikita Kostylev wrote:
> Is it possible to create a table somewhere that will look like
>                 AppLocale Backup Accepted
> Initial login      A        -       - 
> Sync               B        A       -
> Accept             B        A       A (?)
> Revert             A        ?       ?
> Local change       C        ?       ?

Made an attempt.

[Nikita (slow), 2011-01-25 16:04:44]

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6248017/diff/5001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:24: // For OS_CHROMEOS we maintain kApplicationLocale
property in both local state
On 2011/01/25 15:46:43, Denis Lagno wrote:
> On 2011/01/25 14:35:31, Nikita Kostylev wrote:
> > nit: update comment
> 
> but..but..but cannot see what changed in this respect?

Right, it didn't mention LocaleOverride.
I've thought about having a Backup/Accepted usage described somewhere.

http://codereview.chromium.org/6248017/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6248017/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:488: // Changes application locale.
Please add cases when it's called. I was confused that method is called on every
locale change.

http://codereview.chromium.org/6248017/diff/19001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6248017/diff/19001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:72: // Locale changed on login screen      A       
C       -
As we discussed locale change in settings behaves differently i.e. always clears
"accepted" pref.

Is it possible to eliminate "accepted" preference since we always show that
notification when sync language is different?

[Nikita (slow), 2011-01-25 16:32:04]

LGTM

As discussed, Accepted preference is needed to prevent multiple notifications on
each login in case there're multiple users working on the same notebook with
different locales set.

[Denis Lagno, 2011-01-26 10:31:40]

http://codereview.chromium.org/6248017/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6248017/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:488: // Changes application locale.
On 2011/01/25 16:04:44, Nikita Kostylev wrote:
> Please add cases when it's called. I was confused that method is called on
every
> locale change.

reasonable concern.
I've made this method to be called on every language change for a profile.