Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393673003:
Support multiple locales string for accept language list (Closed)
|
Created:
4 years, 2 months ago by Yirui Huang Modified:
4 years, 2 months ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport multiple locales string for accept language list
This patch enables PrependToAcceptLanuagesIfNecessary to take
multi-locale string. This is a preparation for supporting
multi-locale feature from Android N. This patch does not break any
existing behaviors.
BUG=593515
Committed: https://crrev.com/6cc907c7b494642fe93ee850439ad816998e21e3
Cr-Commit-Position: refs/heads/master@{#424372}
Patch Set 1 #Patch Set 2 : Fixing Prepend Accepting List for Locale List #
Total comments: 14
Patch Set 3 : Add Multiple Locales Input For Accept Languages #
Total comments: 8
Patch Set 4 : Add Multiple Locales Input For Accept Languages #Patch Set 5 : Add Multiple Locales For Accept Languages List #
Total comments: 16
Patch Set 6 : Support multiple locales string for language list #Patch Set 7 : Support multiple locales string for language list #
Total comments: 12
Patch Set 8 : Support multiple locales string for language list #
Total comments: 6
Patch Set 9 : Support multiple locales string for language list #Patch Set 10 : rebase and fix presubmit #
Messages
Total messages: 40 (14 generated)
Description was changed from ========== Changre in PrependToAcceptLanguagesIfNecessary Change PrependTOAcceptLanguagesIfNecessay so that it can take a string a string locale BUG= ========== to ========== Add a function in PrependTOAcceptLanguagesIfNecessay so that it can take a string consists of multiple locales and add it to original accept languages list to form a new list. The new accept languages list will be used for chrome browser in Android as language preference. However, an update from Roll SDK to N is needed. BUG=593515 ==========
yirui@google.com changed reviewers: + nona@chromium.org
Here is the code for the function PrependToAcceptLanguagesIfNecessary, where new added languages will be combined to form a new accept language list, which is used in chrome browser in Android. Compared to earlier version, it can deal with a string that is a locale list. Since it is related to code in PwsClientImpl, that part is also updated as well.
Description was changed from ========== Add a function in PrependTOAcceptLanguagesIfNecessay so that it can take a string consists of multiple locales and add it to original accept languages list to form a new list. The new accept languages list will be used for chrome browser in Android as language preference. However, an update from Roll SDK to N is needed. BUG=593515 ========== to ========== Add a function in PrependTOAcceptLanguagesIfNecessay so that it can take a string consists of multiple locales as an input for generating a accept languages list to form a new list. The new accept languages list will be used for chrome browser in Android as language preference. However, an update from Roll SDK to N is needed. BUG=593515 ==========
https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:273: // Check if input locale is a list or a single locale I think simply String[] locales = locale.split(","); works for both cases. https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:299: // but please continue the line. https://codereview.chromium.org/2393673003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:138: PwsClientImpl.prependToAcceptLanguagesIfNecessary(locale, languageList); You actually didn't change this line. Maybe this is changed by clang-format. Please revert this? https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1149: std::vector<std::string> lans; probably, languages or langs are preferred. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1152: if (languages_to_check.length() > 5) { Looks like this is not a good condition for checking the input is locale list or not. For example, "en,ja" is 5-length string but it is a list of the locales. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1158: languages_to_check = languages_to_check.substr(bfound); Since languages_to_check is a std::string, you create new std::string object for each while loop. Instead how about using 2nd argument of the std::string::find method? http://www.cplusplus.com/reference/string/string/find/ https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1160: if (languages_to_check.length() <= 5) { Similarly, languages_to_check can be still list even if it is less than 5 chars, e.g. "en,ja"
1. In prependToAcceptLanguagesIfNecessary, rewrote part of code using the second parameter of std::string::find. 2. In prependToAcceptLanguagesIfNecessary, using DCHECK for language format, just in case input exceptions. 3. In PwsClientImplTest, reverted some unnecessary changes made by 'git cl format'. 4. In test for prependToAcceptLanguagesIfNecessary, deleted tests that would not happened in Android language input system.
https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:273: // Check if input locale is a list or a single locale On 2016/10/05 07:15:40, Seigo Nonaka wrote: > I think simply > > String[] locales = locale.split(","); > > works for both cases. Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:299: // but On 2016/10/05 07:15:40, Seigo Nonaka wrote: > please continue the line. Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:138: PwsClientImpl.prependToAcceptLanguagesIfNecessary(locale, languageList); On 2016/10/05 07:15:41, Seigo Nonaka wrote: > You actually didn't change this line. Maybe this is changed by clang-format. > Please revert this? Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1149: std::vector<std::string> lans; On 2016/10/05 07:15:41, Seigo Nonaka wrote: > probably, languages or langs are preferred. Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1152: if (languages_to_check.length() > 5) { On 2016/10/05 07:15:41, Seigo Nonaka wrote: > Looks like this is not a good condition for checking the input is locale list or > not. > For example, "en,ja" is 5-length string but it is a list of the locales. Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1158: languages_to_check = languages_to_check.substr(bfound); On 2016/10/05 07:15:41, Seigo Nonaka wrote: > Since languages_to_check is a std::string, you create new std::string object for > each while loop. > Instead how about using 2nd argument of the std::string::find method? > http://www.cplusplus.com/reference/string/string/find/ Done. https://codereview.chromium.org/2393673003/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1160: if (languages_to_check.length() <= 5) { On 2016/10/05 07:15:41, Seigo Nonaka wrote: > Similarly, languages_to_check can be still list even if it is less than 5 chars, > e.g. "en,ja" Done.
https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1148: const std::string& locale, 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. https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1151: std::size_t max_check_length = locale.length(); I think you can just use size_t here (dropping std:: prefix). https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1154: if (locale.length() > 5) { BTW, I think you can use StringSplit here. https://cs.chromium.org/chromium/src/base/strings/string_split.h?rcl=0&l=46 https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1173: DCHECK(languages[i].size() == 5u && languages[i][2] == '_'); 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; } }
https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1148: const std::string& locale, On 2016/10/06 02:02:18, Seigo Nonaka wrote: > 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. Done. https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1151: std::size_t max_check_length = locale.length(); On 2016/10/06 02:02:18, Seigo Nonaka wrote: > I think you can just use size_t here (dropping std:: prefix). Done. https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1154: if (locale.length() > 5) { On 2016/10/06 02:02:18, Seigo Nonaka wrote: > BTW, I think you can use StringSplit here. > https://cs.chromium.org/chromium/src/base/strings/string_split.h?rcl=0&l=46 Done. https://codereview.chromium.org/2393673003/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1173: DCHECK(languages[i].size() == 5u && languages[i][2] == '_'); On 2016/10/06 02:02:18, Seigo Nonaka wrote: > 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; > } > } Done.
lgtm
Description was changed from ========== Add a function in PrependTOAcceptLanguagesIfNecessay so that it can take a string consists of multiple locales as an input for generating a accept languages list to form a new list. The new accept languages list will be used for chrome browser in Android as language preference. However, an update from Roll SDK to N is needed. BUG=593515 ========== to ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-local string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ==========
Description was changed from ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-local string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ========== to ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-locale string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ==========
yirui@google.com changed reviewers: + ksk@chromium.org, mariakhomenko@chromium.org
+mariakhomenko as an owner of the Clank. Here is the code for supporting multi-locale feature from Android N. Please take a look. Thanks in advance.
ksk@google.com changed reviewers: + ksk@google.com
https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:266: * @param locale A string representing the default locale. Now this param can contain multiple locales. Please update comment and the param name. https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: for (String l : locales) { Please use descriptive name for l. https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:93: String locale = "aa_AA,bb_BB"; How about having a test for multiple locales has same language code like "aa_AA,bb_BB,bb_XX"? https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1149: // be xx_XX style, where xx is a 2-letter ISO 639-1 compliant language code and 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. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1154: std::vector<std::string> languages = base::SplitString(locales, Can this be locales? languages sounds confusing because this format is different from |language| nor |accept_languages|. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1184: if ((accept_languages->find(language) == std::string::npos || 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.
On 2016/10/06 08:15:22, ksk1 wrote: > https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java > (right): > > https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:266: > * @param locale A string representing the default locale. > Now this param can contain multiple locales. Please update comment and the param > name. > > https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: > for (String l : locales) { > Please use descriptive name for l. > > https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java > (right): > > https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:93: > String locale = "aa_AA,bb_BB"; > How about having a test for multiple locales has same language code like > "aa_AA,bb_BB,bb_XX"? > > https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... > File chrome/browser/android/preferences/pref_service_bridge.cc (right): > > https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... > chrome/browser/android/preferences/pref_service_bridge.cc:1149: // be xx_XX > style, where xx is a 2-letter ISO 639-1 compliant language code and > 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. > > https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... > chrome/browser/android/preferences/pref_service_bridge.cc:1154: > std::vector<std::string> languages = base::SplitString(locales, > Can this be locales? languages sounds confusing because this format is different > from |language| nor |accept_languages|. > > https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... > chrome/browser/android/preferences/pref_service_bridge.cc:1184: if > ((accept_languages->find(language) == std::string::npos || > 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" e.g. when the input is "aa_bb, bb_BB". > It shouldn't be good even if it doesn't happen in practice.
nits only, I will approve when you address the other reviewers' comments. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1157: for (std::size_t i = 0; i < languages.size(); i++) { you can just use size_t here, Chrome defines it (see other examples in this file) https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1161: continue; use {} for blocks that are more than one line (e.g. due to comments)
https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:266: * @param locale A string representing the default locale. On 2016/10/06 08:15:22, ksk1 wrote: > Now this param can contain multiple locales. Please update comment and the param > name. Done. https://codereview.chromium.org/2393673003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: for (String l : locales) { On 2016/10/06 08:15:22, ksk1 wrote: > Please use descriptive name for l. Done. https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:93: String locale = "aa_AA,bb_BB"; On 2016/10/06 08:15:22, ksk1 wrote: > How about having a test for multiple locales has same language code like > "aa_AA,bb_BB,bb_XX"? Thanks for pointing out this case. A language code should only be inserted after the last languageTag that contains that language. Here is the example it should be: locales = "aa_AA,aa_BB",acceptLanguages = "xx-XX,xx". output = "aa-AA,aa-BB,aa,xx-XX,xx". I tried to fix it by checking the rest of locales that have not been inserted yet. (from off line discussion with nona@) However, I also noticed that in some previous test cases, original languageList appeared to be "xx-XX,xx,xx-YY", which should not happen under this implementation, therefore I applied changes on them as well. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1149: // be xx_XX style, where xx is a 2-letter ISO 639-1 compliant language code and On 2016/10/06 08:15:22, ksk1 wrote: > 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. currently, we will keep '_' form for the locales, this could be TODO in the future for combining '-' and '_' into the same form. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1154: std::vector<std::string> languages = base::SplitString(locales, On 2016/10/06 08:15:22, ksk1 wrote: > Can this be locales? languages sounds confusing because this format is different > from |language| nor |accept_languages|. Done. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1157: for (std::size_t i = 0; i < languages.size(); i++) { On 2016/10/06 16:38:00, Maria wrote: > you can just use size_t here, Chrome defines it (see other examples in this > file) Done. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1161: continue; On 2016/10/06 16:38:00, Maria wrote: > use {} for blocks that are more than one line (e.g. due to comments) Done. https://codereview.chromium.org/2393673003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1184: if ((accept_languages->find(language) == std::string::npos || On 2016/10/06 08:15:22, ksk1 wrote: > 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. set is used for this checking process.
https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:308: String language_add = unique_list.get(i).substring(0, 2); How about: String locale = unique_list.get(i); String language = locale.substring(0, 2); if (!seen_languages.contains(language)) { ... } output_list.add(locale); https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1150: // Input |locales| is a comma separated language tags. Each locale comma separated locales? https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1166: if (locale_str.size() != 5 || != 5u https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1171: std::string region_code = locale_str.substr(3, 2); county_code We call it country code in the method comment, we should be consistent.
lgtm with style nits. https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:281: // TODO(yirui): Support BCP47 compliant format including 3-letter nit: you can continue line until 100 chars. https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:299: // If language is not in the accept languages list, also add language nit: you can continue line until 100 chars.
https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:281: // TODO(yirui): Support BCP47 compliant format including 3-letter On 2016/10/07 10:06:40, Seigo Nonaka wrote: > nit: you can continue line until 100 chars. Done. https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:299: // If language is not in the accept languages list, also add language On 2016/10/07 10:06:40, Seigo Nonaka wrote: > nit: you can continue line until 100 chars. Done. https://codereview.chromium.org/2393673003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:308: String language_add = unique_list.get(i).substring(0, 2); On 2016/10/07 09:16:23, ksk1 wrote: > How about: > String locale = unique_list.get(i); > String language = locale.substring(0, 2); > if (!seen_languages.contains(language)) { > ... > } > output_list.add(locale); Done. https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1150: // Input |locales| is a comma separated language tags. Each locale On 2016/10/07 09:16:23, ksk1 wrote: > comma separated locales? Done. https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1166: if (locale_str.size() != 5 || On 2016/10/07 09:16:23, ksk1 wrote: > != 5u Done. https://codereview.chromium.org/2393673003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1171: std::string region_code = locale_str.substr(3, 2); On 2016/10/07 09:16:23, ksk1 wrote: > county_code > We call it country code in the method comment, we should be consistent. Done.
lgtm
lgtm
https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: String locale_string = locales + "," + acceptLanguages; style: use camelCase for variable names in Java here and below https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:278: HashSet<String> seen_locales = new HashSet<String>(); nit: can use HashSet<>() and ArrayList<>() etc here and below https://codereview.chromium.org/2393673003/diff/140001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1170: std::string lang_code = locale_str.substr(0, 2); initialize instead of assigning, e.g. std::string lang_code(locale_str.substr(0, 2)), here and below as assigning does an extra copy
https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: String locale_string = locales + "," + acceptLanguages; On 2016/10/07 19:14:29, Maria wrote: > style: use camelCase for variable names in Java here and below Done. https://codereview.chromium.org/2393673003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:278: HashSet<String> seen_locales = new HashSet<String>(); On 2016/10/07 19:14:29, Maria wrote: > nit: can use HashSet<>() and ArrayList<>() etc here and below Done. https://codereview.chromium.org/2393673003/diff/140001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2393673003/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1170: std::string lang_code = locale_str.substr(0, 2); On 2016/10/07 19:14:29, Maria wrote: > initialize instead of assigning, e.g. std::string lang_code(locale_str.substr(0, > 2)), here and below as assigning does an extra copy Done.
lgtm
The CQ bit was checked by yirui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nona@chromium.org, ksk@google.com Link to the patchset: https://codereview.chromium.org/2393673003/#ps160001 (title: "Support multiple locales string for language list")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yirui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ksk@google.com, mariakhomenko@chromium.org, nona@chromium.org Link to the patchset: https://codereview.chromium.org/2393673003/#ps180001 (title: "rebase and fix presubmit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-locale string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ========== to ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-locale string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-locale string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 ========== to ========== Support multiple locales string for accept language list This patch enables PrependToAcceptLanuagesIfNecessary to take multi-locale string. This is a preparation for supporting multi-locale feature from Android N. This patch does not break any existing behaviors. BUG=593515 Committed: https://crrev.com/6cc907c7b494642fe93ee850439ad816998e21e3 Cr-Commit-Position: refs/heads/master@{#424372} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6cc907c7b494642fe93ee850439ad816998e21e3 Cr-Commit-Position: refs/heads/master@{#424372} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
