Support multiple locales string for accept language list

chrome/browser/android/preferences/pref_service_bridge.cc

Index: chrome/browser/android/preferences/pref_service_bridge.cc
diff --git a/chrome/browser/android/preferences/pref_service_bridge.cc b/chrome/browser/android/preferences/pref_service_bridge.cc
index 4ddaa35f57066297b40d4ccac5e6c67181670552..263154661ae40fe3dc59139cff0608ef2b7b2dc9 100644
--- a/chrome/browser/android/preferences/pref_service_bridge.cc
+++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -18,6 +18,7 @@
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/scoped_observer.h"
 #include "base/strings/string_util.h"
@@ -1063,7 +1064,6 @@ static void ResetAcceptLanguages(JNIEnv* env,
                                  const JavaParamRef<jstring>& default_locale) {
   std::string accept_languages(l10n_util::GetStringUTF8(IDS_ACCEPT_LANGUAGES));
   std::string locale_string(ConvertJavaStringToUTF8(env, default_locale));
-
   PrefServiceBridge::PrependToAcceptLanguagesIfNecessary(locale_string,
                                                          &accept_languages);
   GetPrefService()->SetString(prefs::kAcceptLanguages, accept_languages);
@@ -1147,39 +1147,62 @@ bool PrefServiceBridge::RegisterPrefServiceBridge(JNIEnv* env) {
 void PrefServiceBridge::PrependToAcceptLanguagesIfNecessary(
     const std::string& locale,

[Seigo Nonaka, 2016/10/06 02:02:18]

This is now comma-separated multiple locales. Could you change the argument name
to "locales"?

Also, it is good to leave comment the expected input format in the function
comment, for example,

|locales| is a comma separated language tags. Each language tag should be xx_XX
style, here xx is a 2-letter ISO 639-1 compliant language code and XX is a
2-letter ISO 3166-1 compliant country code.

Then let's leave a TODO comment like

TODO(yirui): Support BCP47 compliant format including 3-letter country code, '-'
separator and missing region case.

[Yirui Huang, 2016/10/06 02:29:11]

Done.

     std::string* accept_languages) {
-  if (locale.size() != 5u || locale[2] != '_')  // not well-formed
-    return;
-
-  std::string language(locale.substr(0, 2));
-  std::string region(locale.substr(3, 2));
-
-  // Java mostly follows ISO-639-1 and ICU, except for the following three.
-  // See documentation on java.util.Locale constructor for more.
-  if (language == "iw") {
-    language = "he";
-  } else if (language == "ji") {
-    language = "yi";
-  } else if (language == "in") {
-    language = "id";
+  std::vector<std::string> languages;
+  std::size_t max_check_length = locale.length();

[Seigo Nonaka, 2016/10/06 02:02:18]

I think you can just use size_t here (dropping std:: prefix).

[Yirui Huang, 2016/10/06 02:29:11]

Done.

+  std::size_t check = 0;
+  // Check see if it is a Locale List or single Locale
+  if (locale.length() > 5) {

[Seigo Nonaka, 2016/10/06 02:02:18]

BTW, I think you can use StringSplit here.
https://cs.chromium.org/chromium/src/base/strings/string_split.h?rcl=0&l=46

[Yirui Huang, 2016/10/06 02:29:11]

Done.

+    while (locale.find(",", check) != std::string::npos) {
+      std::size_t found = locale.find(",", check);
+      languages.push_back(locale.substr(check, found - check));
+      check = found + 1;
+      // Check the last element
+      if (max_check_length - check <= 5) {
+        languages.push_back(locale.substr(check));
+        break;
+      }
+    }
+  } else {
+    languages.push_back(locale);
   }
 
-  std::string language_region(language + "-" + region);
-
-  if (accept_languages->find(language_region) == std::string::npos) {
-    std::vector<std::string> parts;
-    parts.push_back(language_region);
-    // If language is not in the accept languages list, also add language code.
-    // This will work with the IDS_ACCEPT_LANGUAGE localized strings bundled
-    // with Chrome but may fail on arbitrary lists of language tags due to
-    // differences in case and whitespace.
-    if (accept_languages->find(language + ",") == std::string::npos &&
-        !std::equal(language.rbegin(), language.rend(),
-                    accept_languages->rbegin())) {
-      parts.push_back(language);
+  std::vector<std::string> parts;
+  for (std::size_t i = 0; i < languages.size(); i++) {
+    // Since in Android Language Input, region is always attached with language.
+    // Therefore, an unwell-formed input, such as 'ms', should not be expected.
+    DCHECK(languages[i].size() == 5u && languages[i][2] == '_');

[Seigo Nonaka, 2016/10/06 02:02:18]

As we chatted offline, sorry DCHECK is bit aggressive in this case.
Eventually we may need to support missing region cases, but at this moment,
let's keep the existing behaviors. Then how about

for (size_t i = 0; i < language.size(); ++i) {
  if (languages[i].size() != 5u || languages[i][2] != '_') {
    TODO(yirui): Support BCP47 compliant format including 3-letter country code,
'-' separator and missing region case.
    continue; 
  }
}

[Yirui Huang, 2016/10/06 02:29:11]

Done.

+    std::string language(languages[i].substr(0, 2));
+    std::string region(languages[i].substr(3, 2));
+
+    // Java mostly follows ISO-639-1 and ICU, except for the following three.
+    // See documentation on java.util.Locale constructor for more.
+    if (language == "iw") {
+      language = "he";
+    } else if (language == "ji") {
+      language = "yi";
+    } else if (language == "in") {
+      language = "id";
+    }
+
+    std::string language_region(language + "-" + region);
+
+    if (accept_languages->find(language_region) == std::string::npos) {
+      parts.push_back(language_region);
+      // If language is not in the accept languages list, also add language
+      // code.
+      // This will work with the IDS_ACCEPT_LANGUAGE localized strings bundled
+      // with Chrome but may fail on arbitrary lists of language tags due to
+      // differences in case and whitespace.
+      if ((accept_languages->find(language) == std::string::npos ||
+           accept_languages->find(language + ",") == std::string::npos) &&
+          !std::equal(language.rbegin(), language.rend(),
+                      accept_languages->rbegin())) {
+        parts.push_back(language);
+      }
     }
-    parts.push_back(*accept_languages);
-    *accept_languages = base::JoinString(parts, ",");
   }
+  parts.push_back(*accept_languages);
+  *accept_languages = base::JoinString(parts, ",");
 }
 
 // static