Fix race condition in MD Settings Languages page causing inconsistent settings

chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/language_settings_private/language_settings_private_api.h"
 
 #include <algorithm>
 #include <map>
 #include <memory>
 #include <string>
@@ skipping 35 matching lines @@
 
 namespace extensions {
 
 namespace language_settings_private = api::language_settings_private;
 
 #if defined(OS_CHROMEOS)
 using chromeos::input_method::InputMethodDescriptor;
 using chromeos::input_method::InputMethodDescriptors;
 using chromeos::input_method::InputMethodManager;
 using chromeos::input_method::InputMethodUtil;
+#endif
 
 namespace {
 
+static const char kErrorLanguageAlreadyEnabled[] = "Language already enabled";
+static const char kErrorLanguageNotEnabled[] = "Language not enabled";
+
+#if defined(OS_CHROMEOS)
 // Returns the set of IDs of all enabled IMEs.
 std::unordered_set<std::string> GetEnabledIMEs(
     scoped_refptr<InputMethodManager::State> ime_state) {
   const std::vector<std::string>& ime_ids(ime_state->GetActiveInputMethodIds());
   return std::unordered_set<std::string>(ime_ids.begin(), ime_ids.end());
 }
 
 // Returns the set of IDs of enabled IMEs for the given pref.
 std::unordered_set<std::string> GetIMEsFromPref(PrefService* prefs,
                                                 const char* pref_name) {
@@ skipping 76 matching lines @@
         // Remove the added descriptor from the candidate list.
         it = descriptors.erase(it);
       } else {
         it++;
       }
     }
   }
 
   return extension_ime_list;
 }
+#endif
 
 }  // anonymous namespace
-#endif
 
 LanguageSettingsPrivateGetLanguageListFunction::
     LanguageSettingsPrivateGetLanguageListFunction() {
 }
 
 LanguageSettingsPrivateGetLanguageListFunction::
     ~LanguageSettingsPrivateGetLanguageListFunction() {
 }
 
 ExtensionFunction::ResponseAction
@@ skipping 70 matching lines @@
     if (spellcheck_language_set.count(pair.first) > 0)
       language.supports_spellcheck.reset(new bool(true));
     if (translate_language_set.count(pair.first) > 0)
       language.supports_translate.reset(new bool(true));
 
     language_list->Append(language.ToValue());
   }
   return RespondNow(OneArgument(std::move(language_list)));
 }
 
-LanguageSettingsPrivateSetLanguageListFunction::
-    LanguageSettingsPrivateSetLanguageListFunction()
-    : chrome_details_(this) {
-}
+LanguageSettingsPrivateEnableLanguageFunction::
+    LanguageSettingsPrivateEnableLanguageFunction()
+    : chrome_details_(this) {}
 
-LanguageSettingsPrivateSetLanguageListFunction::
-    ~LanguageSettingsPrivateSetLanguageListFunction() {
-}
+LanguageSettingsPrivateEnableLanguageFunction::
+    ~LanguageSettingsPrivateEnableLanguageFunction() {}
 
 ExtensionFunction::ResponseAction
-LanguageSettingsPrivateSetLanguageListFunction::Run() {
-  std::unique_ptr<language_settings_private::SetLanguageList::Params>
+LanguageSettingsPrivateEnableLanguageFunction::Run() {
+  std::unique_ptr<language_settings_private::EnableLanguage::Params>
       parameters =
-          language_settings_private::SetLanguageList::Params::Create(*args_);
+          language_settings_private::EnableLanguage::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(parameters.get());
 
   std::unique_ptr<translate::TranslatePrefs> translate_prefs =
       ChromeTranslateClient::CreateTranslatePrefs(
           chrome_details_.GetProfile()->GetPrefs());
-  translate_prefs->UpdateLanguageList(parameters->language_codes);
+
+  std::vector<std::string> languages;
+  translate_prefs->GetLanguageList(&languages);
+
+  if (std::find(languages.begin(), languages.end(),
+                parameters->language_code) != languages.end()) {
+    return RespondNow(Error(kErrorLanguageAlreadyEnabled));

[Devlin, 2016/08/22 18:38:19]

drive-by: To me, this seems a little weird.  I would just silently let it
return.  But, you'll know best.

[stevenjb, 2016/08/22 18:41:30]

I requested this. The UI shouldn't generally do this and if it is, this could
help debug any problems.

Alternatively I guess we could return a boolean result instead, would that be
preferable?

[Devlin, 2016/08/22 19:01:00]

Given this is a private API, I don't care that much. :)  Whatever y'all want. 
It just struck me as a little unusual.

+  }
+
+  languages.push_back(parameters->language_code);

[Devlin, 2016/08/22 18:38:19]

drive-by: do we need to check that the language exists?

+  translate_prefs->UpdateLanguageList(languages);
 
   return RespondNow(NoArguments());
 }
+
+LanguageSettingsPrivateDisableLanguageFunction::
+    LanguageSettingsPrivateDisableLanguageFunction()
+    : chrome_details_(this) {}
+
+LanguageSettingsPrivateDisableLanguageFunction::
+    ~LanguageSettingsPrivateDisableLanguageFunction() {}
+
+ExtensionFunction::ResponseAction
+LanguageSettingsPrivateDisableLanguageFunction::Run() {
+  std::unique_ptr<language_settings_private::DisableLanguage::Params>
+      parameters =
+          language_settings_private::DisableLanguage::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters.get());
+
+  std::unique_ptr<translate::TranslatePrefs> translate_prefs =
+      ChromeTranslateClient::CreateTranslatePrefs(
+          chrome_details_.GetProfile()->GetPrefs());
+
+  std::vector<std::string> languages;
+  translate_prefs->GetLanguageList(&languages);
+
+  auto it =
+      std::find(languages.begin(), languages.end(), parameters->language_code);
+  if (it == languages.end())
+    return RespondNow(Error(kErrorLanguageNotEnabled));
+
+  languages.erase(it);
+  translate_prefs->UpdateLanguageList(languages);
+
+  return RespondNow(NoArguments());
+}
 
 LanguageSettingsPrivateGetSpellcheckDictionaryStatusesFunction::
     LanguageSettingsPrivateGetSpellcheckDictionaryStatusesFunction() {
 }
 
 LanguageSettingsPrivateGetSpellcheckDictionaryStatusesFunction::
     ~LanguageSettingsPrivateGetSpellcheckDictionaryStatusesFunction() {
 }
 
 ExtensionFunction::ResponseAction
@@ skipping 284 matching lines @@
                               input_method_list.end(), input_method_id);
   if (pos != input_method_list.end()) {
     input_method_list.erase(pos);
     prefs->SetString(pref_name, base::JoinString(input_method_list, ","));
   }
 #endif
   return RespondNow(NoArguments());
 }
 
 }  // namespace extensions