Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10224004:
Use Android API for GetDisplayNameForLocale(). (Closed)
|
Created:
8 years, 8 months ago by Xianzhu Modified:
8 years, 7 months ago Reviewers:
jungshik, sky, John Grabowski, jungshik at Google, Mark Mentovai, wangxianzhu, tony, Ilya Sherman CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org, dyu1, dhollowa+watch_chromium.org, Ilya Sherman, jshin+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse Android API for GetDisplayNameForLocale().
Using Android API, we can reduce the data size of ICU and thus reduce the
binary size of chromium-android.
BUG=none
TEST=L10nUtilTest.GetDisplayNameForLocale,L10nUtilTest.GetDisplayNameForCountry
TBR=sky
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135484
Patch Set 1 #
Total comments: 25
Patch Set 2 : Changes according to review comments #Patch Set 3 : Remove is_for_ui parameter of GetDisplayNameForCountry #Patch Set 4 : Add Junshik into ui/base/l10n/OWNERS #Patch Set 5 : jungshik@chromium.org -> jungshik@google.com #
Total comments: 2
Patch Set 6 : jungshik@google.com->jshin@chromium.org in OWNERS #
Total comments: 5
Patch Set 7 : Add TODOs #Patch Set 8 : Rebase #Patch Set 9 : Workaround CQ issue (by removing ui/base/l10n/OWNERS from this CL) #Patch Set 10 : For landing #
Messages
Total messages: 41 (0 generated)
Hi, reviewers, I added you as reviewer because the CL contains changes in the directory where you are listed in OWNERS. So you can just look at the change under your directory. Thanks, Xianzhu
http://codereview.chromium.org/10224004/diff/1/build/all_android.gyp File build/all_android.gyp (left): http://codereview.chromium.org/10224004/diff/1/build/all_android.gyp#oldcode84 build/all_android.gyp:84: '../ui/ui.gyp:gfx_unittests', This is removed because gfx_unittests is a legacy alias of ui_unittests. http://codereview.chromium.org/10224004/diff/1/build/android/gtest_filter/ui_... File build/android/gtest_filter/ui_unittests_disabled (right): http://codereview.chromium.org/10224004/diff/1/build/android/gtest_filter/ui_... build/android/gtest_filter/ui_unittests_disabled:45: L10nUtilTest.GetDisplayNameForLocale These tests are disabled because they can only be run in ui_unittests_apk (which doesn't filter tests here for now)
I only reviewed the files in base and build. http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... File base/android/java/org/chromium/base/LocaleUtils.java (right): http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... base/android/java/org/chromium/base/LocaleUtils.java:22: return locale.getLanguage() + "-" + locale.getCountry(); Is this always right? Will there never be a case where we intend something like “en” without a country? Languages like “ru”? What about cases like “es-419”, does this handle those? Should it? http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc File base/android/locale_utils.cc (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc#n... base/android/locale_utils.cc:35: DCHECK(U_SUCCESS(error)); #include "base/logging.h" for this. http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h File base/android/locale_utils.h (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:7: #pragma once http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:9: #include <string> Blank line between C system headers and C++ system headers.
autofill/ lgtm http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... File chrome/browser/autofill/autofill_country.cc (right): http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_country.cc:486: icu::Locale icu_locale(locale.c_str()); nit: This seems to no longer be used. http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_country.cc:497: locale, false); nit: I don't fully follow the significance of the last parameter, but the localized country names are indeed used in the UI -- though I don't think there's ever any other text adjacent to them. So, I'm not sure what the value of this last param ought to be; wanted to make sure you'd considered it and chosen appropriately, rather than just defaulting to |false|. http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.h File ui/base/l10n/l10n_util.h (right): http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.h#newco... ui/base/l10n/l10n_util.h:54: // Returns the display name of the |country_code| in |display_locale|. nit: You should either include a comment describing the use of |is_for_ui|, or remove it from the API.
base/android/ LGTM if you can confirm the ui apk compiles and links happily when re-gypping with GYP_DEFINES="$GYP_DEFINES gtest_target_type=shared_library" android_gyp Please wait for other LGTMs http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc File base/android/locale_utils.cc (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc#n... base/android/locale_utils.cc:27: int32_t (*uloc_func)(const char*, char*, int32_t, UErrorCode*), consider a typedef for this func pointer http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h File base/android/locale_utils.h (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:7: On 2012/04/25 20:00:20, Mark Mentovai wrote: > #pragma once I know this is the style guide, but is it really necessary for headers only compiled on Android?
http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h File base/android/locale_utils.h (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:7: John Grabowski wrote: > On 2012/04/25 20:00:20, Mark Mentovai wrote: > > #pragma once > > I know this is the style guide, but is it really necessary for headers only > compiled on Android? Strictly speaking? No, due to the optimization that GCC and Clang do when they see the #define guard pattern. But it’s part of the boilerplate so I don’t know why it wouldn’t be here. We don’t make an exception for Mac-only or Linux-only headers, and it’s not necessary in those cases for the same reason.
http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc File ui/base/l10n/l10n_util.cc (right): http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc#newc... ui/base/l10n/l10n_util.cc:521: return GetDisplayNameForLocale("_" + country_code, display_locale, is_for_ui); This hardly seems worth adding a new function for. Are you really using this enough to warrant that? Assuming you are, I would rather see a GetLocaleForCountryCode() so that instead you do: GetDisplayNameForLocale(GetLocaleForCountryCode(), ...) That said, I don't know enough about locale->country code to say if this is right.
http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... File base/android/java/org/chromium/base/LocaleUtils.java (right): http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... base/android/java/org/chromium/base/LocaleUtils.java:22: return locale.getLanguage() + "-" + locale.getCountry(); On 2012/04/25 20:00:20, Mark Mentovai wrote: > Is this always right? Will there never be a case where we intend something like > “en” without a country? Languages like “ru”? What about cases like “es-419”, > does this handle those? Should it? I think it should handle locales without country. "es-419" should be handled automatically if Java Locale returns language "es" and country "419". http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc File base/android/locale_utils.cc (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc#n... base/android/locale_utils.cc:27: int32_t (*uloc_func)(const char*, char*, int32_t, UErrorCode*), On 2012/04/25 20:32:27, John Grabowski wrote: > consider a typedef for this func pointer Done. http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.cc#n... base/android/locale_utils.cc:35: DCHECK(U_SUCCESS(error)); On 2012/04/25 20:00:20, Mark Mentovai wrote: > #include "base/logging.h" for this. Done. http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h File base/android/locale_utils.h (right): http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:7: On 2012/04/25 20:39:58, Mark Mentovai wrote: > John Grabowski wrote: > > On 2012/04/25 20:00:20, Mark Mentovai wrote: > > > #pragma once > > > > I know this is the style guide, but is it really necessary for headers only > > compiled on Android? > > Strictly speaking? No, due to the optimization that GCC and Clang do when they > see the #define guard pattern. But it’s part of the boilerplate so I don’t know > why it wouldn’t be here. We don’t make an exception for Mac-only or Linux-only > headers, and it’s not necessary in those cases for the same reason. Done. http://codereview.chromium.org/10224004/diff/1/base/android/locale_utils.h#ne... base/android/locale_utils.h:9: #include <string> On 2012/04/25 20:00:20, Mark Mentovai wrote: > Blank line between C system headers and C++ system headers. Done. http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... File chrome/browser/autofill/autofill_country.cc (right): http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_country.cc:486: icu::Locale icu_locale(locale.c_str()); On 2012/04/25 20:10:36, Ilya Sherman wrote: > nit: This seems to no longer be used. Done. http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_country.cc:497: locale, false); On 2012/04/25 20:10:36, Ilya Sherman wrote: > nit: I don't fully follow the significance of the last parameter, but the > localized country names are indeed used in the UI -- though I don't think > there's ever any other text adjacent to them. So, I'm not sure what the value > of this last param ought to be; wanted to make sure you'd considered it and > chosen appropriately, rather than just defaulting to |false|. I'm also not sure about this. I included the parameter in the function to match the original l10n_util::GetDisplayNameForLocale(), and use |false| here to the changed code equivalent to the original code. The is_for_ui parameter of GetDisplayNameForLocale has been there since the first public commit of l10n_util.h. I doubt if it is still useful now. I'll ask related people about it. http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.h File ui/base/l10n/l10n_util.h (right): http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.h#newco... ui/base/l10n/l10n_util.h:54: // Returns the display name of the |country_code| in |display_locale|. On 2012/04/25 20:10:36, Ilya Sherman wrote: > nit: You should either include a comment describing the use of |is_for_ui|, or > remove it from the API. Done.
http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc File ui/base/l10n/l10n_util.cc (right): http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc#newc... ui/base/l10n/l10n_util.cc:521: return GetDisplayNameForLocale("_" + country_code, display_locale, is_for_ui); On 2012/04/25 21:06:43, sky wrote: > This hardly seems worth adding a new function for. Are you really using this > enough to warrant that? > Assuming you are, I would rather see a GetLocaleForCountryCode() so that instead > you do: > > GetDisplayNameForLocale(GetLocaleForCountryCode(), ...) > > That said, I don't know enough about locale->country code to say if this is > right. Both ICU uloc_getDisplayName and base::android::GetDisplayNameForLocale() will return the display name of the country for a locale string in format of '"_" + country_code'. I think we can also make this rule exposed for l10n_util::GetDisplayNameForLocale(), so that we can eliminate the new function. For GetLocaleForCountryCode(), I feel it a bit strange because "_CC" doesn't look a valid locale to me.
http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... File base/android/java/org/chromium/base/LocaleUtils.java (right): http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... base/android/java/org/chromium/base/LocaleUtils.java:22: return locale.getLanguage() + "-" + locale.getCountry(); Xianzhu wrote: > On 2012/04/25 20:00:20, Mark Mentovai wrote: > > Is this always right? Will there never be a case where we intend something > like > > “en” without a country? Languages like “ru”? What about cases like “es-419”, > > does this handle those? Should it? > > I think it should handle locales without country. "es-419" should be handled > automatically if Java Locale returns language "es" and country "419". How ’bout things like “ru”? Chrome calls the locale for that “ru”, no dashes or countries or anything. Will returning something else here (I’m not sure what it’ll return, actually) cause a problem?
http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... File chrome/browser/autofill/autofill_country.cc (right): http://codereview.chromium.org/10224004/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_country.cc:497: locale, false); On 2012/04/25 20:10:36, Ilya Sherman wrote: > nit: I don't fully follow the significance of the last parameter, but the > localized country names are indeed used in the UI -- though I don't think > there's ever any other text adjacent to them. So, I'm not sure what the value > of this last param ought to be; wanted to make sure you'd considered it and > chosen appropriately, rather than just defaulting to |false|. Just noticed that the is_for_ui parameter of the original GetDisplayNameForLocale() is for correct RTL placing of parenthesis. As we don't have parenthesis in country display names, I removed the parameter in Patch Set 3.
http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... File base/android/java/org/chromium/base/LocaleUtils.java (right): http://codereview.chromium.org/10224004/diff/1/base/android/java/org/chromium... base/android/java/org/chromium/base/LocaleUtils.java:22: return locale.getLanguage() + "-" + locale.getCountry(); On 2012/04/25 22:38:51, Mark Mentovai wrote: > Xianzhu wrote: > > On 2012/04/25 20:00:20, Mark Mentovai wrote: > > > Is this always right? Will there never be a case where we intend something > > like > > > “en” without a country? Languages like “ru”? What about cases like “es-419”, > > > does this handle those? Should it? > > > > I think it should handle locales without country. "es-419" should be handled > > automatically if Java Locale returns language "es" and country "419". > > How ’bout things like “ru”? Chrome calls the locale for that “ru”, no dashes or > countries or anything. Will returning something else here (I’m not sure what > it’ll return, actually) cause a problem? Sorry I forgot to mention I would make the change to handle "ru". The code Patch Set 1 will return "ru" in the case.
On 2012/04/25 22:53:15, Xianzhu wrote: > Sorry I forgot to mention I would make the change to handle "ru". The code Patch > Set 1 will return "ru" in the case. Sorry I meant Patch Set 2 :)
LGTM in base. I only reviewed in that directory.
On 2012/04/25 22:36:23, Xianzhu wrote: > http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc > File ui/base/l10n/l10n_util.cc (right): > > http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc#newc... > ui/base/l10n/l10n_util.cc:521: return GetDisplayNameForLocale("_" + > country_code, display_locale, is_for_ui); > On 2012/04/25 21:06:43, sky wrote: > > This hardly seems worth adding a new function for. Are you really using this > > enough to warrant that? > > Assuming you are, I would rather see a GetLocaleForCountryCode() so that > instead > > you do: > > > > GetDisplayNameForLocale(GetLocaleForCountryCode(), ...) > > > > That said, I don't know enough about locale->country code to say if this is > > right. > > Both ICU uloc_getDisplayName and base::android::GetDisplayNameForLocale() will > return the display name of the country for a locale string in format of '"_" + > country_code'. I think we can also make this rule exposed for > l10n_util::GetDisplayNameForLocale(), so that we can eliminate the new function. > > For GetLocaleForCountryCode(), I feel it a bit strange because "_CC" doesn't > look a valid locale to me. @sky, GetLocaleForCountryCode() seems not very feasible, and to the caller GetDisplayNameForCountry() seems clearer especially after the confusing is_for_ui parameter removed, so I kept it in Patch Set 3. Does this look good to you?
On Wed, Apr 25, 2012 at 3:36 PM, <wangxianzhu@chromium.org> wrote: > > http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc > File ui/base/l10n/l10n_util.cc (right): > > http://codereview.chromium.org/10224004/diff/1/ui/base/l10n/l10n_util.cc#newc... > ui/base/l10n/l10n_util.cc:521: return GetDisplayNameForLocale("_" + > country_code, display_locale, is_for_ui); > On 2012/04/25 21:06:43, sky wrote: >> >> This hardly seems worth adding a new function for. Are you really > > using this >> >> enough to warrant that? >> Assuming you are, I would rather see a GetLocaleForCountryCode() so > > that instead >> >> you do: > > >> GetDisplayNameForLocale(GetLocaleForCountryCode(), ...) > > >> That said, I don't know enough about locale->country code to say if > > this is >> >> right. > > > Both ICU uloc_getDisplayName and > base::android::GetDisplayNameForLocale() will return the display name of > the country for a locale string in format of '"_" + country_code'. I > think we can also make this rule exposed for > l10n_util::GetDisplayNameForLocale(), so that we can eliminate the new > function. > > For GetLocaleForCountryCode(), I feel it a bit strange because "_CC" > doesn't look a valid locale to me. I'm not sure I understand. I'm suggesting instead of adding GetDisplayNameForCountry you add GetLocaleForCountry. Usage of the function then becomes GetDisplayNameForCountry then becomes GetDisplayNameForLocale(GetDisplayNameForCountry() ...); -Scott
> I'm not sure I understand. > > I'm suggesting instead of adding GetDisplayNameForCountry you add > GetLocaleForCountry. Usage of the function then becomes > GetDisplayNameForCountry then becomes > GetDisplayNameForLocale(GetDisplayNameForCountry() ...); > > -Scott GetLocaleForCountry() would be like this: // Return a locale name with only the country part (like "_CC"). std::string GetLocaleForCountry(const std::string& country); It doesn't look good to me because the "_CC" format of locale doesn't look like a valid locale format. It's also not feasible to let GetLocaleForCountry() return the full locale because 1) country:locale is a 1:n mapping; 2) GetDisplayNameForLocale() will return result in format of "Language (Country)" for a full locale, while GetDisplayNameForCountry() is expected to return "Country" only. I meant the alternate way of "exposing the rule for l10n_util::GetDisplayNameForLocale()" to add following comments in current GetDisplayNameForLocale(): // If locale is missing language part (i.e. in the format of "_CC"), this function returns the display name of the country. However I think this is not as clear as adding l10n_util::GetDisplayNameForCountry(). In addition, with the added function, the caller doesn't need to consider the 'is_for_ui' parameter of GetDisplayNameForLocale() which is no-use in the case.
On Thu, Apr 26, 2012 at 9:49 AM, <wangxianzhu@chromium.org> wrote: >> I'm not sure I understand. > > >> I'm suggesting instead of adding GetDisplayNameForCountry you add >> GetLocaleForCountry. Usage of the function then becomes >> GetDisplayNameForCountry then becomes >> GetDisplayNameForLocale(GetDisplayNameForCountry() ...); > > >> -Scott > > > GetLocaleForCountry() would be like this: > > // Return a locale name with only the country part (like "_CC"). > std::string GetLocaleForCountry(const std::string& country); > > It doesn't look good to me because the "_CC" format of locale doesn't look > like > a valid locale format. > > It's also not feasible to let GetLocaleForCountry() return the full locale > because > 1) country:locale is a 1:n mapping; > 2) GetDisplayNameForLocale() will return result in format of "Language > (Country)" for a full locale, while GetDisplayNameForCountry() is expected > to > return "Country" only. Aren't you saying you can't convert a country code to a locale code? If so, how can GetDisplayNameForCountry work? But either way, I'm not really an expert here. Jungshik would be a better reviewer for this. -Scott > I meant the alternate way of "exposing the rule for > l10n_util::GetDisplayNameForLocale()" to add following comments in current > GetDisplayNameForLocale(): > > // If locale is missing language part (i.e. in the format of "_CC"), this > function returns the display name of the country. > > However I think this is not as clear as adding > l10n_util::GetDisplayNameForCountry(). In addition, with the added function, > the > caller doesn't need to consider the 'is_for_ui' parameter of > GetDisplayNameForLocale() which is no-use in the case.
On 2012/04/26 17:55:07, sky wrote: > On Thu, Apr 26, 2012 at 9:49 AM, <mailto:wangxianzhu@chromium.org> wrote: > >> I'm not sure I understand. > > > > > >> I'm suggesting instead of adding GetDisplayNameForCountry you add > >> GetLocaleForCountry. Usage of the function then becomes > >> GetDisplayNameForCountry then becomes > >> GetDisplayNameForLocale(GetDisplayNameForCountry() ...); > > > > > >> -Scott > > > > > > GetLocaleForCountry() would be like this: > > > > // Return a locale name with only the country part (like "_CC"). > > std::string GetLocaleForCountry(const std::string& country); > > > > It doesn't look good to me because the "_CC" format of locale doesn't look > > like > > a valid locale format. > > > > It's also not feasible to let GetLocaleForCountry() return the full locale > > because > > 1) country:locale is a 1:n mapping; > > 2) GetDisplayNameForLocale() will return result in format of "Language > > (Country)" for a full locale, while GetDisplayNameForCountry() is expected > > to > > return "Country" only. > > Aren't you saying you can't convert a country code to a locale code? > If so, how can GetDisplayNameForCountry work? GetDisplayNameForCountry works because the ICU implementation of GetDisplayNameForLocale just returns display name for country for incomplete locales like "_CC" containing only the country code. However, it seems not suitable to let GetLocaleForCountry as a public API return the incomplete locale like "_CC". > But either way, I'm not really an expert here. > Jungshik would be a better reviewer for this. Added Jungshik into ui/base/l10n/OWNERS. > > -Scott > > > I meant the alternate way of "exposing the rule for > > l10n_util::GetDisplayNameForLocale()" to add following comments in current > > GetDisplayNameForLocale(): > > > > // If locale is missing language part (i.e. in the format of "_CC"), this > > function returns the display name of the country. > > > > However I think this is not as clear as adding > > l10n_util::GetDisplayNameForCountry(). In addition, with the added function, > > the > > caller doesn't need to consider the 'is_for_ui' parameter of > > GetDisplayNameForLocale() which is no-use in the case.
The code change LGTM, but Jungshik may have some additional feedback. https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... File ui/base/l10n/l10n_util.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... ui/base/l10n/l10n_util.cc:494: // zh-Hant because Java API doesn't support scripts. Does the java api support pt-br vs pt-pt? What about es-419 vs just es?
https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... File ui/base/l10n/l10n_util.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... ui/base/l10n/l10n_util.cc:494: // zh-Hant because Java API doesn't support scripts. On 2012/04/27 20:29:27, tony wrote: > Does the java api support pt-br vs pt-pt? What about es-419 vs just es? Yes, Java API support these locales, with the support of its backend ICU. zh-Hans and zh-Hant are not supported because 'Hans' and 'Hant' are not countries but scripts. Java API doesn't support locale scripts.
On 2012/04/27 20:47:25, Xianzhu wrote: > https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... > File ui/base/l10n/l10n_util.cc (right): > > https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... > ui/base/l10n/l10n_util.cc:494: // zh-Hant because Java API doesn't support > scripts. > On 2012/04/27 20:29:27, tony wrote: > > Does the java api support pt-br vs pt-pt? What about es-419 vs just es? > > Yes, Java API support these locales, with the support of its backend ICU. > > zh-Hans and zh-Hant are not supported because 'Hans' and 'Hant' are not > countries but scripts. Java API doesn't support locale scripts. Would zh-TW and zh-CN work on Android? Maybe you should do that instead?
On 2012/04/27 22:39:40, tony wrote: > On 2012/04/27 20:47:25, Xianzhu wrote: > > > https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... > > File ui/base/l10n/l10n_util.cc (right): > > > > > https://chromiumcodereview.appspot.com/10224004/diff/17001/ui/base/l10n/l10n_... > > ui/base/l10n/l10n_util.cc:494: // zh-Hant because Java API doesn't support > > scripts. > > On 2012/04/27 20:29:27, tony wrote: > > > Does the java api support pt-br vs pt-pt? What about es-419 vs just es? > > > > Yes, Java API support these locales, with the support of its backend ICU. > > > > zh-Hans and zh-Hant are not supported because 'Hans' and 'Hant' are not > > countries but scripts. Java API doesn't support locale scripts. > > Would zh-TW and zh-CN work on Android? Maybe you should do that instead? We intentionally use zh-Hans and zh-Hant to get display names of zh-TW and zh-CN. Here are the comments in GetDisplayNameForLocale(): // Internally, we use the language code of zh-CN and zh-TW, but we want the // display names to be Chinese (Simplified) and Chinese (Traditional) instead // of Chinese (China) and Chinese (Taiwan). To do that, we pass zh-Hans // and zh-Hant to ICU. Even with this mapping, we'd get // 'Chinese (Simplified Han)' and 'Chinese (Traditional Han)' in English and // even longer results in other languages. Arguably, they're better than // the current results : Chinese (China) / Chinese (Taiwan).
On 2012/04/27 23:00:55, Xianzhu wrote: > We intentionally use zh-Hans and zh-Hant to get display names of zh-TW and > zh-CN. Here are the comments in GetDisplayNameForLocale(): Ah, right. Thanks for clarifying.
https://chromiumcodereview.appspot.com/10224004/diff/22001/base/android/local... File base/android/locale_utils.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/22001/base/android/local... base/android/locale_utils.cc:61: ConvertUTF8ToJavaString(env, variant).obj())); Does "Java on Android" support Java 7 locale class? In that case, you'd better use a factory method forLanguageTag? That way, you can handle 'script' tag correctly. http://docs.oracle.com/javase/7/docs/api/java/util/Locale.html#forLanguageTag... An alternative is use a new class LocaleBuilder ( http://docs.oracle.com/javase/7/docs/api/java/util/Locale.Builder.html ). It's also new in Java 7. So, if neither is supported yet on Android, I guess you're stuck with what you have now. In that case, a comment (TODO) would be nice to put in here. https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... File ui/base/l10n/l10n_util.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... ui/base/l10n/l10n_util.cc:520: return GetDisplayNameForLocale("_" + country_code, display_locale, false); I would use "und_" (for unknown language) instead of "_" here.
https://chromiumcodereview.appspot.com/10224004/diff/22001/base/android/local... File base/android/locale_utils.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/22001/base/android/local... base/android/locale_utils.cc:61: ConvertUTF8ToJavaString(env, variant).obj())); On 2012/05/04 17:46:03, Jungshik Shin wrote: > Does "Java on Android" support Java 7 locale class? In that case, you'd better > use a factory method forLanguageTag? That way, you can handle 'script' tag > correctly. > > http://docs.oracle.com/javase/7/docs/api/java/util/Locale.html#forLanguageTag...) > > An alternative is use a new class LocaleBuilder ( > http://docs.oracle.com/javase/7/docs/api/java/util/Locale.Builder.html ). It's > also new in Java 7. > > So, if neither is supported yet on Android, I guess you're stuck with what you > have now. In that case, a comment (TODO) would be nice to put in here. > Thanks for the information. They are very useful. Just verified that the Java 7 Locale API is not yet supported on Android, so I will add a TODO. https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... File ui/base/l10n/l10n_util.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... ui/base/l10n/l10n_util.cc:520: return GetDisplayNameForLocale("_" + country_code, display_locale, false); On 2012/05/04 17:46:03, Jungshik Shin wrote: > I would use "und_" (for unknown language) instead of "_" here. > Just tried using "und_", but the returned display name was like "Unknown Language (Country)" instead of "Country".
LGTM https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... File ui/base/l10n/l10n_util.cc (right): https://chromiumcodereview.appspot.com/10224004/diff/22001/ui/base/l10n/l10n_... ui/base/l10n/l10n_util.cc:520: return GetDisplayNameForLocale("_" + country_code, display_locale, false); On 2012/05/04 18:13:42, Xianzhu Wang wrote: > On 2012/05/04 17:46:03, Jungshik Shin wrote: > > I would use "und_" (for unknown language) instead of "_" here. > > > > Just tried using "und_", but the returned display name was like "Unknown > Language (Country)" instead of "Country". Thanks for trying out and sorry for wasting your time. I realized that getDisplayName both in C(++) and Java do the right thing with your original input ("_US", "_CN", etc).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/29002
Can't process patch for file ui/base/l10n/OWNERS. Hunk header is incorrect: 1 vs 2
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/26004
Can't process patch for file ui/base/l10n/OWNERS. Hunk header is incorrect: 1 vs 2
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/26004
Can't process patch for file ui/base/l10n/OWNERS. Hunk header is incorrect: 1 vs 2
On 2012/05/04 18:47:13, I haz the power (commit-bot) wrote: > Can't process patch for file ui/base/l10n/OWNERS. > Hunk header is incorrect: 1 vs 2 It it's helpful, just remove OWNERS from your change and let's land that change separately.
Seems a bug of CQ script? The "hunk header" of the OWNERS file is "@@ -1 +1,2 @@" which seems not recognized by patch.py. On Fri, May 4, 2012 at 8:50 PM, <tony@chromium.org> wrote: > On 2012/05/04 18:47:13, I haz the power (commit-bot) wrote: > >> Can't process patch for file ui/base/l10n/OWNERS. >> Hunk header is incorrect: 1 vs 2 >> > > It it's helpful, just remove OWNERS from your change and let's land that > change > separately. > > http://codereview.chromium.**org/10224004/<http://codereview.chromium.org/102... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/34002
Presubmit check for 10224004-34002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ui
Presubmit checks took 4.8s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/34002
Try job failure for 10224004-34002 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10224004/34005
Change committed as 135484 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
