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/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/scoped_observer.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"
@@ skipping 1025 matching lines @@
 
 static void SetEulaAccepted(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   g_browser_process->local_state()->SetBoolean(prefs::kEulaAccepted, true);
 }
 
 static void ResetAcceptLanguages(JNIEnv* env,
                                  const JavaParamRef<jobject>& obj,
                                  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);
 }
 
 // Sends all information about the different versions to Java.
 // From browser_about_handler.cc
 static ScopedJavaLocalRef<jobject> GetAboutVersionStrings(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
@@ skipping 61 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
 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);
+  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));
 
-  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);
+    // 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";
     }
-    parts.push_back(*accept_languages);
-    *accept_languages = base::JoinString(parts, ",");
+
+    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, ",");
 }
 
 // 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));
 }