Extend with a language code in http accept languages

net/http/http_util_icu.cc

+// Copyright (c) 2016 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 "net/http/http_util_icu.h"
+
+#include <algorithm>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
+#include "third_party/icu/source/common/unicode/uloc.h"
+
+namespace net {
+namespace http_util_icu {

[Ryan Sleevi, 2016/12/15 23:44:07]

line break between 17/18 and 18/19

[Yirui Huang, 2016/12/16 08:00:08]

Done.

+// The input is a comma separated languages list, this function allows
+// whitespace between each languages. It will come from the preference and a
+// user does not manually edit the preference file. The accept languages that
+// customized in Android Webview from developers are not processed here.
+std::string GenerateAcceptLanguageHeader(const std::string& raw_language_list) {
+  std::vector<std::string> locales_list = base::SplitString(
+      raw_language_list, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);

[Ryan Sleevi, 2016/12/15 23:44:07]

The previous design was optimized to avoid needing to copy the strings
(StringTokenizer uses StringPiece)

This new implementation makes quite a few copies. Considering this is in the
critical path for request processing, and considering it doesn't seem too
difficult here to do the efficient thing, can you please update to
reduce/eliminate the additional copies?

+
+  // If language is not in the accept languages list, also add language
+  // code. A language code should only be inserted after the last
+  // languageTag that contains that language.
+  std::set<std::string> seen_languages;

[Ryan Sleevi, 2016/12/15 23:44:07]

This makes a full copy, and the std::set itself isn't terribly efficient. Would
an unordered_set or hash be more appropriate here for what you're testing?

+  std::vector<std::string> output_list;
+  for (auto it = locales_list.rbegin(); it != locales_list.rend(); ++it) {

[Ryan Sleevi, 2016/12/15 23:44:07]

This forces a non-const reference, because the auto can't bind to the const
version of .rbegin()

Why not simply

for (const auto& locale_string : locales_list)

?

+    char locale_ID[ULOC_FULLNAME_CAPACITY] = {};
+    char language_code[ULOC_LANG_CAPACITY] = {};
+
+    UErrorCode error = U_ZERO_ERROR;
+    std::string locale_string = *it;
+    uloc_forLanguageTag(locale_string.c_str(), locale_ID,
+                        ULOC_FULLNAME_CAPACITY, nullptr, &error);
+    if (U_FAILURE(error)) {
+      LOG(ERROR) << "Ignoring invalid locale representation " << locale_string;
+      continue;
+    }
+
+    error = U_ZERO_ERROR;
+    uloc_getLanguage(locale_ID, language_code, ULOC_LANG_CAPACITY, &error);
+    if (U_FAILURE(error)) {
+      LOG(ERROR) << "Ignoring invalid locale representation " << locale_string;
+      continue;
+    }
+
+    if (seen_languages.find(language_code) == seen_languages.end()) {
+      output_list.push_back(language_code);
+      seen_languages.insert(language_code);
+    }
+
+    if (language_code != *it)
+      output_list.push_back(locale_string);
+  }
+
+  std::reverse(output_list.begin(), output_list.end());

[Ryan Sleevi, 2016/12/15 23:44:07]

This is not at all obvious why you reverse here.

+
+  // We use integers for q-value and q-value decrement that are 10 times larger
+  // than actual values to avoid a problem with comparing two floating point
+  // numbers.
+  const unsigned int kQvalueDecrement10 = 2;
+  unsigned int qvalue10 = 10;
+  std::string lang_list_with_q;
+  for (unsigned i = 0; i < output_list.size(); i++) {
+    if (qvalue10 == 10) {
+      // q=1.0 is implicit.
+      lang_list_with_q = output_list[i];
+    } else {
+      DCHECK_LT(qvalue10, 10U);
+      base::StringAppendF(&lang_list_with_q, ",%s;q=0.%d",
+                          output_list[i].c_str(), qvalue10);
+    }
+    // It does not make sense to have 'q=0'.
+    if (qvalue10 > kQvalueDecrement10)
+      qvalue10 -= kQvalueDecrement10;
+  }
+  return lang_list_with_q;
+}

[Ryan Sleevi, 2016/12/15 23:44:07]

linebreak between 83/84

[Yirui Huang, 2016/12/16 08:00:08]

Done.

+}

[Ryan Sleevi, 2016/12/15 23:44:07]

// namespace http_util_icu

[Yirui Huang, 2016/12/16 08:00:08]

Done.

+}

[Ryan Sleevi, 2016/12/15 23:44:07]

// namespace net

[Yirui Huang, 2016/12/16 08:00:08]

Done.