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..b37864adbf382d5b6dda3724e1b30b81b0abf7f7 100644
--- a/chrome/browser/android/preferences/pref_service_bridge.cc
+++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -20,6 +20,7 @@
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/scoped_observer.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/android/preferences/important_sites_util.h"
@@ -1144,42 +1145,52 @@ bool PrefServiceBridge::RegisterPrefServiceBridge(JNIEnv* env) {
 // This logic should be kept in sync with prependToAcceptLanguagesIfNecessary in
 // chrome/android/java/src/org/chromium/chrome/browser/
 //     physicalweb/PwsClientImpl.java
+// Input |locales| is a comma separated language tags. Each language tag should
+// be xx_XX style, where xx is a 2-letter ISO 639-1 compliant language code and

[ksk1, 2016/10/06 08:15:22]

In Java side, language tag indicates xx-XX. Here, language tag indicates xx_XX
style and language_region indicates xx-XX. This might be confusing.

[Yirui Huang, 2016/10/07 04:14:56]

currently, we will keep '_' form for the locales, this could be TODO in the
future for combining '-' and '_' into the same form.

+// XX is a 2-letter ISO 3166-1 compliant country code.
 void PrefServiceBridge::PrependToAcceptLanguagesIfNecessary(
-    const std::string& locale,
+    const std::string& locales,
     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 = base::SplitString(locales,

[ksk1, 2016/10/06 08:15:22]

Can this be locales? languages sounds confusing because this format is different
from |language| nor |accept_languages|.

[Yirui Huang, 2016/10/07 04:14:56]

Done.

+      ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  std::vector<std::string> parts;
+  for (std::size_t i = 0; i < languages.size(); i++) {

[Maria, 2016/10/06 16:38:00]

you can just use size_t here, Chrome defines it (see other examples in this
file)

[Yirui Huang, 2016/10/07 04:14:56]

Done.

+    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;

[Maria, 2016/10/06 16:38:00]

use {} for blocks that are more than one line (e.g. due to comments)

[Yirui Huang, 2016/10/07 04:14:56]

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) {
-    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::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 ||

[ksk1, 2016/10/06 08:15:22]

If I understand correctly, when (accept_languages->find(language) ==
std::string::npos) is true, other 2 conditions should always be true.

Also, this logic is different from Java side code. 
"aa"
It shouldn't be good even if it doesn't happen in practice.

[Yirui Huang, 2016/10/07 04:14:56]

set is used for this checking process.

+           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