Sync starting language and input method preferences

chrome/browser/chromeos/preferences.cc

Index: chrome/browser/chromeos/preferences.cc
diff --git a/chrome/browser/chromeos/preferences.cc b/chrome/browser/chromeos/preferences.cc
index d0e04df2792ce1d2af8709b4bf1f57bad55f0e96..3bbff17074a2d2db822a8d7442454fd3f709c8c8 100644
--- a/chrome/browser/chromeos/preferences.cc
+++ b/chrome/browser/chromeos/preferences.cc
@@ -45,11 +45,49 @@
 
 namespace chromeos {
 
+namespace {
+
 static const char kFallbackInputMethodLocale[] = "en-US";
 
+// Returns a copy of |old_value| with any unique CSVs from |new_value|
+// appended, a logical union that retains order. Quadratic runtime; use only
+// for small lists.

[michaelpg, 2014/06/04 01:12:02]

Why not use ListValues instead of StringValues for these preferences?

* The existing prefs already use strings
* This function will only be called at most once per device
* Given the nature of the preferences, time complexity is not 
  a concern
* There is no set-union for vectors which retains the pre-existing,
  non-sorted order (AFAIK) so this work is necessary anyway

+std::string MergeCSVs(const std::string& old_value,
+                      const std::string& new_value) {
+  std::string merged;
+  std::vector<std::string> old_tokens;
+  std::vector<std::string> new_tokens;
+
+  base::SplitString(old_value, ',', &old_tokens);
+  base::SplitString(new_value, ',', &new_tokens);
+
+  for (size_t i = 0; i < new_tokens.size(); i++) {
+    // Skip token if it's already in |old_tokens|.
+    if (std::find(old_tokens.begin(), old_tokens.end(), new_tokens[i]) ==
+        old_tokens.end())
+      old_tokens.push_back(new_tokens[i]);
+  }
+  return JoinString(old_tokens, ',');
+}
+
+// Merges one CSV pref into another using MergeCSVs.
+void MergeStringPrefs(StringPrefMember& to_pref,
+                      StringPrefMember& from_pref,
+                      const std::string& pref_name) {
+  std::string to_value = to_pref.GetValue();
+  std::string from_value = from_pref.GetValue();
+
+  std::string merged_value = MergeCSVs(to_value, from_value);
+  if (merged_value != to_value)
+    to_pref.SetValue(merged_value);
+}
+
+}  // anonymous namespace
+
 Preferences::Preferences()
     : prefs_(NULL),
       input_method_manager_(input_method::InputMethodManager::Get()),
+      should_merge_locale_prefs_(false),
       user_(NULL),
       user_is_primary_(false) {
   // Do not observe shell, if there is no shell instance; e.g., in some unit
@@ -61,6 +99,7 @@ Preferences::Preferences()
 Preferences::Preferences(input_method::InputMethodManager* input_method_manager)
     : prefs_(NULL),
       input_method_manager_(input_method_manager),
+      should_merge_locale_prefs_(false),
       user_(NULL),
       user_is_primary_(false) {
   // Do not observe shell, if there is no shell instance; e.g., in some unit
@@ -213,20 +252,29 @@ void Preferences::RegisterProfilePrefs(
       prefs::kLanguagePreviousInputMethod,
       "",
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  // We don't sync the list of input methods and preferred languages since a
-  // user might use two or more devices with different hardware keyboards.
-  // crosbug.com/15181
   registry->RegisterStringPref(
       prefs::kLanguagePreferredLanguages,
+      "",
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterStringPref(
+      prefs::kLanguagePreferredLanguagesLocal,

[Alexander Alekseev, 2014/06/04 14:20:57]

I think it is dangerous to change meaning of existing preference.
Would you mind adding prefs::kLanguagePreferredLanguagesSyncable ?

Even if you found and renamed all existing usage of kLanguagePreferredLanguages,
there might be pending CLs using kLanguagePreferredLanguages in current way.

       kFallbackInputMethodLocale,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterStringPref(
       prefs::kLanguagePreloadEngines,
+      "",
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterStringPref(
+      prefs::kLanguagePreloadEnginesLocal,

[Alexander Alekseev, 2014/06/04 14:20:57]

The same for engines.

       hardware_keyboard_id,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterStringPref(
       prefs::kLanguageEnabledExtensionImes,
       "",
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterStringPref(
+      prefs::kLanguageEnabledExtensionImesLocal,
+      "",

[Alexander Alekseev, 2014/06/04 14:20:57]

The same for EnabledExtensionsImes

       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 
   registry->RegisterIntegerPref(
@@ -329,9 +377,17 @@ void Preferences::InitUserPrefs(PrefServiceSyncable* prefs) {
                                    prefs, callback);
   touch_hud_projection_enabled_.Init(prefs::kTouchHudProjectionEnabled,
                                      prefs, callback);
+  preferred_languages_.Init(prefs::kLanguagePreferredLanguages,
+                            prefs, callback);
+  preferred_languages_local_.Init(prefs::kLanguagePreferredLanguagesLocal,
+                                  prefs, callback);
   preload_engines_.Init(prefs::kLanguagePreloadEngines, prefs, callback);
+  preload_engines_local_.Init(prefs::kLanguagePreloadEnginesLocal,
+                              prefs, callback);
   enabled_extension_imes_.Init(prefs::kLanguageEnabledExtensionImes,
                                prefs, callback);
+  enabled_extension_imes_local_.Init(prefs::kLanguageEnabledExtensionImesLocal,
+                                     prefs, callback);
   current_input_method_.Init(prefs::kLanguageCurrentInputMethod,
                              prefs, callback);
   previous_input_method_.Init(prefs::kLanguagePreviousInputMethod,
@@ -388,6 +444,27 @@ void Preferences::ApplyPreferences(ApplyReason reason,
       UserManager::Get()->GetOwnerEmail() == user_->email();
   const bool user_is_active = user_->is_active();
 
+  if (reason == REASON_INITIALIZATION) {
+    // Check that the global language/input preferences have been set.
+    if (prefs_->FindPreference(prefs::kLanguagePreferredLanguages)->
+        HasUserSetting()) {
+      // If no local value has been set, this is a pre-existing profile.
+      if (!prefs_->FindPreference(prefs::kLanguagePreferredLanguagesLocal)->
+          HasUserSetting()) {
+        // Migrate to using local values by copying the global values.
+        preferred_languages_local_.SetValue(preferred_languages_.GetValue());
+        preload_engines_local_.SetValue(preload_engines_.GetValue());

[Alexander Alekseev, 2014/06/04 14:23:56]

The same as my comment below: the list of supported languages and extensions
might be different across devices.

+        enabled_extension_imes_local_.SetValue(
+            enabled_extension_imes_.GetValue());

[Alexander Alekseev, 2014/06/04 14:20:57]

This is dangerous, as you do not check that remote values are supported locally.
Remote values can be either from older version of chrome, or from newer version.
Also you cannot guarantee that list of supported extensions is the same on all
platforms.

+      } else {
+        should_merge_locale_prefs_ = false;
+      }
+    } else {
+      // The global preference hasn't been set, so we need to sync and merge it.
+      should_merge_locale_prefs_ = true;
+    }
+  }
+
   system::TouchpadSettings touchpad_settings;
   system::MouseSettings mouse_settings;
 
@@ -511,7 +588,9 @@ void Preferences::ApplyPreferences(ApplyReason reason,
       pref_name == prefs::kTouchHudProjectionEnabled) {
     if (user_is_active) {
       const bool enabled = touch_hud_projection_enabled_.GetValue();
-      ash::Shell::GetInstance()->SetTouchHudProjectionEnabled(enabled);
+      // There may not be a shell, e.g., in some unit tests.
+      if (ash::Shell::HasInstance())
+        ash::Shell::GetInstance()->SetTouchHudProjectionEnabled(enabled);
     }
   }
 
@@ -531,25 +610,28 @@ void Preferences::ApplyPreferences(ApplyReason reason,
       UpdateAutoRepeatRate();
   }
 
+  if (reason == REASON_PREF_CHANGED &&
+      pref_name == prefs::kLanguagePreferredLanguagesLocal)
+    SetSyncedLocalePrefs();
+
   if (reason != REASON_PREF_CHANGED && user_is_active) {
     SetInputMethodList();
-  } else if (pref_name == prefs::kLanguagePreloadEngines && user_is_active) {
-    SetLanguageConfigStringListAsCSV(language_prefs::kGeneralSectionName,
-                                     language_prefs::kPreloadEnginesConfigName,
-                                     preload_engines_.GetValue());
+  } else if (pref_name == prefs::kLanguagePreloadEnginesLocal) {
+    if (user_is_active) {
+      SetLanguageConfigStringListAsCSV(
+          language_prefs::kGeneralSectionName,
+          language_prefs::kPreloadEnginesConfigName,
+          preload_engines_local_.GetValue());
+    }
+    SetSyncedLocalePrefs();
   }
 
   if (reason != REASON_PREF_CHANGED ||
-      pref_name == prefs::kLanguageEnabledExtensionImes) {
-    if (user_is_active) {
-      std::string value(enabled_extension_imes_.GetValue());
-
-      std::vector<std::string> split_values;
-      if (!value.empty())
-        base::SplitString(value, ',', &split_values);
-
-      input_method_manager_->SetEnabledExtensionImes(&split_values);
-    }
+      pref_name == prefs::kLanguageEnabledExtensionImesLocal) {
+    if (user_is_active)
+      SetEnabledExtensionImes();
+    if (reason == REASON_PREF_CHANGED)
+      SetSyncedLocalePrefs();
   }
 
   if (user_is_active) {
@@ -562,6 +644,25 @@ void Preferences::ApplyPreferences(ApplyReason reason,
 void Preferences::OnIsSyncingChanged() {
   DVLOG(1) << "OnIsSyncingChanged";
   ForceNaturalScrollDefault();
+
+  if (should_merge_locale_prefs_ && prefs_->IsSyncing()) {
+    should_merge_locale_prefs_ = false;
+    MergeStringPrefs(preferred_languages_local_,
+                     preferred_languages_,
+                     prefs::kLanguagePreferredLanguagesLocal);
+    MergeStringPrefs(preload_engines_local_,
+                     preload_engines_,
+                     prefs::kLanguagePreloadEnginesLocal);
+    MergeStringPrefs(enabled_extension_imes_local_,
+                     enabled_extension_imes_,
+                     prefs::kLanguageEnabledExtensionImesLocal);
+
+    SetLanguageConfigStringListAsCSV(language_prefs::kGeneralSectionName,
+                                     language_prefs::kPreloadEnginesConfigName,
+                                     preload_engines_local_.GetValue());
+    SetEnabledExtensionImes();
+    SetSyncedLocalePrefs();
+  }
 }
 
 void Preferences::ForceNaturalScrollDefault() {
@@ -587,7 +688,7 @@ void Preferences::SetLanguageConfigStringListAsCSV(const char* section,
 
   // Transfers the xkb id to extension-xkb id.
   if (input_method_manager_->MigrateInputMethods(&split_values))
-    preload_engines_.SetValue(JoinString(split_values, ','));
+    preload_engines_local_.SetValue(JoinString(split_values, ','));
 
   if (section == std::string(language_prefs::kGeneralSectionName) &&
       name == std::string(language_prefs::kPreloadEnginesConfigName)) {
@@ -597,30 +698,49 @@ void Preferences::SetLanguageConfigStringListAsCSV(const char* section,
 }
 
 void Preferences::SetInputMethodList() {
-  // When |preload_engines_| are set, InputMethodManager::ChangeInputMethod()
-  // might be called to change the current input method to the first one in the
-  // |preload_engines_| list. This also updates previous/current input method
-  // prefs. That's why GetValue() calls are placed before the
-  // SetLanguageConfigStringListAsCSV() call below.
+  // When |preload_engines_local_| are set,
+  // InputMethodManager::ChangeInputMethod() might be called to change the
+  // current input method to the first one in the |preload_engines_local_| list.
+  // This also updates previous/current input method prefs. That's why
+  // GetValue() calls are placed before the SetLanguageConfigStringListAsCSV()
+  // call below.
   const std::string previous_input_method_id =
       previous_input_method_.GetValue();
   const std::string current_input_method_id = current_input_method_.GetValue();
   SetLanguageConfigStringListAsCSV(language_prefs::kGeneralSectionName,
                                    language_prefs::kPreloadEnginesConfigName,
-                                   preload_engines_.GetValue());
-
-  // ChangeInputMethod() has to be called AFTER the value of |preload_engines_|
-  // is sent to the InputMethodManager. Otherwise, the ChangeInputMethod request
-  // might be ignored as an invalid input method ID. The ChangeInputMethod()
-  // calls are also necessary to restore the previous/current input method prefs
-  // which could have been modified by the SetLanguageConfigStringListAsCSV call
-  // above to the original state.
+                                   preload_engines_local_.GetValue());
+
+  // ChangeInputMethod() has to be called AFTER the value of
+  // |preload_engines_local_| is sent to the InputMethodManager. Otherwise, the
+  // ChangeInputMethod request might be ignored as an invalid input method ID.
+  // The ChangeInputMethod() calls are also necessary to restore the
+  // previous/current input method prefs which could have been modified by the
+  // SetLanguageConfigStringListAsCSV call above to the original state.
   if (!previous_input_method_id.empty())
     input_method_manager_->ChangeInputMethod(previous_input_method_id);
   if (!current_input_method_id.empty())
     input_method_manager_->ChangeInputMethod(current_input_method_id);
 }
 
+void Preferences::SetEnabledExtensionImes() {
+  std::string value(enabled_extension_imes_local_.GetValue());
+  std::vector<std::string> split_values;
+  if (!value.empty())
+    base::SplitString(value, ',', &split_values);
+  input_method_manager_->SetEnabledExtensionImes(&split_values);
+}
+
+void Preferences::SetSyncedLocalePrefs() {
+  // Set the language and input prefs at the same time. Otherwise, we may,
+  // e.g., use a stale languages setting but push a new preload engines setting.
+  if (!should_merge_locale_prefs_) {
+    preferred_languages_.SetValue(preferred_languages_local_.GetValue());
+    preload_engines_.SetValue(preload_engines_local_.GetValue());
+    enabled_extension_imes_.SetValue(enabled_extension_imes_local_.GetValue());
+  }
+}
+
 void Preferences::UpdateAutoRepeatRate() {
   input_method::AutoRepeatRate rate;
   rate.initial_delay_in_ms = xkb_auto_repeat_delay_pref_.GetValue();