TranslatePrefTest: Added test case for UpdateLanguageList.

components/translate/core/browser/translate_prefs_unittest.cc

Index: components/translate/core/browser/translate_prefs_unittest.cc
diff --git a/components/translate/core/browser/translate_prefs_unittest.cc b/components/translate/core/browser/translate_prefs_unittest.cc
index 8ef04f99ec93bcaf71cd16bd51d41e643e4f6ef5..13364591c2e714acad7c9fba30e35d56a40700bb 100644
--- a/components/translate/core/browser/translate_prefs_unittest.cc
+++ b/components/translate/core/browser/translate_prefs_unittest.cc
@@ -23,6 +23,13 @@ namespace {
 
 const char kTestLanguage[] = "en";
 
+#if defined(OS_CHROMEOS)

[groby-ooo-7-16, 2017/03/08 00:49:15]

If you're refactoring this anyways - could you factor this out into a function?
There are _numerous_ tests that do exactly this, and it's... annoying. (It's
also annoying that tests in components/ depend on things defined in chrome/, but
that's a larger story :)

[Matt Giuca, 2017/03/15 02:45:18]

Hmm, I had a bit of a think about it but it's not quite clear where this would
go (it doesn't really belong in translate_prefs.h, because it's only used in
tests). It isn't clear to me why this pref name is parameterised in the code
anyway (and only concrete in tests); why isn't it just hard-coded in the code
itself?

I'd rather not have to solve this issue: this CL is a side-yak on a
codebase-wide refactor (https://codereview.chromium.org/2695883003) and I can't
stop to clean up nearby problems in all of those places. I already went one step
further here and added a test. (This is kind of time-critical because the longer
it takes to land that CL, the more merge conflicts I have to deal with.)

+const char kPreferredLanguagesPref[] = "settings.language.preferred_languages";
+#else
+const char* kPreferredLanguagesPref = nullptr;
+#endif
+const char kAcceptLanguagesPref[] = "intl.accept_languages";
+
 }  // namespace
 
 namespace translate {
@@ -31,19 +38,21 @@ class TranslatePrefTest : public testing::Test {
  protected:
   TranslatePrefTest()
       : prefs_(new sync_preferences::TestingPrefServiceSyncable()) {
-#if defined(OS_CHROMEOS)
-    const char* preferred_languages_prefs =
-        "settings.language.preferred_languages";
-#else
-    const char* preferred_languages_prefs = NULL;
-#endif
     translate_prefs_.reset(new translate::TranslatePrefs(
-        prefs_.get(), "intl.accept_languages", preferred_languages_prefs));
+        prefs_.get(), kAcceptLanguagesPref, kPreferredLanguagesPref));
     TranslatePrefs::RegisterProfilePrefs(prefs_->registry());
     now_ = base::Time::Now();
     two_days_ago_ = now_ - base::TimeDelta::FromDays(2);
   }
 
+  void SetUp() override {
+    prefs_->registry()->RegisterStringPref(kAcceptLanguagesPref, std::string());
+#if defined(OS_CHROMEOS)
+    prefs_->registry()->RegisterStringPref(kPreferredLanguagesPref,

[groby-ooo-7-16, 2017/03/08 00:49:15]

I'd really like to break the defined(OS_CHROMEOS) dependency - would you mind
changing this to 

if(kPreferredLanguagesPref)
  ...RegisterStringPref(kPreferred...

[Matt Giuca, 2017/03/15 02:45:18]

Done.

[Matt Giuca, 2017/03/15 07:45:14]

Undone... unfortunately I get this compile error on ChromeOS:

error: address of array 'kPreferredLanguagesPref' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
    if (kPreferredLanguagesPref) {
    ~~  ^~~~~~~~~~~~~~~~~~~~~~~

So this has to remain a compile-time check.

+                                           std::string());
+#endif
+  }
+
   void SetLastDeniedTime(const std::string& language, base::Time time) {
     DenialTimeUpdate update(prefs_.get(), language, 2);
     update.AddDenialTime(time);
@@ -212,6 +221,23 @@ TEST_F(TranslatePrefTest, DenialTimeUpdate_SlidingWindow) {
             now_ - base::TimeDelta::FromMinutes(2));
 }
 
+TEST_F(TranslatePrefTest, UpdateLanguageList) {
+  // Test with basic set of languages (no country codes).
+  std::vector<std::string> languages{"en", "ja"};
+  translate_prefs_->UpdateLanguageList(languages);
+  EXPECT_EQ("en,ja", prefs_->GetString(kAcceptLanguagesPref));
+
+  // Test with languages that have country codes. Expect accepted languages both
+  // with and without a country code. (See documentation for
+  // ExpandLanguageCodes.)
+  languages = {"en-US", "ja", "en-CA"};
+  translate_prefs_->UpdateLanguageList(languages);
+#if defined(OS_CHROMEOS)
+  EXPECT_EQ("en-US,ja,en-CA", prefs_->GetString(kPreferredLanguagesPref));
+#endif
+  EXPECT_EQ("en-US,en,ja,en-CA", prefs_->GetString(kAcceptLanguagesPref));
+}
+
 TEST_F(TranslatePrefTest, ULPPrefs) {
   // Mock the pref.
   // Case 1: well formed ULP.