Move management of accept languages to language_prefs.

With this cl adding and removing languages to kAcceptLanguages will be handled by language_prefs.
The idea behind this refactoring work is to separate concepts of accept languages (saved in Chrome format, consumed by translate and non-translate clients) and blocked/fluent languages that are stored in translate format and are consumed by translate clients.
Next step is to move isFluent from language_prefs to translate_prefs.

Bug: 949251
Change-Id: I6971887bec72856c35ca61010c4ff6976f622164
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2565069Reviewed-by: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trevor  Perrier <perrier@chromium.org>
Commit-Queue: Basia Zimirska <basiaz@google.com>
Cr-Commit-Position: refs/heads/master@{#834962}

chrome/browser/translate/chrome_translate_client.cc

@@ -157,14 +157,8 @@ ChromeTranslateClient::per_frame_translate_driver() {
 // static
 std::unique_ptr<translate::TranslatePrefs>
 ChromeTranslateClient::CreateTranslatePrefs(PrefService* prefs) {
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  const char* preferred_languages_prefs = language::prefs::kPreferredLanguages;
-#else
-  const char* preferred_languages_prefs = NULL;
-#endif
   std::unique_ptr<translate::TranslatePrefs> translate_prefs(
-      new translate::TranslatePrefs(prefs, language::prefs::kAcceptLanguages,
-                                    preferred_languages_prefs));
+      new translate::TranslatePrefs(prefs));
 
   // We need to obtain the country here, since it comes from VariationsService.
   // components/ does not have access to that.

components/language/core/browser/BUILD.gn

@@ -26,6 +26,7 @@ static_library("browser") {
 
   deps = [
     "//base",
+    "//build:chromeos_buildflags",
     "//components/keyed_service/core",
     "//components/language/core/common",
     "//components/pref_registry",
@@ -46,6 +47,7 @@ source_set("unit_tests") {
   ]
   deps = [
     ":browser",
+    ":test_support",
     "//base",
     "//components/pref_registry:pref_registry",
     "//components/prefs",
@@ -57,3 +59,19 @@ source_set("unit_tests") {
     "//testing/gtest",
   ]
 }
+
+source_set("test_support") {
+  testonly = true
+  sources = [
+    "language_prefs_test_util.cc",
+    "language_prefs_test_util.h",
+  ]
+  deps = [
+    ":browser",
+    "//base",
+    "//build:chromeos_buildflags",
+    "//components/prefs",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+}

components/language/core/browser/language_prefs.cc

@@ -14,6 +14,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
+#include "build/chromeos_buildflags.h"
 #include "components/language/core/browser/pref_names.h"
 #include "components/language/core/common/language_util.h"
 #include "components/language/core/common/locale_util.h"
@@ -86,6 +87,30 @@ void LanguagePrefs::ResetEmptyFluentLanguagesToDefault() {
     ResetFluentLanguagesToDefaults();
 }
 
+void LanguagePrefs::GetAcceptLanguagesList(
+    std::vector<std::string>* languages) const {
+  DCHECK(languages);
+  DCHECK(languages->empty());
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  const std::string& key = language::prefs::kPreferredLanguages;
+#else
+  const std::string& key = language::prefs::kAcceptLanguages;
+#endif
+
+  *languages = base::SplitString(prefs_->GetString(key), ",",
+                                 base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+}
+
+void LanguagePrefs::SetAcceptLanguagesList(
+    const std::vector<std::string>& languages) {
+  std::string languages_str = base::JoinString(languages, ",");
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  prefs_->SetString(language::prefs::kPreferredLanguages, languages_str);
+#endif
+
+  prefs_->SetString(language::prefs::kAcceptLanguages, languages_str);
+}
+
 // static
 base::Value LanguagePrefs::GetDefaultFluentLanguages() {
   typename base::Value::ListStorage languages;

components/language/core/browser/language_prefs.h

@@ -44,6 +44,12 @@ class LanguagePrefs {
   static base::Value GetDefaultFluentLanguages();
   // If the list of fluent languages is empty, reset it to defaults.
   void ResetEmptyFluentLanguagesToDefault();
+  // Gets the language list of the language settings. Language settings list
+  // have the Chrome internal format.
+  void GetAcceptLanguagesList(std::vector<std::string>* languages) const;
+  // Updates the language list of the language settings. Languages are expected
+  // to be in the Chrome internal format.
+  void SetAcceptLanguagesList(const std::vector<std::string>& languages);
 
  private:
   base::Value* GetFluentLanguages();

components/language/core/browser/language_prefs_test_util.cc

+// Copyright 2020 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 "components/language/core/browser/language_prefs_test_util.h"
+
+#include "base/strings/string_util.h"
+#include "build/chromeos_buildflags.h"
+#include "components/language/core/browser/pref_names.h"
+#include "components/prefs/pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace language {
+namespace test {
+
+AcceptLanguagesTester::AcceptLanguagesTester(PrefService* user_prefs)
+    : prefs_(user_prefs) {}
+
+void AcceptLanguagesTester::ExpectLanguagePrefs(
+    const std::string& expected_prefs,
+    const std::string& expected_prefs_chromeos) const {
+  if (expected_prefs.empty()) {
+    EXPECT_TRUE(prefs_->GetString(language::prefs::kAcceptLanguages).empty());
+  } else {
+    EXPECT_EQ(expected_prefs,
+              prefs_->GetString(language::prefs::kAcceptLanguages));
+  }
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  if (expected_prefs_chromeos.empty()) {
+    EXPECT_TRUE(
+        prefs_->GetString(language::prefs::kPreferredLanguages).empty());
+  } else {
+    EXPECT_EQ(expected_prefs_chromeos,
+              prefs_->GetString(language::prefs::kPreferredLanguages));
+  }
+#endif
+}
+
+// Similar to function above: this one expects both ChromeOS and other
+// platforms to have the same value of language prefs.
+void AcceptLanguagesTester::ExpectLanguagePrefs(
+    const std::string& expected_prefs) const {
+  ExpectLanguagePrefs(expected_prefs, expected_prefs);
+}
+
+void AcceptLanguagesTester::SetLanguagePrefs(
+    const std::vector<std::string>& languages) {
+  std::string languages_str = base::JoinString(languages, ",");
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  prefs_->SetString(language::prefs::kPreferredLanguages, languages_str);
+#endif
+
+  prefs_->SetString(language::prefs::kAcceptLanguages, languages_str);
+}
+
+}  // namespace test
+}  // namespace language

components/language/core/browser/language_prefs_test_util.h

+// Copyright 2020 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.
+
+#ifndef COMPONENTS_LANGUAGE_CORE_BROWSER_LANGUAGE_PREFS_TEST_UTIL_H_
+#define COMPONENTS_LANGUAGE_CORE_BROWSER_LANGUAGE_PREFS_TEST_UTIL_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "base/strings/string_piece.h"
+
+class PrefService;
+
+namespace language {
+namespace test {
+
+// Helper class for testing Accept Languages.
+class AcceptLanguagesTester {
+ public:
+  explicit AcceptLanguagesTester(PrefService* user_prefs);
+
+  // Checks that the provided strings are equivalent to the content language
+  // prefs. Chrome OS uses a different pref, so we need to handle it separately.
+  void ExpectLanguagePrefs(const std::string& expected_prefs,
+                           const std::string& expected_prefs_chromeos) const;
+
+  // Similar to function above: this one expects both ChromeOS and other
+  // platforms to have the same value of language prefs.
+  void ExpectLanguagePrefs(const std::string& expected_prefs) const;
+
+  void SetLanguagePrefs(const std::vector<std::string>& languages);
+
+ private:
+  PrefService* prefs_;
+};
+
+}  // namespace test
+}  // namespace language
+
+#endif  // COMPONENTS_LANGUAGE_CORE_BROWSER_LANGUAGE_PREFS_TEST_UTIL_H_

components/language/core/browser/language_prefs_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/language/core/browser/language_prefs_test_util.h"
 #include "components/language/core/browser/pref_names.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
@@ -41,6 +42,13 @@ class LanguagePrefsTest : public testing::Test {
     language_prefs_.reset(new language::LanguagePrefs(prefs_.get()));
   }
 
+  void SetUp() override {
+    prefs_->SetString(language::prefs::kAcceptLanguages, std::string());
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+    prefs_->SetString(language::prefs::kPreferredLanguages, std::string());
+#endif
+  }
+
   void ExpectFluentLanguageListContent(
       const std::vector<std::string>& list) const {
     const base::Value* values = prefs_->Get(language::prefs::kFluentLanguages);
@@ -158,4 +166,33 @@ TEST_F(LanguagePrefsTest, GetFirstLanguageTest) {
   EXPECT_EQ("", language::GetFirstLanguage(""));
 }
 
+TEST_F(LanguagePrefsTest, UpdateLanguageList) {
+  language::test::AcceptLanguagesTester content_languages_tester =
+      language::test::AcceptLanguagesTester(prefs_.get());
+  // Empty update.
+  std::vector<std::string> languages;
+  language_prefs_->SetAcceptLanguagesList(languages);
+  content_languages_tester.ExpectLanguagePrefs("");
+
+  // One language.
+  languages = {"en"};
+  language_prefs_->SetAcceptLanguagesList(languages);
+  content_languages_tester.ExpectLanguagePrefs("en");
+
+  // More than one language.
+  languages = {"en", "ja", "it"};
+  language_prefs_->SetAcceptLanguagesList(languages);
+  content_languages_tester.ExpectLanguagePrefs("en,ja,it");
+
+  // Locale-specific codes.
+  // The list is exanded by adding the base languagese.
+  languages = {"en-US", "ja", "en-CA", "fr-CA"};
+  language_prefs_->SetAcceptLanguagesList(languages);
+  content_languages_tester.ExpectLanguagePrefs("en-US,ja,en-CA,fr-CA");
+
+  // List already expanded.
+  languages = {"en-US", "en", "fr", "fr-CA"};
+  language_prefs_->SetAcceptLanguagesList(languages);
+  content_languages_tester.ExpectLanguagePrefs("en-US,en,fr,fr-CA");
+}
 }  // namespace language

components/translate/core/browser/BUILD.gn

@@ -120,6 +120,7 @@ source_set("unit_tests") {
     "//components/assist_ranker/proto",
     "//components/infobars/core",
     "//components/language/core/browser",
+    "//components/language/core/browser:test_support",
     "//components/language/core/common:common",
     "//components/pref_registry:pref_registry",
     "//components/prefs",

components/translate/core/browser/mock_translate_client.cc

@@ -34,8 +34,7 @@ PrefService* MockTranslateClient::GetPrefs() {
 }
 
 std::unique_ptr<TranslatePrefs> MockTranslateClient::GetTranslatePrefs() {
-  return std::make_unique<TranslatePrefs>(prefs_, accept_languages_prefs,
-                                          preferred_languages_prefs);
+  return std::make_unique<TranslatePrefs>(prefs_);
 }
 
 }  // namespace testing

components/translate/core/browser/translate_manager_unittest.cc

@@ -132,9 +132,7 @@ class TranslateManagerTest : public ::testing::Test {
  protected:
   TranslateManagerTest()
       : registration_(&prefs_),
-        translate_prefs_(&prefs_,
-                         accept_languages_prefs,
-                         preferred_languages_prefs),
+        translate_prefs_(&prefs_),
         manager_(TranslateDownloadManager::GetInstance()),
         mock_translate_client_(&driver_, &prefs_),
         mock_language_model_({MockLanguageModel::LanguageDetails("en", 1.0)}) {}

components/translate/core/browser/translate_prefs.cc

@@ -22,7 +22,6 @@
 #include "base/util/values/values_util.h"
 #include "base/values.h"
 #include "build/build_config.h"
-#include "build/chromeos_buildflags.h"
 #include "components/language/core/browser/language_prefs.h"
 #include "components/language/core/common/language_experiments.h"
 #include "components/language/core/common/language_util.h"
@@ -81,7 +80,7 @@ void PurgeUnsupportedLanguagesInLanguageFamily(base::StringPiece language,
 
 }  // namespace
 
-const char kForceTriggerTranslateCount[] =
+const char TranslatePrefs::kPrefForceTriggerTranslateCount[] =
     "translate_force_trigger_on_english_count_for_backoff_1";
 const char TranslatePrefs::kPrefNeverPromptSitesDeprecated[] =
     "translate_site_blacklist";
@@ -190,17 +189,9 @@ TranslateLanguageInfo& TranslateLanguageInfo::operator=(
 TranslateLanguageInfo& TranslateLanguageInfo::operator=(
     TranslateLanguageInfo&&) noexcept = default;
 
-TranslatePrefs::TranslatePrefs(PrefService* user_prefs,
-                               const char* accept_languages_pref,
-                               const char* preferred_languages_pref)
-    : accept_languages_pref_(accept_languages_pref),
-      prefs_(user_prefs),
+TranslatePrefs::TranslatePrefs(PrefService* user_prefs)
+    : prefs_(user_prefs),
       language_prefs_(std::make_unique<language::LanguagePrefs>(user_prefs)) {
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  preferred_languages_pref_ = preferred_languages_pref;
-#else
-  DCHECK(!preferred_languages_pref);
-#endif
   MigrateNeverPromptSites();
   ResetEmptyBlockedLanguagesToDefaults();
 }
@@ -278,7 +269,7 @@ void TranslatePrefs::AddToLanguageList(base::StringPiece input_language,
   // Add the language to the list.
   if (!base::Contains(languages, chrome_language)) {
     languages.push_back(chrome_language);
-    UpdateLanguageList(languages);
+    language_prefs_->SetAcceptLanguagesList(languages);
   }
 }
 
@@ -303,7 +294,7 @@ void TranslatePrefs::RemoveFromLanguageList(base::StringPiece input_language) {
 
     languages.erase(it);
     PurgeUnsupportedLanguagesInLanguageFamily(chrome_language, &languages);
-    UpdateLanguageList(languages);
+    language_prefs_->SetAcceptLanguagesList(languages);
 
     // We should unblock the language if this was the last one from the same
     // language family.
@@ -384,12 +375,12 @@ void TranslatePrefs::RearrangeLanguage(
       return;
   }
 
-  UpdateLanguageList(languages);
+  language_prefs_->SetAcceptLanguagesList(languages);
 }
 
 void TranslatePrefs::SetLanguageOrder(
     const std::vector<std::string>& new_order) {
-  UpdateLanguageList(new_order);
+  language_prefs_->SetAcceptLanguagesList(new_order);
 }
 
 // static
@@ -735,28 +726,7 @@ void TranslatePrefs::ResetDenialState() {
 
 void TranslatePrefs::GetLanguageList(
     std::vector<std::string>* const languages) const {
-  DCHECK(languages);
-  DCHECK(languages->empty());
-
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  const std::string& key = preferred_languages_pref_;
-#else
-  const std::string& key = accept_languages_pref_;
-#endif
-
-  *languages = base::SplitString(prefs_->GetString(key), ",",
-                                 base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-}
-
-void TranslatePrefs::UpdateLanguageList(
-    const std::vector<std::string>& languages) {
-  std::string languages_str = base::JoinString(languages, ",");
-
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  prefs_->SetString(preferred_languages_pref_, languages_str);
-#endif
-
-  prefs_->SetString(accept_languages_pref_, languages_str);
+  language_prefs_->GetAcceptLanguagesList(languages);
 }
 
 bool TranslatePrefs::CanTranslateLanguage(
@@ -811,19 +781,19 @@ std::string TranslatePrefs::GetRecentTargetLanguage() const {
 }
 
 int TranslatePrefs::GetForceTriggerOnEnglishPagesCount() const {
-  return prefs_->GetInteger(kForceTriggerTranslateCount);
+  return prefs_->GetInteger(kPrefForceTriggerTranslateCount);
 }
 
 void TranslatePrefs::ReportForceTriggerOnEnglishPages() {
   int current_count = GetForceTriggerOnEnglishPagesCount();
   if (current_count != -1 && current_count < std::numeric_limits<int>::max())
-    prefs_->SetInteger(kForceTriggerTranslateCount, current_count + 1);
+    prefs_->SetInteger(kPrefForceTriggerTranslateCount, current_count + 1);
 }
 
 void TranslatePrefs::ReportAcceptedAfterForceTriggerOnEnglishPages() {
   int current_count = GetForceTriggerOnEnglishPagesCount();
   if (current_count != -1)
-    prefs_->SetInteger(kForceTriggerTranslateCount, -1);
+    prefs_->SetInteger(kPrefForceTriggerTranslateCount, -1);
 }
 
 // static
@@ -853,7 +823,7 @@ void TranslatePrefs::RegisterProfilePrefs(
   registry->RegisterStringPref(kPrefTranslateRecentTarget, "",
                                user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterIntegerPref(
-      kForceTriggerTranslateCount, 0,
+      kPrefForceTriggerTranslateCount, 0,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 
 #if defined(OS_ANDROID) || defined(OS_IOS)

components/translate/core/browser/translate_prefs.h

@@ -17,7 +17,6 @@
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "build/chromeos_buildflags.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "url/gurl.h"
 
@@ -159,11 +158,7 @@ class TranslatePrefs {
     kDown
   };
 
-  // |preferred_languages_pref| is only used on Chrome OS, other platforms must
-  // pass NULL.
-  TranslatePrefs(PrefService* user_prefs,
-                 const char* accept_languages_pref,
-                 const char* preferred_languages_pref);
+  explicit TranslatePrefs(PrefService* user_prefs);
 
   ~TranslatePrefs();
 
@@ -335,7 +330,6 @@ class TranslatePrefs {
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(TranslatePrefsTest, UpdateLanguageList);
   FRIEND_TEST_ALL_PREFIXES(TranslatePrefsTest,
                            UpdateLanguageListFeatureEnabled);
   FRIEND_TEST_ALL_PREFIXES(TranslatePrefsTest, BlockLanguage);
@@ -353,9 +347,6 @@ class TranslatePrefs {
   FRIEND_TEST_ALL_PREFIXES(TranslatePrefsTest, MoveLanguageDown);
   friend class TranslatePrefsTest;
 
-  // Updates the language list of the language settings.
-  void UpdateLanguageList(const std::vector<std::string>& languages);
-
   void ResetBlockedLanguagesToDefault();
   void ClearNeverPromptSiteList();
   void ClearAlwaysTranslateLanguagePairs();
@@ -370,14 +361,6 @@ class TranslatePrefs {
 
   bool IsDictionaryEmpty(const char* pref_id) const;
 
-  // Path to the preference storing the accept languages.
-  const std::string accept_languages_pref_;
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  // Path to the preference storing the preferred languages.
-  // Only used on ChromeOS.
-  std::string preferred_languages_pref_;
-#endif
-
   // Retrieves the dictionary mapping the number of times translation has been
   // denied for a language, creating it if necessary.
   base::DictionaryValue* GetTranslationDeniedCountDictionary();

ios/chrome/browser/translate/chrome_ios_translate_client.mm

@@ -82,8 +82,7 @@ ChromeIOSTranslateClient::~ChromeIOSTranslateClient() {
 std::unique_ptr<translate::TranslatePrefs>
 ChromeIOSTranslateClient::CreateTranslatePrefs(PrefService* prefs) {
   return std::unique_ptr<translate::TranslatePrefs>(
-      new translate::TranslatePrefs(prefs, language::prefs::kAcceptLanguages,
-                                    nullptr));
+      new translate::TranslatePrefs(prefs));
 }
 
 translate::TranslateManager* ChromeIOSTranslateClient::GetTranslateManager() {

ios/web_view/internal/cwv_preferences.mm

@@ -38,9 +38,7 @@
 }
 
 - (void)resetTranslationSettings {
-  translate::TranslatePrefs translatePrefs(
-      _prefService, language::prefs::kAcceptLanguages,
-      /*preferred_languages_pref=*/nullptr);
+  translate::TranslatePrefs translatePrefs(_prefService);
   translatePrefs.ResetToDefaults();
 }
 

ios/web_view/internal/translate/web_view_translate_client.mm

@@ -112,9 +112,7 @@ PrefService* WebViewTranslateClient::GetPrefs() {
 
 std::unique_ptr<translate::TranslatePrefs>
 WebViewTranslateClient::GetTranslatePrefs() {
-  return std::make_unique<translate::TranslatePrefs>(
-      GetPrefs(), language::prefs::kAcceptLanguages,
-      /*preferred_languages_pref=*/nullptr);
+  return std::make_unique<translate::TranslatePrefs>(GetPrefs());
 }
 
 translate::TranslateAcceptLanguages*

weblayer/browser/translate_client_impl.cc

@@ -34,8 +34,7 @@ namespace {
 std::unique_ptr<translate::TranslatePrefs> CreateTranslatePrefs(
     PrefService* prefs) {
   std::unique_ptr<translate::TranslatePrefs> translate_prefs(
-      new translate::TranslatePrefs(prefs, language::prefs::kAcceptLanguages,
-                                    /*preferred_languages_pref=*/nullptr));
+      new translate::TranslatePrefs(prefs));
 
   // We need to obtain the country here, since it comes from VariationsService.
   // components/ does not have access to that.