Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406203002:
Use BCP47 compliant format for locale representation (Closed)
|
Created:
4 years, 2 months ago by Yirui Huang Modified:
4 years, 1 month ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse BCP47 compliant format for locale representation.
Locale.toString() is not a interchangeable representation.
Locale.toLanguageTag should be used for obtaining a value in IETF
BCP47 language tag representation. However, toLanguageTag does not
work on Android M and before. Thereofore, we use a
self-implementation for converting Locale object to BCP47 compliant
format string in that situation.
Similarly, Locale.forLanguageTag should be used for constructing
Locale object from BCP47 String representation of locales. However,
this is not available on Android L and before. Thus, we introduce
self-implementation for backward compatibility.
This CL also includes:
- Renaming getDefaultLocale to getDefaultLocaleString. This leads to
function name changes in related files.
- Adding methods for switching language code between Chrome and
Android.
- Adding methods for returning a locale with updated language code
for Chrome or Android.
BUG=593515
Committed: https://crrev.com/9174164b690d246b72273cb4df448224ed8c4f7e
Cr-Commit-Position: refs/heads/master@{#428281}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebased to master #
Total comments: 8
Patch Set 3 : create to/forLanguageTag in LocaleUtils #
Total comments: 16
Patch Set 4 : apply ICU instead of self-implementing #
Total comments: 4
Patch Set 5 : rebase master, unit tests added for LocaleUtils #
Total comments: 33
Patch Set 6 : format return in description #
Total comments: 12
Patch Set 7 : HashMap used and Error checking added #
Total comments: 21
Patch Set 8 : Rename getDefaultLocaleString, Locale.Builder used #
Total comments: 4
Patch Set 9 : add updateLocaleForChromium in LocaleUtils #
Total comments: 12
Patch Set 10 : rebase master, new branch cherry-pick #Patch Set 11 : Self-implement forLanguageTag only. Fixed a bug in AwContents.java #
Total comments: 4
Patch Set 12 : add more explanations about why Locale.getLanguage/toLanguageTag is not used #
Total comments: 6
Patch Set 13 : cleanup LocaleUtils, update codes that are not reverted from "rebase master" #
Total comments: 12
Patch Set 14 : Reverted toLanguageTag/forLanguageTag, update test with correct form #
Total comments: 14
Patch Set 15 : TargetApi used, renamed two update functions in LocaleUtils #
Total comments: 2
Patch Set 16 : Add tests for checking if updated lang and deprecared lang are pointing to the same object #
Total comments: 10
Patch Set 17 : Add mapping from Chromium to Android for forLanguageTag #
Total comments: 2
Patch Set 18 : Rebased, using .equals(), create compat for forLanguageTag, more tests added #
Total comments: 4
Patch Set 19 : BCP 47, change in description #Messages
Total messages: 195 (136 generated)
Description was changed from ========== Support BCP47 form in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It makes all return values as a language tag in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in LocaleUtils. Support multiple locales string for 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 BCP47 form in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It makes all return values as a language tag in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in LocaleUtils. BUG=593515 ==========
yirui@google.com changed reviewers: + ksk@google.com, nona@chromium.org
On 2016/10/11 09:31:53, yirui wrote: Since LocaleUtils.getDefaultLocale() will always return a well formed format, I deleted some tests that involve format checking.
according to the BCP47 spec (https://tools.ietf.org/html/bcp47), Language-Tag can contain several attributes that is not supported yet such as script. So I feel the subject "Support BCP47 form in accept language list" sounds misunderstanding. https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:229: String getAcceptLanguage() { Can we rename this to getAcceptLanguages or something? Also, please update the comment and variable names properly. https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: static String prependToAcceptLanguagesIfNecessary(String locales, String acceptLanguages) { I suspect that this CL contains your previous CL. Maybe, need to rebase? https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:63: assertTrue(acceptLanguage.contains(tag)); If tag is "xx" and acceptLanguage is "xx-XXX", this test passes, but it should fail. How about preserving the existing assert like: assertTrue(acceptLanguage.startsWith(tag + ",") || acceptLanguage.contains(tag + ";") || acceptLanguage.equals(tag))
Description was changed from ========== Support BCP47 form in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It makes all return values as a language tag in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in LocaleUtils. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It makes all return values as a language tag in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in LocaleUtils. BUG=593515 ==========
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It makes all return values as a language tag in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in LocaleUtils. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in these two forms in LocaleUtils. BUG=593515 ==========
https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:282: String language; You can use Locale.forLanguageTag here like for (String locale_str : localeList) { Locale locale = Locale.forLanguageTag(locale_str); String language = locale.getLanguage(); String country = locale.getCountry(); ... } https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:302: if (country != "") { Please use !country.isEmpty() here. https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1192: if (country_code != "") Please use !country_code.empty() here. https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1211: if (it->second != "") ditto.
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in these two forms in LocaleUtils. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in these two forms in LocaleUtils. BUG=593515 ==========
https://codereview.chromium.org/2406203002/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1163: std::string lang_code; I'm not sure we can use ICU functions here, but if it is possible, you may want to use ICU function like http://icu-project.org/apiref/icu4c/uloc_8h.html#aa45d6457f72867880f079e27a63... If you are not familiar with ICU, please ignore this comment and please leave TODO comment like TODO(nona): use uloc_forLanguageTag instead of self implementation.
android.annotation.SuppressLint is needed for the usage of Locale.forLanguageTag. https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:229: String getAcceptLanguage() { On 2016/10/11 09:58:08, ksk1 wrote: > Can we rename this to getAcceptLanguages or something? > Also, please update the comment and variable names properly. Done. https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275: static String prependToAcceptLanguagesIfNecessary(String locales, String acceptLanguages) { On 2016/10/11 09:58:08, ksk1 wrote: > I suspect that this CL contains your previous CL. Maybe, need to rebase? Done. https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (right): https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:63: assertTrue(acceptLanguage.contains(tag)); On 2016/10/11 09:58:08, ksk1 wrote: > If tag is "xx" and acceptLanguage is "xx-XXX", this test passes, but it should > fail. > How about preserving the existing assert like: > assertTrue(acceptLanguage.startsWith(tag + ",") > || acceptLanguage.contains(tag + ";") > || acceptLanguage.equals(tag)) Done. https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:282: String language; On 2016/10/11 11:29:28, Seigo Nonaka wrote: > You can use Locale.forLanguageTag here like > > for (String locale_str : localeList) { > Locale locale = Locale.forLanguageTag(locale_str); > String language = locale.getLanguage(); > String country = locale.getCountry(); > ... > } Done. https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:302: if (country != "") { On 2016/10/11 11:29:28, Seigo Nonaka wrote: > Please use !country.isEmpty() here. Done. https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1192: if (country_code != "") On 2016/10/11 11:29:28, Seigo Nonaka wrote: > Please use !country_code.empty() here. Done. https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1211: if (it->second != "") On 2016/10/11 11:29:28, Seigo Nonaka wrote: > ditto. Done.
On 2016/10/11 12:17:33, yirui wrote:
> android.annotation.SuppressLint is needed for the usage of
> Locale.forLanguageTag.
Ugh, yes Locale.forLanguageTag is available after API Level 21 but Chrome need
still supports JellyBeans(API Level 16...)
Then, how about introduce LocaleUtils.forLangauageTag/LocaleUtils.toLanguageTag?
I think BCP47 <-> Locale conversion can be common utility functions.
Note that you can use Locale.{to|for}LanguageTag with checking API level.
(https://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT)
>
>
https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
> (right):
>
>
https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:229:
> String getAcceptLanguage() {
> On 2016/10/11 09:58:08, ksk1 wrote:
> > Can we rename this to getAcceptLanguages or something?
> > Also, please update the comment and variable names properly.
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:275:
> static String prependToAcceptLanguagesIfNecessary(String locales, String
> acceptLanguages) {
> On 2016/10/11 09:58:08, ksk1 wrote:
> > I suspect that this CL contains your previous CL. Maybe, need to rebase?
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java
> (right):
>
>
https://codereview.chromium.org/2406203002/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:63:
> assertTrue(acceptLanguage.contains(tag));
> On 2016/10/11 09:58:08, ksk1 wrote:
> > If tag is "xx" and acceptLanguage is "xx-XXX", this test passes, but it
should
> > fail.
> > How about preserving the existing assert like:
> > assertTrue(acceptLanguage.startsWith(tag + ",")
> > || acceptLanguage.contains(tag + ";")
> > || acceptLanguage.equals(tag))
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
> (right):
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:282:
> String language;
> On 2016/10/11 11:29:28, Seigo Nonaka wrote:
> > You can use Locale.forLanguageTag here like
> >
> > for (String locale_str : localeList) {
> > Locale locale = Locale.forLanguageTag(locale_str);
> > String language = locale.getLanguage();
> > String country = locale.getCountry();
> > ...
> > }
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:302:
> if (country != "") {
> On 2016/10/11 11:29:28, Seigo Nonaka wrote:
> > Please use !country.isEmpty() here.
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/preferences/pref_service_bridge.cc (right):
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/...
> chrome/browser/android/preferences/pref_service_bridge.cc:1192: if
(country_code
> != "")
> On 2016/10/11 11:29:28, Seigo Nonaka wrote:
> > Please use !country_code.empty() here.
>
> Done.
>
>
https://codereview.chromium.org/2406203002/diff/20001/chrome/browser/android/...
> chrome/browser/android/preferences/pref_service_bridge.cc:1211: if (it->second
> != "")
> On 2016/10/11 11:29:28, Seigo Nonaka wrote:
> > ditto.
>
> Done.
In patch 5, I made own version of for/toLanguageTag in LocaleUtils and combined the update of language code in LocaleUtils. In patch 6, I tried use ICU functions while reading documents. Please let me know if there is anything I could do to improve here. https://codereview.chromium.org/2406203002/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1163: std::string lang_code; On 2016/10/11 11:31:36, Seigo Nonaka wrote: > I'm not sure we can use ICU functions here, but if it is possible, you may want > to use ICU function like > http://icu-project.org/apiref/icu4c/uloc_8h.html#aa45d6457f72867880f079e27a63... > > If you are not familiar with ICU, please ignore this comment and please leave > TODO comment like > > TODO(nona): use uloc_forLanguageTag instead of self implementation. Done.
https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:39: public static String languageAdjust(String language) { Can this be private? https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:59: */ Can we have unit tests for forLanguageTag and toLanguageTag? https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:62: Locale locale = Locale.forLanguageTag(languageTag); return Locale.forLanguageTag(languageTag); https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:64: } else { How about removing else? https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:70: } else if (languageTag.charAt(2) == '-') { This throws IndexOutOfBoundsException when languageTag length is smaller than 3. Please have length check before calling charAt and substring. How about: String[] tags = languageTag.split("-"); if (tags.length == 0) { return new Locale(""); } String language = tags[0]; if (language.length() != 2 && language.length() != 3) { return new Locale(""); } if (tag.length == 1) { return new Locale(language); } String country = tag[1]; if (country.length() != 2 && country.length() != 3) { return new Locale(language); } return new Locale(language, country); https://codereview.chromium.org/2406203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:256: HashSet<String> seenLocales = new HashSet<>(); Should this be seenLanguageTags? https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1169: } else if (locale_str[2] == '-') { Please have length check before accessing elements. https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1186: lang_code = "id"; Is it OK not to have tl -> fil conversion?
https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:69: remove https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1170: nullptr, &error); wrong indent
https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:39: public static String languageAdjust(String language) { On 2016/10/12 05:52:50, ksk1 wrote: > Can this be private? Done. https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:59: */ On 2016/10/12 05:52:50, ksk1 wrote: > Can we have unit tests for forLanguageTag and toLanguageTag? Done. https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:62: Locale locale = Locale.forLanguageTag(languageTag); On 2016/10/12 05:52:50, ksk1 wrote: > return Locale.forLanguageTag(languageTag); Done. https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:64: } else { On 2016/10/12 05:52:50, ksk1 wrote: > How about removing else? Done. https://codereview.chromium.org/2406203002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/LocaleUtils.java:70: } else if (languageTag.charAt(2) == '-') { On 2016/10/12 05:52:50, ksk1 wrote: > This throws IndexOutOfBoundsException when languageTag length is smaller than 3. > Please have length check before calling charAt and substring. > How about: > String[] tags = languageTag.split("-"); > if (tags.length == 0) { > return new Locale(""); > } > String language = tags[0]; > if (language.length() != 2 && language.length() != 3) { > return new Locale(""); > } > if (tag.length == 1) { > return new Locale(language); > } > String country = tag[1]; > if (country.length() != 2 && country.length() != 3) { > return new Locale(language); > } > return new Locale(language, country); For original input languageTag, I assumed that all language tags are in well formed, since LocalesUtils.getDefaultString() is called to form all tags. But I agreed that this way, it is more safer to deal to exceptions. Thanks! https://codereview.chromium.org/2406203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:256: HashSet<String> seenLocales = new HashSet<>(); On 2016/10/12 05:52:50, ksk1 wrote: > Should this be seenLanguageTags? Done. https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1169: } else if (locale_str[2] == '-') { On 2016/10/12 05:52:50, ksk1 wrote: > Please have length check before accessing elements. Please see patch 6 for another implementing method. https://codereview.chromium.org/2406203002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:1186: lang_code = "id"; On 2016/10/12 05:52:50, ksk1 wrote: > Is it OK not to have tl -> fil conversion? Done in patch 6 :) https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:69: On 2016/10/12 06:10:24, ksk1 wrote: > remove Done. https://codereview.chromium.org/2406203002/diff/100001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1170: nullptr, &error); On 2016/10/12 06:10:24, ksk1 wrote: > wrong indent Done.
https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:39: private static String languageAdjust(String language) { "adjust" sounds like bit ambiguous. How about migrateDeprecatedLanguage? https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:44: if ("iw".equals(language)) { optional: you can prepare static map for this purpose, like private static Map<String, String> DEPRECATED_LANGUAGE_MAP; static { HashMap<String, String> map = new HashMap<>(); map.put("iw", "he"); // Hebrew map.put("ji", "yi"); // Yiddish ... DEPRECATED_LANGUAGE_MAP = Collections.unmodifiableMap(map); } Then, String newLanguageID = DEPRECATED_LANGUAGE_MAP.get(language); return newLanguageID == null ? language : newLanguageID; https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:58: * This function does the same as Locale.forLanguageTag() when API level is lower than 21. forLanguageTag does more complicated things than simply parsing xx-XX style. How about, This function can be used for alternative of Locale.forLanguageTag before API Level 21. This function creates Locale object from xx-XX style string where xx is language code and XX is a country code. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:62: public static Locale forLanguageTagCompat(String languageTag) { This can be private? if this is public only for testing purpose, please put @visibleForTesting https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:88: return Locale.forLanguageTag(languageTag); Look like you don't migrate the deprecated language code. Is the Android after Lollipop no longer has a problem of deprecated language code? If yes, please leave a comment. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:99: return locale.toLanguageTag(); Same as above. Please check the deprecated language code. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:100: } else { nit: you can drop else here. https://codereview.chromium.org/2406203002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:270: uniqueList.add(languageTag); You are creating same locale object twice by calling LocaleUtils.forLanguageTag. How about keeping Locale object instead of String in uniqueList? like HashSet<Locale> seenLocales = new HashSet<>(); ArrayList<Locale> uniqueList = new ArrayList<>(); for (String localeString : localeList) { Locale locale = LocaleUtils.forLanguageTag(localeString); if (seenLocales.contains(locale)) { continue; } if (!locale.getCountry().isEmpty()) { seenLocales.add(locale); } } ... HashSet<String> seenLanguages = new HashSet<>(); ArrayList<String> outputList = new ArrayList<>(); for (int i = uniqueList.size() - 1; i >= 0; i--) { Locale locale = uniqueList.get(i); String language = locale.getLanguage(); String country = locale.getCountry(); if (!seenLanguages.contains(language)) { seenLanguages.add(language); outputList.add(language); } outputList.add(LocaleUtils.toLanguageTag(locale)); } https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1152: // language tags(BCP47 compliant format). Each language tag contains nit: continues line? https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1164: char locale_ID[ULOC_FULLNAME_CAPACITY]; I'd like to initialize these three variables with zero like char locale_ID[ULOC_FULLNAME_CAPACITY] = {}; And also good to move outside of for-loop. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1169: nullptr, &error); You need to check the result of this function. like UErrorCode error = U_ZERO_ERROR; uloc_forLanguageTag(locale_str.c_str(), locale_ID, ULOC_FULLNAME_CAPACITY, nullptr, &error); if (U_FAILURE(error)) { LOG(ERROR) << "Ignoring invalid locale representation " << locale_str; 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_str; continue; } error = U_ZERO_ERROR; uloc_getCountry(locale_ID, country_code, ULOC_COUNTRY_CAPACITY, &error); if (U_FAILURE(error)) { // You can choose dropping entire locale string or keeping language code with dropping country code here. } https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1173: std::string coun_code(country_code); coun_code sounds weird to me. How about country_code with replacing existing country_code with country_code_buffer? https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1175: // Java mostly follows ISO-639-1 and ICU, except for the following four. You already did this conversion in Java. I think you can remove these lines. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: if (coun_code != "") Please use coun_code.empty()
https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:39: private static String languageAdjust(String language) { On 2016/10/17 04:12:15, Seigo Nonaka wrote: > "adjust" sounds like bit ambiguous. > How about migrateDeprecatedLanguage? I think "tl" is not deprecated. https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:261: String country = locale.getCountry(); if (language.isEmpty()) { continue; } https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:267: if (!country.isEmpty()) { Why is this if needed? uniqueList can be inconsistent with seenLanguageTags. This would never be happen in practice, but this is confusing for future development. https://codereview.chromium.org/2406203002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (left): https://codereview.chromium.org/2406203002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:128: public void testInvalidLanguageTagNotPrepended() { I think we shouldn't remove tests for invalid cases. https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1172: std::string lang_code(language_code); Using both lang_code and language_code is confusing. How about s/language_code/language_code_buffer/ and s/lang_code/language_code/ Same comment for country code. https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1174: if (error != U_ZERO_ERROR || lang_code.empty()) { continue; } https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: if (coun_code != "") !coun_code.empty()
https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:39: private static String languageAdjust(String language) { On 2016/10/17 04:12:15, Seigo Nonaka wrote: > "adjust" sounds like bit ambiguous. > How about migrateDeprecatedLanguage? Done. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:39: private static String languageAdjust(String language) { On 2016/10/17 05:06:04, ksk1 wrote: > On 2016/10/17 04:12:15, Seigo Nonaka wrote: > > "adjust" sounds like bit ambiguous. > > How about migrateDeprecatedLanguage? > > I think "tl" is not deprecated. Done. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:44: if ("iw".equals(language)) { On 2016/10/17 04:12:15, Seigo Nonaka wrote: > optional: > you can prepare static map for this purpose, like > > private static Map<String, String> DEPRECATED_LANGUAGE_MAP; > > static { > HashMap<String, String> map = new HashMap<>(); > map.put("iw", "he"); // Hebrew > map.put("ji", "yi"); // Yiddish > ... > DEPRECATED_LANGUAGE_MAP = Collections.unmodifiableMap(map); > } > > Then, > > String newLanguageID = DEPRECATED_LANGUAGE_MAP.get(language); > return newLanguageID == null ? language : newLanguageID; changed using HashMap, but since this function is called by ResourceExtractor.java(new commit), I made it public. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:58: * This function does the same as Locale.forLanguageTag() when API level is lower than 21. On 2016/10/17 04:12:15, Seigo Nonaka wrote: > forLanguageTag does more complicated things than simply parsing xx-XX style. > How about, > > This function can be used for alternative of Locale.forLanguageTag before API > Level 21. > This function creates Locale object from xx-XX style string where xx is language > code and XX is a country code. Done. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:62: public static Locale forLanguageTagCompat(String languageTag) { On 2016/10/17 04:12:15, Seigo Nonaka wrote: > This can be private? if this is public only for testing purpose, please put > @visibleForTesting Done. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:88: return Locale.forLanguageTag(languageTag); On 2016/10/17 04:12:15, Seigo Nonaka wrote: > Look like you don't migrate the deprecated language code. Is the Android after > Lollipop no longer has a problem of deprecated language code? If yes, please > leave a comment. It seems these three deprecated language codes are withdrawn in ISO 639-2. (http://www.loc.gov/standards/iso639-2/php/code_changes.php) but it might be safer to check using updateLanguageForChromium. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:99: return locale.toLanguageTag(); On 2016/10/17 04:12:15, Seigo Nonaka wrote: > Same as above. Please check the deprecated language code. Done. https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:100: } else { On 2016/10/17 04:12:15, Seigo Nonaka wrote: > nit: you can drop else here. Done. https://codereview.chromium.org/2406203002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:270: uniqueList.add(languageTag); On 2016/10/17 04:12:15, Seigo Nonaka wrote: > You are creating same locale object twice by calling LocaleUtils.forLanguageTag. > How about keeping Locale object instead of String in uniqueList? > like > > HashSet<Locale> seenLocales = new HashSet<>(); > ArrayList<Locale> uniqueList = new ArrayList<>(); > for (String localeString : localeList) { > Locale locale = LocaleUtils.forLanguageTag(localeString); > if (seenLocales.contains(locale)) { > continue; > } > if (!locale.getCountry().isEmpty()) { > seenLocales.add(locale); > } > } > > ... > > HashSet<String> seenLanguages = new HashSet<>(); > ArrayList<String> outputList = new ArrayList<>(); > > for (int i = uniqueList.size() - 1; i >= 0; i--) { > Locale locale = uniqueList.get(i); > String language = locale.getLanguage(); > String country = locale.getCountry(); > if (!seenLanguages.contains(language)) { > seenLanguages.add(language); > outputList.add(language); > } > outputList.add(LocaleUtils.toLanguageTag(locale)); > } Done. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1152: // language tags(BCP47 compliant format). Each language tag contains On 2016/10/17 04:12:15, Seigo Nonaka wrote: > nit: continues line? Done. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1164: char locale_ID[ULOC_FULLNAME_CAPACITY]; On 2016/10/17 04:12:15, Seigo Nonaka wrote: > I'd like to initialize these three variables with zero like > > char locale_ID[ULOC_FULLNAME_CAPACITY] = {}; > > And also good to move outside of for-loop. Done. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1169: nullptr, &error); On 2016/10/17 04:12:16, Seigo Nonaka wrote: > You need to check the result of this function. > > like > > UErrorCode error = U_ZERO_ERROR; > uloc_forLanguageTag(locale_str.c_str(), locale_ID, ULOC_FULLNAME_CAPACITY, > nullptr, &error); > if (U_FAILURE(error)) { > LOG(ERROR) << "Ignoring invalid locale representation " << locale_str; > 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_str; > continue; > } > > error = U_ZERO_ERROR; > uloc_getCountry(locale_ID, country_code, ULOC_COUNTRY_CAPACITY, &error); > if (U_FAILURE(error)) { > // You can choose dropping entire locale string or keeping language code with > dropping country code here. > } As this been discussed off line, entire locale string will be dropped off if error occurs. Since if no county code attached, it will return empty. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1173: std::string coun_code(country_code); On 2016/10/17 04:12:16, Seigo Nonaka wrote: > coun_code sounds weird to me. How about country_code with replacing existing > country_code with country_code_buffer? Done. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1175: // Java mostly follows ISO-639-1 and ICU, except for the following four. On 2016/10/17 04:12:16, Seigo Nonaka wrote: > You already did this conversion in Java. I think you can remove these lines. I deleted some tests that should not appear in this situation, such as taking the argument as "iw-IL", which should alrealy be returned as "he-IL" form in Java. https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: if (coun_code != "") On 2016/10/17 04:12:16, Seigo Nonaka wrote: > Please use coun_code.empty() Done. https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:261: String country = locale.getCountry(); On 2016/10/17 05:06:04, ksk1 wrote: > if (language.isEmpty()) { > continue; > } Done. https://codereview.chromium.org/2406203002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:267: if (!country.isEmpty()) { On 2016/10/17 05:06:04, ksk1 wrote: > Why is this if needed? > uniqueList can be inconsistent with seenLanguageTags. > This would never be happen in practice, but this is confusing for future > development. I changed a little bit here, only using uniqueList. https://codereview.chromium.org/2406203002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java (left): https://codereview.chromium.org/2406203002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:128: public void testInvalidLanguageTagNotPrepended() { On 2016/10/17 05:06:04, ksk1 wrote: > I think we shouldn't remove tests for invalid cases. Done. https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1172: std::string lang_code(language_code); On 2016/10/17 05:06:04, ksk1 wrote: > Using both lang_code and language_code is confusing. > How about s/language_code/language_code_buffer/ and s/lang_code/language_code/ > > Same comment for country code. Done. https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1174: On 2016/10/17 05:06:04, ksk1 wrote: > if (error != U_ZERO_ERROR || lang_code.empty()) { > continue; > } U_FAILURE is used for checking. https://codereview.chromium.org/2406203002/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: if (coun_code != "") On 2016/10/17 05:06:04, ksk1 wrote: > !coun_code.empty() same as in Java side, not necessary, so removed check.
https://codereview.chromium.org/2406203002/diff/200001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2406203002/diff/200001/base/BUILD.gn#newcode2354 base/BUILD.gn:2354: "android/javatests/src/org/chromium/base/LocaleUtilsTest.java", Move to l. 2352 to be alphabetical order? https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:76: String language = tag[0]; String language = updateLanguageForChromium(tag[0]); And remove updateLanguageForChromium at l.81. https://codereview.chromium.org/2406203002/diff/200001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:45: assertEquals("fil", localeString); Please have tests for language that will be updated + country. https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: std::string language_code_buffer(language_code); Please use "language_code_buffer" for char array and language_code for string.
https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:44: if ("iw".equals(language)) { On 2016/10/17 10:40:51, yirui wrote: > On 2016/10/17 04:12:15, Seigo Nonaka wrote: > > optional: > > you can prepare static map for this purpose, like > > > > private static Map<String, String> DEPRECATED_LANGUAGE_MAP; > > > > static { > > HashMap<String, String> map = new HashMap<>(); > > map.put("iw", "he"); // Hebrew > > map.put("ji", "yi"); // Yiddish > > ... > > DEPRECATED_LANGUAGE_MAP = Collections.unmodifiableMap(map); > > } > > > > Then, > > > > String newLanguageID = DEPRECATED_LANGUAGE_MAP.get(language); > > return newLanguageID == null ? language : newLanguageID; > > changed using HashMap, but since this function is called by > ResourceExtractor.java(new commit), I made it public. I don't think we need to expose this function. Please see the comment in ResourceExtractor https://codereview.chromium.org/2406203002/diff/140001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:88: return Locale.forLanguageTag(languageTag); On 2016/10/17 10:40:51, yirui wrote: > On 2016/10/17 04:12:15, Seigo Nonaka wrote: > > Look like you don't migrate the deprecated language code. Is the Android after > > Lollipop no longer has a problem of deprecated language code? If yes, please > > leave a comment. > > It seems these three deprecated language codes are withdrawn in ISO 639-2. > (http://www.loc.gov/standards/iso639-2/php/code_changes.php) but it might be > safer to check using updateLanguageForChromium. Okay thanks. Then, looks like Android can still return ISO 639-1 country code. We should keep this code even for the latest device. https://developer.android.com/reference/java/util/Locale.html https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/140001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1175: // Java mostly follows ISO-639-1 and ICU, except for the following four. On 2016/10/17 10:40:52, yirui wrote: > On 2016/10/17 04:12:16, Seigo Nonaka wrote: > > You already did this conversion in Java. I think you can remove these lines. > > I deleted some tests that should not appear in this situation, such as taking > the argument as "iw-IL", which should alrealy be returned as "he-IL" form in > Java. Okay, make sense to me. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:71: static Locale forLanguageTagCompat(String languageTag) { Please put public if this is accessed from test code. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:92: * It creates Locale object from xx-XX style string where xx is language code and XX is "This function creates Locale object" or simply "Creates Locale object ..." https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:116: String[] tag = languageTag.split("-"); Please do not split with hyphen here since there can be a script between language and country. How about this? String languageForChrome = LANGUAGE_MAP.get(locale.getLanguage()); if (languageForChrome != null) { locale = new Locale.Builder().setLocale(locale).setLanguage(languageForChrome).build(); } return locale.toLangaugeTag(); https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/ResourceExtractor.java:177: String updatedLanguage = LocaleUtils.updateLanguageForChromium(originalLanguage); I don't think we should expose updateLanguageForChromium. How about changing LocaleUtils.getDefaultLocale() to return Locale object instead of String. String language = LocaleUtils.getDefaultLocale().getLanguage();
https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:42: * @return the string for the given locale with language and/or country code, translating "or" sounds strange https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1193: No need to update language code? If we don't need to update language code, the behavior is now different from PwsClientImpl.java. Please update method comment.
In LocaleUtils.java, considering script, similar to toLanguageTag, I was also trying to change forLanguageTag(Compat) and add some test cases in order to be prepared for languageTag with script. However, I am not sure about the approach to construct a Locale with script before API 21, since they dont have Locale.Builder(). Also, will the assumption that input is in one of the forms true is script is considered? 1. empty string: ""; 2. language code only: "en"; 3. language and country code: "en-US"; 4. language, script and country code: "sr-Latn-SR"; https://codereview.chromium.org/2406203002/diff/200001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2406203002/diff/200001/base/BUILD.gn#newcode2354 base/BUILD.gn:2354: "android/javatests/src/org/chromium/base/LocaleUtilsTest.java", On 2016/10/17 11:21:15, ksk1 wrote: > Move to l. 2352 to be alphabetical order? Yes. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:42: * @return the string for the given locale with language and/or country code, translating On 2016/10/17 11:34:57, ksk1 wrote: > "or" sounds strange Done. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:71: static Locale forLanguageTagCompat(String languageTag) { On 2016/10/17 11:30:46, Seigo Nonaka wrote: > Please put public if this is accessed from test code. Done. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:76: String language = tag[0]; On 2016/10/17 11:21:16, ksk1 wrote: > String language = updateLanguageForChromium(tag[0]); > And remove updateLanguageForChromium at l.81. Done. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:92: * It creates Locale object from xx-XX style string where xx is language code and XX is On 2016/10/17 11:30:46, Seigo Nonaka wrote: > "This function creates Locale object" or simply "Creates Locale object ..." Done. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:116: String[] tag = languageTag.split("-"); On 2016/10/17 11:30:46, Seigo Nonaka wrote: > Please do not split with hyphen here since there can be a script between > language and country. > > How about this? > > String languageForChrome = LANGUAGE_MAP.get(locale.getLanguage()); > if (languageForChrome != null) { > locale = new > Locale.Builder().setLocale(locale).setLanguage(languageForChrome).build(); > } > return locale.toLangaugeTag(); Done. https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/ResourceExtractor.java:177: String updatedLanguage = LocaleUtils.updateLanguageForChromium(originalLanguage); On 2016/10/17 11:30:46, Seigo Nonaka wrote: > I don't think we should expose updateLanguageForChromium. > How about changing LocaleUtils.getDefaultLocale() to return Locale object > instead of String. > > String language = LocaleUtils.getDefaultLocale().getLanguage(); since the original getLanguage does the same thing as updateLanguageForChromium, and updateLanguageForChromium should not be exposed. I made LocaleUtils.getLanguage, which can be used in this situation. https://codereview.chromium.org/2406203002/diff/200001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/200001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:45: assertEquals("fil", localeString); On 2016/10/17 11:21:16, ksk1 wrote: > Please have tests for language that will be updated + country. Done. I also added some test for invalid inputs in testForLanguageTagCompat() https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1190: std::string language_code_buffer(language_code); On 2016/10/17 11:21:16, ksk1 wrote: > Please use "language_code_buffer" for char array and language_code for string. Done. https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1193: On 2016/10/17 11:34:57, ksk1 wrote: > No need to update language code? > If we don't need to update language code, the behavior is now different from > PwsClientImpl.java. Please update method comment. I think they have the same behavior? for pref_service_bridge.cc, LocaleUtils.getDefaultLocale() is called in ChromeActivitySessionTracker.java; for PwsClientImpl.java, LocaleUtils.getDefaultLocale() is also called in updateAcceptLanguages().
In Patch 13, a change has been made to gerDefaultLocale, the return value changed from a locale string to a Locale. On the other hand, getDefaultLocaleString is added accordingly and will be called by native. Also, Locale.Builder() will be used in toLanguageTag and forLanguageTag in LocaleUtils. Finally, all files involve these functions are being modified as well.
Almost there. Please address minor nits and please also update the description. This CL is no longer only for accept languages. At least you may want to mention about - To get well-formed BCP47 representation, use toLanguageTag/forLanguageTag instead of toString() or self-implementation. - However, toLanguageTag/forLanguageTag is not available on older Android, so leave a self implementation for backward compatibility. https://codereview.chromium.org/2406203002/diff/240001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/240001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:87: String languageForChrome = LANGUAGE_MAP.get(locale.getLanguage()); Looks like you are doing the same things in three places, forlanguageTag, toLanguageTag and getDefaultLocale. Could you introduce utility function to reduce the code duplication? https://codereview.chromium.org/2406203002/diff/240001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/240001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:18: // TODO(yirui): update tests for LocaleList when SDK Roll is ready. nit: update tests for LocaleList once SDK roll is completed.
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale, language tag format will be well formed in these two forms in LocaleUtils. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation, toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ==========
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation, toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation, toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ==========
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation, toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ========== to ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation. toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ==========
The CQ bit was checked by yirui@google.com to run a CQ dry run
Description is modified. https://codereview.chromium.org/2406203002/diff/240001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/240001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:87: String languageForChrome = LANGUAGE_MAP.get(locale.getLanguage()); On 2016/10/18 04:11:15, Seigo Nonaka wrote: > Looks like you are doing the same things in three places, forlanguageTag, > toLanguageTag and getDefaultLocale. Could you introduce utility function to > reduce the code duplication? Done. https://codereview.chromium.org/2406203002/diff/240001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/240001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:18: // TODO(yirui): update tests for LocaleList when SDK Roll is ready. On 2016/10/18 04:11:15, Seigo Nonaka wrote: > nit: update tests for LocaleList once SDK roll is completed. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
How about Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation and toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation instead of self implementation. However, toLanguageTag and forLangaugeTag is not available on Android APIs level lower than 21. Thus introducing LocaleUtils.forLanguageTag/LocaleUtils.toLanguageTag for keeping backward compatibility. This CL also contains several cleaning up: - LocaleUtils.getDefaultLocale and LocaleUtils.getLocale are misleading. Renamed to LocaleUtils.getDefaultLocaleString and LocaleUtils.toLanguageTag. - Now LocaleUtils.getDefaultLocale() returns Locale object rather than String representation. https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:61: if (languageForChrome != null) { nit: You can return immediately like if (languageForChrome == null) { return locale; } return new Locale.Builder().setLocale(locale).setLanguage(languageForChrome).build();
https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/200001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1193: On 2016/10/17 13:42:28, yirui wrote: > On 2016/10/17 11:34:57, ksk1 wrote: > > No need to update language code? > > If we don't need to update language code, the behavior is now different from > > PwsClientImpl.java. Please update method comment. > > I think they have the same behavior? > for pref_service_bridge.cc, LocaleUtils.getDefaultLocale() is called in > ChromeActivitySessionTracker.java; > for PwsClientImpl.java, LocaleUtils.getDefaultLocale() is also called in > updateAcceptLanguages(). If I understand correctly, deprecated code never be passed to C++ side; however, the behavior of the method is different from Java side one. In C++ side, PrependToAcceptLanguagesIfNecessary("in-ID", "") returns "in-ID". In Java side, prependToAcceptLanguagesIfNecessary("in-ID", "") returns "id-ID". It must be no problem for now, but it might make developers confuse in the future. https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:54: * @return the default locale with updated language codes for Chromium, with translated modern Remove "default"? https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:134: * represents this locale. s/this/default/ ? https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:244: * Get the language code for the default locale and prepend it to the Accept-Language string if s/locale/locales/ https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:247: * @param locales A string representing a default language tag or a list of default language I think this comment sounds strange. How about: comma separated language tags representing a list of default locales. https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:249: * @param acceptLanguages The default language list for the language of the user's locale. s/locale/locales/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Support BCP47 format in accept language list Accept language list is now supporting BCP47 compliant format including 3-letter country code, '-' separator and missing country code cases. It takes all input values as language tags in the form of language code only or language code plus country code. Also, LocaleUtils is called instead of Locale in order to get a well-formed BCP47 representation. toLanguageTag and forLanguageTag are used instead of toString() or self-implementation. However, these two methods are not available for APIs lower than 21. In that case, a self implementation for backward capatibility is involved. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales instead of self implementation. However, toLanguageTag and forLangaugeTag are not valid on Android APIs level lower than 21. Thus, introducing LocaleUtils.forLanguageTag/toLanguageTag for keeping backward compatibility. This CL also contains several cleaning up: - LocaleUtils.getDefaultLocale and LocaleUtils.getLocale are misleading. Renamed to LocaleUtils.getDefaultLocaleString and LocaleUtils.toLanguageTag respectively. - Now LocaleUtils.getDefaultLocale() returns Locale object rather than String representation. BUG=593515 ==========
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...
Description modified, also removed unused statement in Test. https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:54: * @return the default locale with updated language codes for Chromium, with translated modern On 2016/10/18 06:06:50, ksk1 wrote: > Remove "default"? Done. https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:61: if (languageForChrome != null) { On 2016/10/18 05:56:45, Seigo Nonaka wrote: > nit: You can return immediately like > if (languageForChrome == null) { > return locale; > } > return new > Locale.Builder().setLocale(locale).setLanguage(languageForChrome).build(); Done. Also, I set the function as private, since it's only called inside LocaleUtils. https://codereview.chromium.org/2406203002/diff/260001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:134: * represents this locale. On 2016/10/18 06:06:50, ksk1 wrote: > s/this/default/ ? yes, it is a default locale, with an updated language code. https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:244: * Get the language code for the default locale and prepend it to the Accept-Language string if On 2016/10/18 06:06:50, ksk1 wrote: > s/locale/locales/ Done. https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:247: * @param locales A string representing a default language tag or a list of default language On 2016/10/18 06:06:50, ksk1 wrote: > I think this comment sounds strange. > How about: comma separated language tags representing a list of default locales. Done. https://codereview.chromium.org/2406203002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:249: * @param acceptLanguages The default language list for the language of the user's locale. On 2016/10/18 06:06:50, ksk1 wrote: > s/locale/locales/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales instead of self implementation. However, toLanguageTag and forLangaugeTag are not valid on Android APIs level lower than 21. Thus, introducing LocaleUtils.forLanguageTag/toLanguageTag for keeping backward compatibility. This CL also contains several cleaning up: - LocaleUtils.getDefaultLocale and LocaleUtils.getLocale are misleading. Renamed to LocaleUtils.getDefaultLocaleString and LocaleUtils.toLanguageTag respectively. - Now LocaleUtils.getDefaultLocale() returns Locale object rather than String representation. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language code between Android and Java, self implementing forLanguageTag and toLanguageTag in LocaleUtils is needed. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language code between Android and Java, self implementing forLanguageTag and toLanguageTag in LocaleUtils is needed. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag and toLanguageTag in LocaleUtils is needed. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag and toLanguageTag in LocaleUtils is needed. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Also, getLocale and getDefaultLocale are renamed as getLocaleString and gerDefaultLocaleString respectively. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Also, getLocale and getDefaultLocale are renamed as getLocaleString and gerDefaultLocaleString respectively. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. In addition, getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString. BUG=593515 ==========
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...)
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, toLanguageTag should be used instead which returns a IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. In addition, getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, given a locale, a return value with IETF BCP47 language tag representation should be used. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, given a locale, a return value with IETF BCP47 language tag representation should be used. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, LocaleUtils is called for returning a value in IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, LocaleUtils is called for returning a value in IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, self implementing for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, LocaleUtils is called for returning a value in IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, a self-implementation for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ==========
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: Exceeded global retry quota
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...
I'm sorry for confusing you with Locale behavior. I'd like to leave more detailed comments for further development. Please take a look my comments. Thank you. https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:24: // Android uses deprecated lanuages codes for Hebrew, Yiddish and Indonesian but How about moving this comment to getLanguageString? https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:83: * represents this locale. Please add toLanguageTag issue in Android M, like /** * Converts Locale object to the BC47 compliant string format. * * Java uses deprecated language codes for Hebrew, Yiddish and Indonesian but * Chromium uses updated codes. Similarly, Android uses "tl" while Chromium uses "fil" * for Tagalog/Filipino. * Note that we cannot use Locale.getLanguage() and Locale.toLanguageTag() for this purpose. * Locale.getLanguage() returns deprecated language code even if the Locale object is * constructed with updated language code. Even if Locale.toLanguageTag() does a special conversion * from deprecated language code to updated one, it is not usable because this special conversion is * not working Android M or before. * * @return a well-formed IETF BCP 47 language tag with language and country code that * represents this locale. */
The CQ bit was checked by yirui@google.com to run a CQ dry run
https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:24: // Android uses deprecated lanuages codes for Hebrew, Yiddish and Indonesian but On 2016/10/19 06:23:25, Seigo Nonaka wrote: > How about moving this comment to getLanguageString? Done. https://codereview.chromium.org/2406203002/diff/380001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:83: * represents this locale. On 2016/10/19 06:23:25, Seigo Nonaka wrote: > Please add toLanguageTag issue in Android M, like > > /** > * Converts Locale object to the BC47 compliant string format. > * > * Java uses deprecated language codes for Hebrew, Yiddish and Indonesian but > * Chromium uses updated codes. Similarly, Android uses "tl" while Chromium uses > "fil" > * for Tagalog/Filipino. > * Note that we cannot use Locale.getLanguage() and Locale.toLanguageTag() for > this purpose. > * Locale.getLanguage() returns deprecated language code even if the Locale > object is > * constructed with updated language code. Even if Locale.toLanguageTag() does a > special conversion > * from deprecated language code to updated one, it is not usable because this > special conversion is > * not working Android M or before. > * > * @return a well-formed IETF BCP 47 language tag with language and country code > that > * represents this locale. > */ Little changes in words are made.
https://codereview.chromium.org/2406203002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:246: * @param comma separated language tags representing a list of default locales. Put the param name "locales". https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:40: ); Revert?
The CQ bit was checked by yirui@google.com to run a CQ dry run
https://codereview.chromium.org/2406203002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:246: * @param comma separated language tags representing a list of default locales. On 2016/10/19 07:21:06, ksk1 wrote: > Put the param name "locales". Done. https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:40: ); On 2016/10/19 07:21:06, ksk1 wrote: > Revert? I renamed the function name getDefaultLocale in LocaleUtils to getDefaultLocaleString, so I am thinking change the names accordingly in other related C++ files. (File reference: base/android/locale_utils.cc line 19 and line 22)
https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:40: ); On 2016/10/19 07:51:19, yirui wrote: > On 2016/10/19 07:21:06, ksk1 wrote: > > Revert? > > I renamed the function name getDefaultLocale in LocaleUtils to > getDefaultLocaleString, so I am thinking change the names accordingly in other > related C++ files. > (File reference: base/android/locale_utils.cc line 19 and line 22) I understand the method renaming, but you inserted spaces at l.40. Is it intentional change?
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Therefore, LocaleUtils is called for returning a value in IETF BCP47 language tag representation. Also, forLanguageTag should be used for constructing Locale object from String representation of locales. However, due to inconsistent ways of dealing with deprecated language codes between Android and Java, a self-implementation for forLanguageTag in LocaleUtils is needed. Several main updates in LocaleUtils: - getLocale and getDefaultLocale are renamed as getLocaleString and getDefaultLocaleString respectively. - getLocaleLanguage is added for updating the language code only. - self-implementation forLanguageTag is added for creating Locale, given language tag with language and country code in BCP47 format. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getLocale and getDefaultLocale as getLocaleString and getDefaultLocaleString respectively. This leads to function name changes in related files. - Adding getLocaleLanguage for updating the language code only. BUG=593515 ==========
Patchset #18 (id:340001) has been deleted
Patchset #9 (id:160001) has been deleted
Patchset #15 (id:300001) has been deleted
Patchset #14 (id:280001) has been deleted
The CQ bit was checked by yirui@google.com to run a CQ dry run
https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... File chromecast/browser/cast_http_user_agent_settings.cc (right): https://codereview.chromium.org/2406203002/diff/400001/chromecast/browser/cas... chromecast/browser/cast_http_user_agent_settings.cc:40: ); On 2016/10/19 07:59:53, ksk1 wrote: > On 2016/10/19 07:51:19, yirui wrote: > > On 2016/10/19 07:21:06, ksk1 wrote: > > > Revert? > > > > I renamed the function name getDefaultLocale in LocaleUtils to > > getDefaultLocaleString, so I am thinking change the names accordingly in other > > related C++ files. > > (File reference: base/android/locale_utils.cc line 19 and line 22) > > I understand the method renaming, but you inserted spaces at l.40. Is it > intentional change? No, that should be reverted. Thanks.
Patchset #18 (id:420001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by yirui@google.com
The CQ bit was checked by yirui@google.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by yirui@google.com
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...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #17 (id:440001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #3 (id:60001) 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...
Patchset #14 (id:460001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The code passed the CQ dry run, please have a check.
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 #14 (id:480001) has been deleted
lgtm, thank you for your tough and tricky work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yirui@google.com changed reviewers: + bauerb@chromium.org
+bauerb as an owner of the Clank. Here is the code for using BCP47 compliant format for locale representation. Please take a look. Thanks in advance.
https://codereview.chromium.org/2406203002/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:246: * @param a comma separated string that represents a list of default locales. @param locales a comma...
lgtm
https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:40: public static Locale forLanguageTag(String languageTag) { Is there a reason you're not using Locale.forLanguageTag at least on L+? https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:69: String updatedLanguageCode = LANGUAGE_MAP.get(originalLanguageCode); You could extract this (return the override from the map if it exists, otherwise the original value) into a shared method. https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:80: * special conversion is not working Android M or before. Could you have a runtime check that does use Locale.toLanguageTag() on N+? https://codereview.chromium.org/2406203002/diff/500001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/500001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:15: public class LocaleUtilsTest extends InstrumentationTestCase { Could this be a JUnit test instead of an instrumentation test? Those are way more lightweight. https://codereview.chromium.org/2406203002/diff/500001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/500001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1152: // language tags(BCP47 compliant format). Each language tag contains a language Nit: space before opening parenthesis.
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...
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getLocale and getDefaultLocale as getLocaleString and getDefaultLocaleString respectively. This leads to function name changes in related files. - Adding getLocaleLanguage for updating the language code only. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for updating language code that is used for Chrome. - Adding getDefaultLocale for botaining a locale with updated language code. BUG=593515 ==========
I noticed that in PwsClientImplTest, Locale.toLanguageTag is called with higher Versions, which means if the make-up language code like "xx" will return "und". Therefore, I changed makeup test cases with real language code and country code with correct format. https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:40: public static Locale forLanguageTag(String languageTag) { On 2016/10/20 12:44:27, Bernhard Bauer wrote: > Is there a reason you're not using Locale.forLanguageTag at least on L+? reverted code, so Locale.forLanguageTag is used for L+ now. also, Locale.toLanguageTag is used for N+. https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:69: String updatedLanguageCode = LANGUAGE_MAP.get(originalLanguageCode); On 2016/10/20 12:44:27, Bernhard Bauer wrote: > You could extract this (return the override from the map if it exists, otherwise > the original value) into a shared method. This function is extracted as updateLanguageForChromium. Meanwhile, updateLocaleForChromium is added for a return value of Locale with updated language code. https://codereview.chromium.org/2406203002/diff/500001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:80: * special conversion is not working Android M or before. On 2016/10/20 12:44:27, Bernhard Bauer wrote: > Could you have a runtime check that does use Locale.toLanguageTag() on N+? Done. https://codereview.chromium.org/2406203002/diff/500001/base/android/javatests... File base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java (right): https://codereview.chromium.org/2406203002/diff/500001/base/android/javatests... base/android/javatests/src/org/chromium/base/LocaleUtilsTest.java:15: public class LocaleUtilsTest extends InstrumentationTestCase { On 2016/10/20 12:44:27, Bernhard Bauer wrote: > Could this be a JUnit test instead of an instrumentation test? Those are way > more lightweight. Done. https://codereview.chromium.org/2406203002/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java (right): https://codereview.chromium.org/2406203002/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:246: * @param a comma separated string that represents a list of default locales. On 2016/10/20 11:28:59, ksk1 wrote: > @param locales a comma... Done. https://codereview.chromium.org/2406203002/diff/500001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2406203002/diff/500001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:1152: // language tags(BCP47 compliant format). Each language tag contains a language On 2016/10/20 12:44:27, Bernhard Bauer wrote: > Nit: space before opening parenthesis. Done.
Thanks! https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:54: @SuppressLint("NewApi") Use @TargetApi(Build.VERSION_CODES.LOLLIPOP) to get a narrower suppression. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:57: if (languageForChrome == null) { Would this actually be null? https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = updateLanguageForChromium(tag[0]); Wait, isn't the input language going to be the Chromium value here and we'd want to convert it to Android language instead of the other way around? https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:112: if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) { Could you add a comment to use >= N here once we build against an SDK that includes the constant? https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:128: return updateLocaleForChromium(locale); Isn't this method only allowed to be called on L+ (because of the Locale.Builder)?
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...)
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.
https://codereview.chromium.org/2406203002/diff/560001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/560001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = getUpdateLanguage(tag[0]); Please add empty check here since languageTag.split("-") returns empty array for "-".
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 #15 (id:520001) 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/2406203002/diff/540001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:54: @SuppressLint("NewApi") On 2016/10/21 09:48:54, Bernhard Bauer wrote: > Use @TargetApi(Build.VERSION_CODES.LOLLIPOP) to get a narrower suppression. Done. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:57: if (languageForChrome == null) { On 2016/10/21 09:48:54, Bernhard Bauer wrote: > Would this actually be null? Actually, the return value of Locale.getLanguage would be the empty string if none is defined. So this is now set to isEmpty() check. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = updateLanguageForChromium(tag[0]); On 2016/10/21 09:48:54, Bernhard Bauer wrote: > Wait, isn't the input language going to be the Chromium value here and we'd want > to convert it to Android language instead of the other way around? Good point here. I did some tests on different Android devices. For versions Jelly Bean and higher, both updated and deprecated language codes are pointing to the same Locale. So it might not be necessary to convert language code back to Android language. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:112: if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) { On 2016/10/21 09:48:54, Bernhard Bauer wrote: > Could you add a comment to use >= N here once we build against an SDK that > includes the constant? Done. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:128: return updateLocaleForChromium(locale); On 2016/10/21 09:48:54, Bernhard Bauer wrote: > Isn't this method only allowed to be called on L+ (because of the > Locale.Builder)? Yes, that is correct. Actually, I added this function because of the call from ResourceExtractor, but all it needs is only an updated language code, so I decide to remove this function and change getUpdateLanguage to public. Also, those two update functions are renamed in a neater way. https://codereview.chromium.org/2406203002/diff/560001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/560001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = getUpdateLanguage(tag[0]); On 2016/10/24 00:58:22, Seigo Nonaka wrote: > Please add empty check here since languageTag.split("-") returns empty array for > "-". Done.
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for updating language code that is used for Chrome. - Adding getDefaultLocale for botaining a locale with updated language code. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for updating language code that is used for Chrome. - Adding methods for returning a locale with updated language code that is used for Chrome. BUG=593515 ==========
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for updating language code that is used for Chrome. - Adding methods for returning a locale with updated language code that is used for Chrome. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding a method for updating language code that is used for Chrome. - Adding a method for returning a locale with updated language code that is used for Chrome. BUG=593515 ==========
Patchset #11 (id:360001) has been deleted
https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:57: if (languageForChrome == null) { On 2016/10/24 05:29:59, yirui wrote: > On 2016/10/21 09:48:54, Bernhard Bauer wrote: > > Would this actually be null? > > Actually, the return value of Locale.getLanguage would be the empty string if > none is defined. So this is now set to isEmpty() check. Oh, this is for the case where locale.getLanguage() returns an empty string? Do you need to handle that in toLanguageTag() below as well? https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = updateLanguageForChromium(tag[0]); On 2016/10/24 05:29:58, yirui wrote: > On 2016/10/21 09:48:54, Bernhard Bauer wrote: > > Wait, isn't the input language going to be the Chromium value here and we'd > want > > to convert it to Android language instead of the other way around? > > Good point here. I did some tests on different Android devices. For versions > Jelly Bean and higher, both updated and deprecated language codes are pointing > to the same Locale. So it might not be necessary to convert language code back > to Android language. Ok... in that case, do you even need the call to getUpdateLocale() in forLanguageTag()? And if so, you could even inline the other call to getUpdateLocale() and pull out the call to getUpdateLanguage(). https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:45: public static String getUpdateLanguage(String language) { Nit: "getUpdatedLanguage". Or actually, maybe use "Chromium" in the method name, to make it clearer what the update is? https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:55: private static Locale getUpdateLocale(Locale locale) { And "getUpdatedLocale". https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:69: if (languageTag.length() == 0) { Actually, you might not need this now -- an empty string will just split to an empty list, and you'll catch that in line 73. https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:100: return LocaleUtils.forLanguageTagCompat(languageTag); LocaleUtils. isn't necessary.
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.
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/2406203002/diff/540001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:57: if (languageForChrome == null) { On 2016/10/24 12:46:01, Bernhard Bauer wrote: > On 2016/10/24 05:29:59, yirui wrote: > > On 2016/10/21 09:48:54, Bernhard Bauer wrote: > > > Would this actually be null? > > > > Actually, the return value of Locale.getLanguage would be the empty string if > > none is defined. So this is now set to isEmpty() check. > > Oh, this is for the case where locale.getLanguage() returns an empty string? Do > you need to handle that in toLanguageTag() below as well? Sorry for the confusion, for this part, what actually needs is to check if there's any update to language code, so LANGUAGE_MAP.get should be used and the condition "languageForChrome == null" stay the same. https://codereview.chromium.org/2406203002/diff/540001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:73: String language = updateLanguageForChromium(tag[0]); On 2016/10/24 12:46:01, Bernhard Bauer wrote: > On 2016/10/24 05:29:58, yirui wrote: > > On 2016/10/21 09:48:54, Bernhard Bauer wrote: > > > Wait, isn't the input language going to be the Chromium value here and we'd > > want > > > to convert it to Android language instead of the other way around? > > > > Good point here. I did some tests on different Android devices. For versions > > Jelly Bean and higher, both updated and deprecated language codes are pointing > > to the same Locale. So it might not be necessary to convert language code back > > to Android language. > > Ok... in that case, do you even need the call to getUpdateLocale() in > forLanguageTag()? > > And if so, you could even inline the other call to getUpdateLocale() and pull > out the call to getUpdateLanguage(). I tested with "tl"/"fil", it turns out that Tagalog/Filipino is different. Unlike the other 3 language codes, the Chromium and deprecated language code are not pointing to the same Locale. Therefore, as you pointed out, I should change "fil" back to "tl" in forLanguageTag. As for toLanguageTag, all language codes should be mapped to Chromium language code. I also added some special cases such as "und" language code and "nn-NO" language tag, so that it works as the same as in toLanguageTag/forLanguageTag for these cases. https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:45: public static String getUpdateLanguage(String language) { On 2016/10/24 12:46:01, Bernhard Bauer wrote: > Nit: "getUpdatedLanguage". Or actually, maybe use "Chromium" in the method name, > to make it clearer what the update is? Done. https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:55: private static Locale getUpdateLocale(Locale locale) { On 2016/10/24 12:46:01, Bernhard Bauer wrote: > And "getUpdatedLocale". Done. https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:69: if (languageTag.length() == 0) { On 2016/10/24 12:46:01, Bernhard Bauer wrote: > Actually, you might not need this now -- an empty string will just split to an > empty list, and you'll catch that in line 73. Done. https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:100: return LocaleUtils.forLanguageTagCompat(languageTag); On 2016/10/24 12:46:01, Bernhard Bauer wrote: > LocaleUtils. isn't necessary. Done.
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding a method for updating language code that is used for Chrome. - Adding a method for returning a locale with updated language code that is used for Chrome. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for switching language code between Chrome and Android. - Adding methods for returning a locale with updated language code for Chrome or Android. BUG=593515 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Almost there :) https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:45: public static String getUpdateLanguage(String language) { On 2016/10/25 08:21:11, yirui wrote: > On 2016/10/24 12:46:01, Bernhard Bauer wrote: > > Nit: "getUpdatedLanguage". Or actually, maybe use "Chromium" in the method > name, > > to make it clearer what the update is? > > Done. Thanks, but please use "updated" rather than "update", because the latter sounds like an imperative or like it's referring to an update. https://codereview.chromium.org/2406203002/diff/620001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/620001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:106: // Check the case where language code is "und". This case would already be caught in line 110, because the length of |language| is going to be 0.
Patchset #19 (id:640001) has been deleted
The CQ bit was checked by yirui@google.com to run a CQ dry run
Patchset #19 (id:660001) 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...
Patchset #17 (id:600001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/580001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:45: public static String getUpdateLanguage(String language) { On 2016/10/25 09:46:29, Bernhard Bauer wrote: > On 2016/10/25 08:21:11, yirui wrote: > > On 2016/10/24 12:46:01, Bernhard Bauer wrote: > > > Nit: "getUpdatedLanguage". Or actually, maybe use "Chromium" in the method > > name, > > > to make it clearer what the update is? > > > > Done. > > Thanks, but please use "updated" rather than "update", because the latter sounds > like an imperative or like it's referring to an update. Done. https://codereview.chromium.org/2406203002/diff/620001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/620001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:106: // Check the case where language code is "und". On 2016/10/25 09:46:29, Bernhard Bauer wrote: > This case would already be caught in line 110, because the length of |language| > is going to be 0. Oh actually, what I should check is "language == 'und'". Since in Locale.forLanguageTag, it converts language "und" to "".
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 #18 (id:680001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
LGTM!
Patchset #18 (id:700001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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, nona@chromium.org Link to the patchset: https://codereview.chromium.org/2406203002/#ps720001 (title: "Rebased, using .equals(), create compat for forLanguageTag, more tests added")
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...)
yirui@google.com changed reviewers: + jam@chromium.org
+jam as an owner of the Clank. Here is the code for using BCP47 compliant format for locale representation. Please take a look. Thanks in advance.
On 2016/10/26 10:52:42, yirui wrote: > +jam as an owner of the Clank. Here is the code for using > BCP47 compliant format for locale representation. > Please take a look. > Thanks in advance. Which directories are you looking for review? Please pick specific owners, i.e. android_webview/owners chrome/android/owners chrome/browser/android/owners
yirui@google.com changed reviewers: + alokp@chromium.org, jshin@chromium.org, torne@chromium.org
-jam, My apologize for being vague about review directories. I added owners for specific directories checking. Adding specific owners, torne@, please take a look at android_webview, base/android changes. alokp@, please take a look at chromecast changes. jshin@, please take a look at ui/base/l10n changes.
yirui@google.com changed reviewers: - jam@chromium.org
chromecast/ rs lgtm
LGTM https://codereview.chromium.org/2406203002/diff/720001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/720001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:133: * Converts Locale object to the BC47 compliant string format. nit: BC47 => BCP 47 https://codereview.chromium.org/2406203002/diff/720001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:148: * Converts Locale object to the BC47 compliant string format. nit: BC47 => BCP 47
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/2406203002/diff/720001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2406203002/diff/720001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:133: * Converts Locale object to the BC47 compliant string format. On 2016/10/27 06:50:52, jungshik at google wrote: > nit: BC47 => BCP 47 Done. https://codereview.chromium.org/2406203002/diff/720001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:148: * Converts Locale object to the BC47 compliant string format. On 2016/10/27 06:50:52, jungshik at google wrote: > nit: BC47 => BCP 47 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
android_webview LGTM
The CQ bit was checked by yirui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, ksk@google.com, jshin@chromium.org, nona@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2406203002/#ps740001 (title: "BCP 47, change in description")
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 ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for switching language code between Chrome and Android. - Adding methods for returning a locale with updated language code for Chrome or Android. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for switching language code between Chrome and Android. - Adding methods for returning a locale with updated language code for Chrome or Android. BUG=593515 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:740001)
Message was sent while issue was closed.
Description was changed from ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for switching language code between Chrome and Android. - Adding methods for returning a locale with updated language code for Chrome or Android. BUG=593515 ========== to ========== Use BCP47 compliant format for locale representation. Locale.toString() is not a interchangeable representation. Locale.toLanguageTag should be used for obtaining a value in IETF BCP47 language tag representation. However, toLanguageTag does not work on Android M and before. Thereofore, we use a self-implementation for converting Locale object to BCP47 compliant format string in that situation. Similarly, Locale.forLanguageTag should be used for constructing Locale object from BCP47 String representation of locales. However, this is not available on Android L and before. Thus, we introduce self-implementation for backward compatibility. This CL also includes: - Renaming getDefaultLocale to getDefaultLocaleString. This leads to function name changes in related files. - Adding methods for switching language code between Chrome and Android. - Adding methods for returning a locale with updated language code for Chrome or Android. BUG=593515 Committed: https://crrev.com/9174164b690d246b72273cb4df448224ed8c4f7e Cr-Commit-Position: refs/heads/master@{#428281} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/9174164b690d246b72273cb4df448224ed8c4f7e Cr-Commit-Position: refs/heads/master@{#428281} |
