Support multiple locales string for accept language list

chrome/browser/android/preferences/pref_service_bridge.cc

 // Copyright 2014 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/android/preferences/pref_service_bridge.h"
 
 #include <jni.h>
 #include <stddef.h>
 
 #include <memory>
 #include <vector>
 
 #include "base/android/build_info.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #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"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_filter_builder.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_factory.h"
 #include "chrome/browser/browsing_data/registrable_domain_filter_builder.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ skipping 1104 matching lines @@
 
 // static
 bool PrefServiceBridge::RegisterPrefServiceBridge(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 // static
 // 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::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));
 
-  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";
+    }
 
-  // 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 ||

[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);
+      }
+    }
   }
-
-  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);
-    }
-    parts.push_back(*accept_languages);
-    *accept_languages = base::JoinString(parts, ",");
-  }
+  parts.push_back(*accept_languages);
+  *accept_languages = base::JoinString(parts, ",");
 }
 
 // static
 std::string PrefServiceBridge::GetAndroidPermissionForContentSetting(
     ContentSettingsType content_type) {
   JNIEnv* env = AttachCurrentThread();
   base::android::ScopedJavaLocalRef<jstring> android_permission =
       Java_PrefServiceBridge_getAndroidPermissionForContentSetting(
           env, content_type);
   if (android_permission.is_null())
     return std::string();
 
   return ConvertJavaStringToUTF8(android_permission);
 }
 
 static void SetSupervisedUserId(JNIEnv* env,
                                 const JavaParamRef<jobject>& obj,
                                 const JavaParamRef<jstring>& pref) {
   GetPrefService()->SetString(prefs::kSupervisedUserId,
                               ConvertJavaStringToUTF8(env, pref));
 }