Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496183002:
Use GetDefaultLocaleListString for returning LocaleList (Closed)
|
Created:
4 years, 1 month ago by Yirui Huang Modified:
4 years, 1 month ago CC:
chromium-reviews, android-webview-reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse GetDefaultLocaleListString for returning LocaleList
Reverted GetDefaultLocaleString for returning a single default
locale. Added GetDefaultLocaleString for getting a default LocaleList
when it is Android N or after and a single default locale otherwise.
By doing so, existing behaviors will be maintained.
Accept languages in both Chrome and Webview are able to take a locale
list. However, detailed changes in Webview are required, such as
GetApplicationLocale in aw_content_browser_client.cc, so that Webview
will be fully ready to take a locale list.
BUG=593515
Committed: https://crrev.com/12d0a4bb3cc3ab1f715c06f11421f24d3c2bc9fc
Cr-Commit-Position: refs/heads/master@{#433037}
Patch Set 1 : rebased to master #Patch Set 2 : extract setLocale method, since configuration.getLocales() returns the same as LocaleUtils.getDefau… #
Total comments: 16
Patch Set 3 : an integration test added, suspended nativecall for getDefaultLocaleListString #
Total comments: 2
Patch Set 4 : comments modified, only BCP47 format in accept languages #
Total comments: 8
Patch Set 5 : change Locales to LocaleList #
Total comments: 2
Patch Set 6 : change locales to localelist in AWContents.java #Messages
Total messages: 64 (47 generated)
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Use GetDefaultLocaleListString for LocaleList Revert GetDefaultLocaleString for returning a single default locale, add GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. This CL also modified accept language list for Webview, it matches the form of accept language list in Chrome now. Changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be ready to take a locale list. BUG=593515 ========== to ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. This CL also modified accept language list for Webview, now it is in the same form as accept language list in Chrome. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ==========
yirui@google.com changed reviewers: + nona@chromium.org
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...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) 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...
Description was changed from ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. This CL also modified accept language list for Webview, now it is in the same form as accept language list in Chrome. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ========== to ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:165: std::string AwContentBrowserClient::GetAcceptLangsImpl() { Question: Is there no unit tests for this function? If no, how about adding test cases for making sure this change keeps existing behaviors. https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); Question: do we no longer need to compare in case insensitively? https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:697: setLocale(); setLocale() sounds confusing since it doesn't accept any arguments. How about "applyLocaleChanges()" or "updateDefaultLocale()"? https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1024: // Set and keep updating locales for webview. I'm confused with "keep updating". What does it indicate? https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1030: nativeSetLocale(LocaleUtils.getDefaultLocaleString(), sCurrentLocales); I think it is good to leave comments why you still need to pass the single locale. https://codereview.chromium.org/2496183002/diff/120001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2496183002/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:194: return toLanguageTag(Locale.getDefault()); return getDefaultLocaleString(); ? https://codereview.chromium.org/2496183002/diff/120001/base/android/locale_ut... File base/android/locale_utils.h (right): https://codereview.chromium.org/2496183002/diff/120001/base/android/locale_ut... base/android/locale_utils.h:21: BASE_EXPORT std::string GetDefaultLocaleListString(); Who uses this function? If nobody uses now, let's remove it. We should add new function when it is necessary.
https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:165: std::string AwContentBrowserClient::GetAcceptLangsImpl() { On 2016/11/15 05:15:59, Seigo Nonaka wrote: > Question: Is there no unit tests for this function? If no, how about adding test > cases for making sure this change keeps existing behaviors. Done. https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); On 2016/11/15 05:15:59, Seigo Nonaka wrote: > Question: do we no longer need to compare in case insensitively? I thought locales_string that we got from here is from LocaleUtils.getDefaultLocaleListString, which returns a well formed BCP47 form. Added lower case checking just in case. https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:697: setLocale(); On 2016/11/15 05:15:59, Seigo Nonaka wrote: > setLocale() sounds confusing since it doesn't accept any arguments. > How about "applyLocaleChanges()" or "updateDefaultLocale()"? Done. https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1024: // Set and keep updating locales for webview. On 2016/11/15 05:15:59, Seigo Nonaka wrote: > I'm confused with "keep updating". What does it indicate? Done. https://codereview.chromium.org/2496183002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1030: nativeSetLocale(LocaleUtils.getDefaultLocaleString(), sCurrentLocales); On 2016/11/15 05:15:59, Seigo Nonaka wrote: > I think it is good to leave comments why you still need to pass the single > locale. Done. https://codereview.chromium.org/2496183002/diff/120001/base/android/java/src/... File base/android/java/src/org/chromium/base/LocaleUtils.java (right): https://codereview.chromium.org/2496183002/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/LocaleUtils.java:194: return toLanguageTag(Locale.getDefault()); On 2016/11/15 05:15:59, Seigo Nonaka wrote: > return getDefaultLocaleString(); > ? Done. https://codereview.chromium.org/2496183002/diff/120001/base/android/locale_ut... File base/android/locale_utils.h (right): https://codereview.chromium.org/2496183002/diff/120001/base/android/locale_ut... base/android/locale_utils.h:21: BASE_EXPORT std::string GetDefaultLocaleListString(); On 2016/11/15 05:15:59, Seigo Nonaka wrote: > Who uses this function? > If nobody uses now, let's remove it. > We should add new function when it is necessary. Removed. This will be used once webview is fully ready for taking a locale list.
yirui@google.com changed reviewers: + ksk@google.com
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
mostly l-g-t-m. Please address my comments. https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); On 2016/11/16 00:23:45, yirui wrote: > On 2016/11/15 05:15:59, Seigo Nonaka wrote: > > Question: do we no longer need to compare in case insensitively? > > I thought locales_string that we got from here is from > LocaleUtils.getDefaultLocaleListString, which returns a well formed BCP47 form. > Added lower case checking just in case. Please note that developer can set accept language manually in Android WebView. Please see what happens when developer passes not well-formed BCP47 locale string. c.f. http://stackoverflow.com/questions/12244375/how-can-i-override-android-webvie... https://codereview.chromium.org/2496183002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1031: // A single locale is passed so that it can be used to maintain behaviors How about We cannot use the first language in sCurrentLocales for the UI language even on Android N. LocaleUtils.getDefaultLocaleString() is capable for UI langauge but it is not guaranteed to be listed at the first of sCurrentLocales.
Patchset #4 (id:200001) has been deleted
https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/120001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); On 2016/11/16 01:26:14, Seigo Nonaka wrote: > On 2016/11/16 00:23:45, yirui wrote: > > On 2016/11/15 05:15:59, Seigo Nonaka wrote: > > > Question: do we no longer need to compare in case insensitively? > > > > I thought locales_string that we got from here is from > > LocaleUtils.getDefaultLocaleListString, which returns a well formed BCP47 > form. > > Added lower case checking just in case. > > Please note that developer can set accept language manually in Android WebView. > Please see what happens when developer passes not well-formed BCP47 locale > string. > > c.f. > http://stackoverflow.com/questions/12244375/how-can-i-override-android-webvie... I see. Thanks for pointing this out. I wasn't aware of developer side. https://codereview.chromium.org/2496183002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1031: // A single locale is passed so that it can be used to maintain behaviors On 2016/11/16 01:26:14, Seigo Nonaka wrote: > How about > > We cannot use the first language in sCurrentLocales for the UI language even on > Android N. LocaleUtils.getDefaultLocaleString() is capable for UI langauge but > it is not guaranteed to be listed at the first of sCurrentLocales. That is a clear explanation. Thanks!
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
Patchset #4 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
yirui@google.com changed reviewers: + bauerb@chromium.org, torne@chromium.org
+torne@ as owner of android_webview and base/android. +bauerb@ as owner of chrome/android. This CL uses a new function to return a LocaleList. Thus, existing behaviors will be maintained. Please have a look. Thanks in advance.
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_...)
https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:162: // PrependToAcceptLanguagesIfNecessary Nit: Don't align comments on subsequent lines. https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); Nit: I would probably make this underscore_case, i.e. found_en_us. And also, if the name is "found" and you're only using the value to check whether the string is contained, maybe move the check here and make it a bool? https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... File android_webview/browser/aw_locale_manager.h (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_locale_manager.h:22: virtual std::string GetLocales() = 0; Maybe name this GetLocaleList(), so it's consistent with the Java version, and also more visually distinct from GetLocale()? And also, document the format of the string? https://codereview.chromium.org/2496183002/diff/260001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1032: // Android N. LocaleUtils.getDefaultLocaleString() is capable for UI langauge but Nit: "language"
Patchset #4 (id:240001) has been deleted
https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:162: // PrependToAcceptLanguagesIfNecessary On 2016/11/16 16:20:07, Bernhard Bauer wrote: > Nit: Don't align comments on subsequent lines. Done. https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:171: size_t found_enUS = locales_string.find("en-US"); On 2016/11/16 16:20:07, Bernhard Bauer wrote: > Nit: I would probably make this underscore_case, i.e. found_en_us. And also, if > the name is "found" and you're only using the value to check whether the string > is contained, maybe move the check here and make it a bool? Thanks, underscore looks clearer. Now I changed it so "locales_string.find("en-US") == std::string::npos" is checked directly https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... File android_webview/browser/aw_locale_manager.h (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/browse... android_webview/browser/aw_locale_manager.h:22: virtual std::string GetLocales() = 0; On 2016/11/16 16:20:07, Bernhard Bauer wrote: > Maybe name this GetLocaleList(), so it's consistent with the Java version, and > also more visually distinct from GetLocale()? And also, document the format of > the string? Done. https://codereview.chromium.org/2496183002/diff/260001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/260001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1032: // Android N. LocaleUtils.getDefaultLocaleString() is capable for UI langauge but On 2016/11/16 16:20:07, Bernhard Bauer wrote: > Nit: "language" Done.
The CQ bit was checked by yirui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
One nit, but otherwise LGTM https://codereview.chromium.org/2496183002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3319: private static native void nativeUpdateDefaultLocale(String locale, String locales); nit: can you name the second parameter here locale_list like the C++ code does? It's nicer when the names match :)
The CQ bit was checked by yirui@google.com to run a CQ dry run
Thank you! https://codereview.chromium.org/2496183002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2496183002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3319: private static native void nativeUpdateDefaultLocale(String locale, String locales); On 2016/11/17 12:08:05, Torne wrote: > nit: can you name the second parameter here locale_list like the C++ code does? > It's nicer when the names match :) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yirui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nona@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2496183002/#ps300001 (title: "change locales to localelist in AWContents.java")
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 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...
Message was sent while issue was closed.
Description was changed from ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ========== to ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 ========== to ========== Use GetDefaultLocaleListString for returning LocaleList Reverted GetDefaultLocaleString for returning a single default locale. Added GetDefaultLocaleString for getting a default LocaleList when it is Android N or after and a single default locale otherwise. By doing so, existing behaviors will be maintained. Accept languages in both Chrome and Webview are able to take a locale list. However, detailed changes in Webview are required, such as GetApplicationLocale in aw_content_browser_client.cc, so that Webview will be fully ready to take a locale list. BUG=593515 Committed: https://crrev.com/12d0a4bb3cc3ab1f715c06f11421f24d3c2bc9fc Cr-Commit-Position: refs/heads/master@{#433037} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/12d0a4bb3cc3ab1f715c06f11421f24d3c2bc9fc Cr-Commit-Position: refs/heads/master@{#433037} |
