Bug fix: Append a language to the list after blocking it for Translate on Chrome OS

chrome/browser/translate/translate_prefs.cc

 // Copyright (c) 2011 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/translate/translate_prefs.h"
 
+#include <set>
+
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/translate/translate_accept_languages.h"
 #include "chrome/browser/translate/translate_manager.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/translate/translate_util.h"
@@ skipping 40 matching lines @@
   std::string main_part = tokens[0];
 
   // Translate doesn't support General Chinese and the sub code is necessary.
   if (main_part == "zh")
     return lang;
 
   TranslateUtil::ToTranslateLanguageSynonym(&main_part);
   return main_part;
 }
 
-}  // namespace
+// Expands language codes to make these more suitable for Accept-Language.
+// Example: ['en-US', 'ja', 'en-CA'] => ['en-US', 'en', 'ja', 'en-CA'].
+// 'en' won't appear twice as this function eliminates duplicates.
+void ExpandLanguageCodes(const std::vector<std::string>& languages,
+                         std::vector<std::string>* expanded_languages) {
+  DCHECK(expanded_languages);

[Takashi Toyoshima, 2013/09/09 12:26:11]

DCHECK(expanded_langauges->empty()) ?

[hajimehoshi, 2013/09/10 03:21:45]

Done.

 
-namespace {
+  // used to eliminate duplicates.
+  std::set<std::string> seen;
 
-void AppendLanguageToAcceptLanguages(PrefService* prefs,
-                                     const std::string& language) {
-  if (!TranslateAcceptLanguages::CanBeAcceptLanguage(language))
-    return;
+  for (std::vector<std::string>::const_iterator it = languages.begin();
+       it != languages.end(); ++it) {
+    const std::string& language = *it;
+    if (seen.find(language) == seen.end()) {
+      expanded_languages->push_back(language);
+      seen.insert(language);
+    }
 
-  std::string accept_language = language;
-  TranslateUtil::ToChromeLanguageSynonym(&accept_language);
-
-  std::string accept_languages_str = prefs->GetString(prefs::kAcceptLanguages);
-  std::vector<std::string> accept_languages;
-  base::SplitString(accept_languages_str, ',', &accept_languages);
-  if (std::find(accept_languages.begin(),
-                accept_languages.end(),
-                accept_language) == accept_languages.end()) {
-    accept_languages.push_back(accept_language);
+    std::vector<std::string> tokens;
+    base::SplitString(language, '-', &tokens);
+    if (tokens.size() == 0)
+      continue;
+    const std::string& main_part = tokens[0];
+    if (seen.find(main_part) == seen.end()) {
+      expanded_languages->push_back(main_part);
+      seen.insert(main_part);
+    }
   }
-  accept_languages_str = JoinString(accept_languages, ',');
-  prefs->SetString(prefs::kAcceptLanguages, accept_languages_str);
 }
 
 }  // namespace
 
 // TranslatePrefs: public: -----------------------------------------------------

[Takashi Toyoshima, 2013/09/09 12:26:11]

BTW, this separator is out of chromium coding style.
Can you remove these separators from this file?
I'm often suggested by other reviewers to remove them, too.

[hajimehoshi, 2013/09/10 03:21:45]

Done.

 
 TranslatePrefs::TranslatePrefs(PrefService* user_prefs)
     : prefs_(user_prefs) {
 }
 
 bool TranslatePrefs::IsBlockedLanguage(
     const std::string& original_language) const {
   return IsValueBlacklisted(kPrefTranslateBlockedLanguages,
                             original_language);
 }
 
 void TranslatePrefs::BlockLanguage(
     const std::string& original_language) {
   BlacklistValue(kPrefTranslateBlockedLanguages, original_language);
-  AppendLanguageToAcceptLanguages(prefs_, original_language);
+
+  // Add the language to the language list at chrome://settings/languages.
+  std::string language = original_language;
+  TranslateUtil::ToChromeLanguageSynonym(&language);
+
+  std::vector<std::string> languages;
+  GetLanguageList(&languages);
+
+  if (std::find(languages.begin(), languages.end(), language) ==
+      languages.end()) {
+    languages.push_back(language);
+    UpdateLanguageList(languages);
+  }
 }
 
 void TranslatePrefs::UnblockLanguage(
     const std::string& original_language) {
   RemoveValueFromBlacklist(kPrefTranslateBlockedLanguages,
                            original_language);
 }
 
 void TranslatePrefs::RemoveLanguageFromLegacyBlacklist(
     const std::string& original_language) {
@@ skipping 112 matching lines @@
   dict->GetInteger(language, &count);
   dict->SetInteger(language, count + 1);
 }
 
 void TranslatePrefs::ResetTranslationAcceptedCount(
     const std::string& language) {
   DictionaryPrefUpdate update(prefs_, kPrefTranslateAcceptedCount);
   update.Get()->SetInteger(language, 0);
 }
 
+void TranslatePrefs::GetLanguageList(std::vector<std::string>* languages) {
+  DCHECK(languages);

[Takashi Toyoshima, 2013/09/09 12:26:11]

DCHECK(languages->empty()) ?

[hajimehoshi, 2013/09/10 03:21:45]

Done.

+
+#if defined(OS_CHROMEOS)
+  const char* key = prefs::kLanguagePreferredLanguages;
+#else
+  const char* key = prefs::kAcceptLanguages;
+#endif
+
+  std::string languages_str = prefs_->GetString(key);
+  base::SplitString(languages_str, ',', languages);
+}
+
+void TranslatePrefs::UpdateLanguageList(
+    const std::vector<std::string>& languages) {
+#if defined(OS_CHROMEOS)
+  std::string languages_str = JoinString(languages, ',');
+  prefs_->SetString(prefs::kLanguagePreferredLanguages, languages_str);
+#endif
+
+  // Save the same language list as accept languages preference as well, but we
+  // need to expand the language list, to make it more acceptable. For instance,
+  // some web sites don't understand 'en-US' but 'en'. See crosbug.com/9884.
+  std::vector<std::string> accept_languages;
+  ExpandLanguageCodes(languages, &accept_languages);

[Takashi Toyoshima, 2013/09/09 12:26:11]

I don't think this works as you expect.

In the case of Chrome OS, it works since it uses kLanguagePreferredLanguages to
get the language list for the preference. If you want to apply the same logic
here, you should update GetLanguageList to return kLanguagePreferredLanguages.
Of course, you need to copy kAcceptLanguages to kLanguagePreferredLanguages if
it doesn't exist.

Otherwise, unexpected languages will appear in the preference. E.g., what happen
if the languages contain "zh-CN", but "zh".

[hajimehoshi, 2013/09/10 03:21:45]

Sorry but I couldn't understand. Please discuss later.

+  std::string accept_languages_str = JoinString(accept_languages, ',');
+  prefs_->SetString(prefs::kAcceptLanguages, accept_languages_str);
+}
+
 // TranslatePrefs: public, static: ---------------------------------------------
 
 // static
 bool TranslatePrefs::CanTranslateLanguage(Profile* profile,
                                           const std::string& language) {
   TranslatePrefs translate_prefs(profile->GetPrefs());
   bool blocked = translate_prefs.IsBlockedLanguage(language);
 
   bool is_accept_language =
       TranslateManager::IsAcceptLanguage(profile, language);
@@ skipping 226 matching lines @@
 
 bool TranslatePrefs::IsListEmpty(const char* pref_id) const {
   const ListValue* blacklist = prefs_->GetList(pref_id);
   return (blacklist == NULL || blacklist->empty());
 }
 
 bool TranslatePrefs::IsDictionaryEmpty(const char* pref_id) const {
   const DictionaryValue* dict = prefs_->GetDictionary(pref_id);
   return (dict == NULL || dict->empty());
 }