|
|
Created:
4 years ago by Yirui Huang Modified:
3 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, lcwu+watch_chromium.org, pkl (ping after 24h if needed), halliwell+watch_chromium.org, agrieve+watch_chromium.org, alokp+watch_chromium.org, android-webview-reviews_chromium.org, jshin+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend with a language code in http accept languages
This CL is a clean up for accept languages in HTTP header.
Both Android Chrome and Android Webview will return a unique language
list. Then in the http header generater, the unique language list
will be expanded with a langauge code. In this way, it keeps the same
format in final accept languages that appear in http header for both
Chrome and Webview.
Since http_util could not use icu, this CL separated method
GenerateAcceptLanguageHeader into a new file http_util_icu under net.
This CL maintains the addition of en-US for accept languages in
Android Webview as before.
BUG=667705
Patch Set 1 : Rebased separate Generate AcceptLanguage Header from http_util #
Total comments: 24
Patch Set 2 : Removed unnecessary imports and checked develop path #
Total comments: 2
Patch Set 3 : Rebased and modified descriptions #
Total comments: 4
Patch Set 4 : Rebased and removed unnecessary import #
Total comments: 2
Patch Set 5 : Rebased and used namespcace instead of class #
Total comments: 28
Patch Set 6 : Rebased, fixed nits and handed over this CL :( #Messages
Total messages: 68 (48 generated)
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Extend with a language code in http accept languages This CL is a clean up for accept languages in HTTP header. Both Android Chrome and Android Webview will return a unique language list. Then in the http header generater, the unique language list will be expanded with a langauge code. In this way, it keeps the same format in final accept languages that appear in http header for both Chrome and Webview. This CL maintains the addition of en-US for accept languages in Android Webview as before. BUG=667705 ========== to ========== Extend with a language code in http accept languages This CL is a clean up for accept languages in HTTP header. Both Android Chrome and Android Webview will return a unique language list. Then in the http header generater, the unique language list will be expanded with a langauge code. In this way, it keeps the same format in final accept languages that appear in http header for both Chrome and Webview. Since http_util could not use icu, this CL separated method GenerateAcceptLanguageHeader into a new file http_util_icu under net. This CL maintains the addition of en-US for accept languages in Android Webview as before. BUG=667705 ==========
yirui@google.com changed reviewers: + ksk@google.com, nona@chromium.org
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/2559243003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2559243003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:159: // Test language code is inserted only after the last language tag that nit: continue line until 100chars? https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:19: #include "base/time/time.h" probably, some of headers are not necessary, e.g. time.h. Please minimize the header inclusions. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:28: // TODO(yirui): We still need to be aware of cases, make it more robust. AndroidWebView is the only case that the developer override the accept language. Could you kindly check what string is passed here (or simply this is not called) with following case. Map<String, String> headers = new HashMap<>(); headers.put("Accept-Language", "xxx,yyy;q=0.8"); webview.loadUrl("http://www.google.com", headers); If the passed accept language string is passed without any modification, we may need to fix this function. However, even in that case, the existing code also has the same issue, so please feel free to leave TODO. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:37: // This will work with the IDS_ACCEPT_LANGUAGE localized strings bundled I think this comment is no longer necessary. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:81: while (t.GetNext()) { Let's use output_list here. You joined string then split it again. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.h:16: // Given a comma separated ordered list of language codes, return nit: continue line until 80 chars. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... File net/http/http_util_icu_unittest.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:5: #include <algorithm> remove? https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:6: #include <limits> remove? https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:8: #include "base/strings/string_util.h" remove? https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:15: class HttpUtilIcuTest : public testing::Test {}; Not necessary. Please remove this class declaration.
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2559243003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2559243003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:159: // Test language code is inserted only after the last language tag that On 2016/12/12 01:48:54, Seigo Nonaka wrote: > nit: continue line until 100chars? Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:19: #include "base/time/time.h" On 2016/12/12 01:48:54, Seigo Nonaka wrote: > probably, some of headers are not necessary, e.g. time.h. > Please minimize the header inclusions. Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:28: // TODO(yirui): We still need to be aware of cases, make it more robust. On 2016/12/12 01:48:54, Seigo Nonaka wrote: > AndroidWebView is the only case that the developer override the accept language. > Could you kindly check what string is passed here (or simply this is not called) > with following case. > > Map<String, String> headers = new HashMap<>(); > headers.put("Accept-Language", "xxx,yyy;q=0.8"); > webview.loadUrl("http://www.google.com", headers); > > If the passed accept language string is passed without any modification, we may > need to fix this function. However, even in that case, the existing code also > has the same issue, so please feel free to leave TODO. The change from developers will not use http_util, therefore, raw string will be in well form. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:37: // This will work with the IDS_ACCEPT_LANGUAGE localized strings bundled On 2016/12/12 01:48:54, Seigo Nonaka wrote: > I think this comment is no longer necessary. Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:81: while (t.GetNext()) { On 2016/12/12 01:48:54, Seigo Nonaka wrote: > Let's use output_list here. You joined string then split it again. Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.h:16: // Given a comma separated ordered list of language codes, return On 2016/12/12 01:48:54, Seigo Nonaka wrote: > nit: continue line until 80 chars. Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... File net/http/http_util_icu_unittest.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/12/12 01:48:54, Seigo Nonaka wrote: > 2016 Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:5: #include <algorithm> On 2016/12/12 01:48:54, Seigo Nonaka wrote: > remove? Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:6: #include <limits> On 2016/12/12 01:48:54, Seigo Nonaka wrote: > remove? Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:8: #include "base/strings/string_util.h" On 2016/12/12 01:48:54, Seigo Nonaka wrote: > remove? Done. https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu_... net/http/http_util_icu_unittest.cc:15: class HttpUtilIcuTest : public testing::Test {}; On 2016/12/12 01:48:54, Seigo Nonaka wrote: > Not necessary. Please remove this class declaration. Done.
https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:9: #include <string> string is necessary for std::string https://codereview.chromium.org/2559243003/diff/80001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/80001/net/http/http_util_icu.... net/http/http_util_icu.cc:21: // customixed from developers are not processed here. nit: /customixed/customized/ And it may be good to mention about Android WebView. How about Similarly, the accept languages on Android WebView can be overridden by developers, but they are not processed here.
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/40001/net/http/http_util_icu.... net/http/http_util_icu.cc:9: #include <string> On 2016/12/12 07:29:26, Seigo Nonaka wrote: > string is necessary for std::string Done. https://codereview.chromium.org/2559243003/diff/80001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/80001/net/http/http_util_icu.... net/http/http_util_icu.cc:21: // customixed from developers are not processed here. On 2016/12/12 07:29:26, Seigo Nonaka wrote: > nit: /customixed/customized/ > And it may be good to mention about Android WebView. > > How about > > Similarly, the accept languages on Android WebView can be overridden by > developers, but they are not processed here. Modified with Android Webview. "+ including those from Android Webview,"
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. Please get review from net owner and Jungshik. https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:13: #include "net/http/http_util.h" nit: Please remove https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... File ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm (right): https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm:11: #include "net/http/http_util.h" nit: Please remove
+ droger@ as an owner for ios/ + slan@ as an owner for chromecast/ + torne@ as an owner for android_webview/ + rsleevi@ as an owner for chrome/browser/net/ and net/ + mariakhomenko@ as an owner for chrome/android and chrome/browser + jshin@ for net/DEPS Thanks in advance! https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:13: #include "net/http/http_util.h" On 2016/12/14 01:27:21, Seigo Nonaka wrote: > nit: Please remove Done. https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... File ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm (right): https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm:11: #include "net/http/http_util.h" On 2016/12/14 01:27:21, Seigo Nonaka wrote: > nit: Please remove Done.
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/14 01:53:10, Yirui Huang wrote: > + droger@ as an owner for ios/ > + slan@ as an owner for chromecast/ > + torne@ as an owner for android_webview/ > + rsleevi@ as an owner for chrome/browser/net/ and net/ > + mariakhomenko@ as an owner for chrome/android and chrome/browser > + jshin@ for net/DEPS > Thanks in advance! > > https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... > File chromecast/browser/cast_http_user_agent_settings.cc (right): > > https://codereview.chromium.org/2559243003/diff/140001/chromecast/browser/cas... > chromecast/browser/cast_http_user_agent_settings.cc:13: #include > "net/http/http_util.h" > On 2016/12/14 01:27:21, Seigo Nonaka wrote: > > nit: Please remove > > Done. > > https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... > File ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm (right): > > https://codereview.chromium.org/2559243003/diff/140001/ios/chrome/browser/net... > ios/chrome/browser/net/ios_chrome_http_user_agent_settings.mm:11: #include > "net/http/http_util.h" > On 2016/12/14 01:27:21, Seigo Nonaka wrote: > > nit: Please remove > > Done. cast rs lgtm
android_webview LGTM, thanks!
ios lgtm
android lgtm
Sorry I'm slow on this - this is quite subtle and it's going to take me a bit to get the time to concentrate and review the spec as well as go through the non-ICU //net consumers to make sure there's no issues. https://codereview.chromium.org/2559243003/diff/160001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/160001/net/http/http_util_icu... net/http/http_util_icu.h:14: class NET_EXPORT HttpUtilIcu { https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... "Prefer grouping functions with a namespace instead of using a class as if it were a namespace. "
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559243003/diff/160001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/160001/net/http/http_util_icu... net/http/http_util_icu.h:14: class NET_EXPORT HttpUtilIcu { On 2016/12/14 20:02:30, Ryan Sleevi wrote: > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > > "Prefer grouping functions with a namespace instead of using a class as if it > were a namespace. " Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
So I'm still working through timezones with folks on the //net ICU issues, but there's opportunities for improvement here that I think we need to take, so we don't regress compared to the previous performance. https://codereview.chromium.org/2559243003/diff/180001/android_webview/browse... File android_webview/browser/net/aw_http_user_agent_settings.cc (right): https://codereview.chromium.org/2559243003/diff/180001/android_webview/browse... android_webview/browser/net/aw_http_user_agent_settings.cc:29: net::http_util_icu::GenerateAcceptLanguageHeader( So I don't think this does the right thing. Note that our non-ICU platforms are Android and iOS, because the OS already provides native functionality (or, in the case of Android, is effectively shipping ICU, just behind a different API) This is why we have net_string_util_icu_alternatives_* - implementations of the same function set, for the appropriate platform that is setting "no ICU" So in order for this to "be right", I believe this function actually needs an Android-specific, non-ICU dependent solution. https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:262: uniqueList.add(LocaleUtils.toLanguageTag(locale)); I'm having trouble convincing myself this is right, but fair warning: I'm not terribly good at Java :) Previously, uniqueList was truly unique because old-258 made sure no dupes (of locale) are introduced. Your new code searches seenLocales, but adds based on .toLanguageTag. Can two locales result in the same language tag (I thought they could)? is LocaleUtil's not reflexive? That is, line 262 (when combined with 257) could be rewritten as: uniqueList.add(LocaleUtils.toLanguageTag(localeUtils.forLanguageTag(localeString))) If it was reflexive, couldn't this just be written as uniqueList.add(localeString) ? Why or why not? https://codereview.chromium.org/2559243003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (left): https://codereview.chromium.org/2559243003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:64: // Should prepend the language tag "en-GB" as well as the language code "en". Why should "en" no longer be added? https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:18: namespace http_util_icu { line break between 17/18 and 18/19 https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:25: raw_language_list, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); 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? https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:30: std::set<std::string> seen_languages; 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? https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:32: for (auto it = locales_list.rbegin(); it != locales_list.rend(); ++it) { 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) ? https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:61: std::reverse(output_list.begin(), output_list.end()); This is not at all obvious why you reverse here. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:83: } linebreak between 83/84 https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:84: } // namespace http_util_icu https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:85: } // namespace net https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:13: namespace http_util_icu { line break between 12/13 https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:27: std::string NET_EXPORT NET_EXPORT std::string https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:30: } // namespace net line breaks between 28/29 and 29/30
The CQ bit was checked by yirui@google.com to run a CQ dry run
Patchset #6 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559243003/diff/180001/android_webview/browse... File android_webview/browser/net/aw_http_user_agent_settings.cc (right): https://codereview.chromium.org/2559243003/diff/180001/android_webview/browse... android_webview/browser/net/aw_http_user_agent_settings.cc:29: net::http_util_icu::GenerateAcceptLanguageHeader( On 2016/12/15 23:44:07, Ryan Sleevi wrote: > So I don't think this does the right thing. > > Note that our non-ICU platforms are Android and iOS, because the OS already > provides native functionality (or, in the case of Android, is effectively > shipping ICU, just behind a different API) > > This is why we have net_string_util_icu_alternatives_* - implementations of the > same function set, for the appropriate platform that is setting "no ICU" > > So in order for this to "be right", I believe this function actually needs an > Android-specific, non-ICU dependent solution. I will hand over this issue to nona@. Hopefully, he will take over this part. https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:262: uniqueList.add(LocaleUtils.toLanguageTag(locale)); On 2016/12/15 23:44:07, Ryan Sleevi wrote: > I'm having trouble convincing myself this is right, but fair warning: I'm not > terribly good at Java :) > > Previously, uniqueList was truly unique because old-258 made sure no dupes (of > locale) are introduced. > > Your new code searches seenLocales, but adds based on .toLanguageTag. Can two > locales result in the same language tag (I thought they could)? is LocaleUtil's > not reflexive? > > That is, line 262 (when combined with 257) could be rewritten as: > uniqueList.add(LocaleUtils.toLanguageTag(localeUtils.forLanguageTag(localeString))) > > > If it was reflexive, couldn't this just be written as > uniqueList.add(localeString) ? Why or why not? The reason why LocaleUtils is involved is because that Java keeps some deprecated languages while Chromium uses others, a language mapping is applied. https://codereview.chromium.org/2559243003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (left): https://codereview.chromium.org/2559243003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:64: // Should prepend the language tag "en-GB" as well as the language code "en". On 2016/12/15 23:44:07, Ryan Sleevi wrote: > Why should "en" no longer be added? From both Android Chrome and Webview, we are returning a language list that contains unique language tags in this patch. And we try to add language code like "en" in net/http instead of adding individually according to platforms. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu.cc File net/http/http_util_icu.cc (right): https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:18: namespace http_util_icu { On 2016/12/15 23:44:07, Ryan Sleevi wrote: > line break between 17/18 and 18/19 Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:83: } On 2016/12/15 23:44:07, Ryan Sleevi wrote: > linebreak between 83/84 Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:84: } On 2016/12/15 23:44:07, Ryan Sleevi wrote: > // namespace http_util_icu Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.cc:85: } On 2016/12/15 23:44:07, Ryan Sleevi wrote: > // namespace net Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:13: namespace http_util_icu { On 2016/12/15 23:44:07, Ryan Sleevi wrote: > line break between 12/13 Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:27: std::string NET_EXPORT On 2016/12/15 23:44:07, Ryan Sleevi wrote: > NET_EXPORT std::string Done. https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:30: } // namespace net On 2016/12/15 23:44:07, Ryan Sleevi wrote: > line breaks between 28/29 and 29/30 Done.
> This CL maintains the addition of en-US for accept languages in > Android Webview as before. I've been debating the need for appending en-US/en to the end for a very long time. See https://codereview.chromium.org/2049993002/ and crbug.com/36182. Dropping them is likely to be ok, but it's also possible that some servers won't cope very well. For this CL, perhaps, we'd better keep the status quo. Below are a few comments. I may have more after a more thorough look. (sorry for being slow). https://codereview.chromium.org/2559243003/diff/180001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2559243003/diff/180001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:58: * "de-DE,de;q=0.8,en-US;q=0.6,en-UK;q=0.4,en;q=0.2" nit: you meant en-GB instead of en-UK? UK is reserved in ISO 3166 and GB is the country code for United Kingdom. https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2559243003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:262: uniqueList.add(LocaleUtils.toLanguageTag(locale)); On 2016/12/15 23:44:07, Ryan Sleevi wrote: > I'm having trouble convincing myself this is right, but fair warning: I'm not > terribly good at Java :) > > Previously, uniqueList was truly unique because old-258 made sure no dupes (of > locale) are introduced. > > Your new code searches seenLocales, but adds based on .toLanguageTag. Can two > locales result in the same language tag (I thought they could)? is LocaleUtil's > not reflexive? > > That is, line 262 (when combined with 257) could be rewritten as: > uniqueList.add(LocaleUtils.toLanguageTag(localeUtils.forLanguageTag(localeString))) > > > If it was reflexive, couldn't this just be written as > uniqueList.add(localeString) ? Why or why not? It depends on an input string (input locale_string). Round-tripping via forLanguageTag followed by toLanguageTag effectively canonicalize the locale identifier. ( see, for instance, https://cs.chromium.org/chromium/src/v8/src/runtime/runtime-i18n.cc?rcl=0&l=69 ). In this particular case, the source of locale identifiers is not random user input but Chrome itself or Android (locale list). So, I don't see a need to canonicalize (it should be already canonicalized). https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu.h File net/http/http_util_icu.h (right): https://codereview.chromium.org/2559243003/diff/180001/net/http/http_util_icu... net/http/http_util_icu.h:13: namespace http_util_icu { On 2016/12/15 23:44:07, Ryan Sleevi wrote: > line break between 12/13 Do we need '_icu' suffix in the namespace? And, as Ryan mentioned earlier, if we want to support CrNet on iOS/Android without ICU dependency, we need to have an non-ICU implementation.
The CQ bit was checked by yirui@google.com to run a CQ dry run
Patchset #6 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559243003/diff/180001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2559243003/diff/180001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:58: * "de-DE,de;q=0.8,en-US;q=0.6,en-UK;q=0.4,en;q=0.2" On 2016/12/16 08:01:45, jungshik at google wrote: > nit: you meant en-GB instead of en-UK? UK is reserved in ISO 3166 and GB is the > country code for United Kingdom. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nona@ - Do you plan to maintain this CL? Otherwise, I'd like to close it out of the queue if it is abandoned. It seems like there are some worthwhile goals here though, so I'd like to check.
Is someone taking this work over? This does still look like it's useful, and rietveld is going read-only soon.
On 2017/09/25 18:37:29, Torne wrote: > Is someone taking this work over? This does still look like it's useful, and > rietveld is going read-only soon. I'm sorry I lose the context and don't follow the latest code. This code maybe out of date. I'm going to revisit when I have time.
On 2017/09/25 18:37:29, Torne wrote: > Is someone taking this work over? This does still look like it's useful, and > rietveld is going read-only soon. I'm sorry I lose the context and don't follow the latest code. This code maybe out of date. I'm going to revisit when I have time. |