[LanguageTag] Migrate l10n_util::GetLanguage callsites in Ash/ChromeOS [chromium/src : main]
Danilo Tedeschi (Gerrit)
Danilo Tedeschi has uploaded the change for review![]( "Open in Gerrit")
Commit message
[LanguageTag] Migrate l10n_util::GetLanguage callsites in Ash/ChromeOS
This CL replaces usages of the legacy `l10n_util::GetLanguage` function
in Ash and ChromeOS subsystems with the type-safe
`base::i18n::GetLanguageTagFromString` API.
This is part of a larger effort to deprecate and remove
`l10n_util::GetLanguage` across the codebase.
Change diff
Change information
- M ash/system/locale/locale_detailed_view.cc
- M ash/system/locale/locale_feature_pod_controller.cc
- M ash/system/unified/current_locale_view.cc
- M chrome/browser/ash/accessibility/live_caption/system_live_caption_service.cc
- M chrome/browser/ash/locale/locale_change_guard.cc
- M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.cc
- M chrome/browser/ash/policy/core/device_local_account_browsertest.cc
- M chrome/browser/ui/ash/quick_answers/ui/quick_answers_util.cc
- M chrome/browser/ui/ash/wm/coral_delegate_impl.cc
- M chrome/browser/ui/webui/ash/login/l10n_util.cc
- M chrome/browser/ui/webui/ash/settings/pages/a11y/select_to_speak_handler.cc
- M chrome/browser/ui/webui/ash/settings/pages/a11y/tts_handler.cc
- M chromeos/components/quick_answers/public/cpp/quick_answers_state.cc
- M chromeos/components/quick_answers/understanding/intent_generator.cc
- M chromeos/components/quick_answers/utils/language_detector.cc
- M chromeos/components/quick_answers/utils/spell_checker.cc
- M chromeos/components/quick_answers/utils/translation_v2_utils.cc
- M third_party/icu
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 8 comments![]( "Open in Gerrit")
#include "ui/base/l10n/l10n_util.h"Is this include still needed?
.value_or(base::i18n::GetKnownLanguageTag("und"))It is possible to use a named constant for "und"? I had to do a bunch of searching to figure out it meant "undetermined". Also, GetKnownLanguageTag() does not have a function comment, which doesn't help.
.value_or(base::i18n::GetKnownLanguageTag("und"))ditto. Or maybe you could have a constant for the undetermined language tag and use that directly.
#include "ui/base/l10n/l10n_util.h"ditto and throughout CL
base::i18n::GetLanguageTagFromString(target_language)Pull these into local variables, this if() condition is too long.
.language_subtag() == detected_language) {After seeing this code repeated many times, what do you think about adding a helper function in base::i18n to extract the language subtag? It could be called GetLanguage or GetLanguageSubtag. Doing this would probably make all these call sites more readable.
COMPONENT_EXPORT(UI_BASE)Please add a comment for this function. For example, is it preferred to use this method over the one above?
Related details
- Ashley Prasad
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 8 comments![]( "Open in Gerrit")
Is this include still needed?
Done
It is possible to use a named constant for "und"? I had to do a bunch of searching to figure out it meant "undetermined". Also, GetKnownLanguageTag() does not have a function comment, which doesn't help.
stoped using "und".
#include "ui/base/l10n/l10n_util.h"Danilo Tedeschiditto
Done
ditto. Or maybe you could have a constant for the undetermined language tag and use that directly.
ack.
#include "ui/base/l10n/l10n_util.h"Danilo Tedeschiditto and throughout CL
I was not able to remove it.
Pull these into local variables, this if() condition is too long.
Done
After seeing this code repeated many times, what do you think about adding a helper function in base::i18n to extract the language subtag? It could be called GetLanguage or GetLanguageSubtag. Doing this would probably make all these call sites more readable.
the code is long because the outer layer is not migrated to use LanguageTag yet. I prefer leaving the more complex code in as a way to force us a bubble up migration of LanguageTag that will make the code cleaner later on. However, if you prefer, I can add a temporary function that takes in a string and returns a language subtag. WDYT?
Please add a comment for this function. For example, is it preferred to use this method over the one above?
- Ashley Prasad
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 4 comments![]( "Open in Gerrit")
.value_or(base::i18n::GetKnownLanguageTag("und"))Danilo TedeschiIt is possible to use a named constant for "und"? I had to do a bunch of searching to figure out it meant "undetermined". Also, GetKnownLanguageTag() does not have a function comment, which doesn't help.
stoped using "und".
Really? I still see it in the latest patchset.
#include "ui/base/l10n/l10n_util.h"Danilo Tedeschiditto and throughout CL
I was not able to remove it.
Just here, or every instance in all the other files you touched?
I'll bet at least some of them aren't needed anymore.
.value_or(base::i18n::GetKnownLanguageTag("und"));Like here.
.language_subtag() == detected_language) {Danilo TedeschiAfter seeing this code repeated many times, what do you think about adding a helper function in base::i18n to extract the language subtag? It could be called GetLanguage or GetLanguageSubtag. Doing this would probably make all these call sites more readable.
the code is long because the outer layer is not migrated to use LanguageTag yet. I prefer leaving the more complex code in as a way to force us a bubble up migration of LanguageTag that will make the code cleaner later on. However, if you prefer, I can add a temporary function that takes in a string and returns a language subtag. WDYT?
Yes, I'd prefer a function that takes a string and returns a subtag. Thanks.
Related details
- Ashley Prasad
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ashley Prasad (Gerrit)
Ashley Prasad added 20 comments![]( "Open in Gerrit")
Bug: Noneoptional nit: add `Test: <tests run>`.
if (from_tag && to_tag) {Please fix this WARNING reported by autoreview issue finding: If one of the locales is invalid (causing `from_tag` or `to_tag` to be `std::nullopt`), this will skip to `return false;` instead of evaluating differences.
In the original code, `l10n_util::GetLanguage` would return strings for both, and if one was valid (e.g. `en`) and the other was invalid (e.g. `xyz`), they would be evaluated as different and return `true`. This could cause the notification to be silently ignored if one of the locales is malformed.
@ashleydp - It looks like the logic here is a bit different than the original code, I think it was originally check if they did not match and returning true. Now it appears that if either are not set we're returning false. Is this intentional?
if (from_tag && to_tag) {nit: can we invert this check to return early in the false condition instead of having the nested if?
std::string(base::i18n::GetLanguageTagFromString(
g_browser_process->GetApplicationLocale())
.transform(&base::i18n::LanguageTag::language_subtag)
.value_or(std::string_view())));qq: It looks like we're using the same general logic in several files, is this something we can put in a helper function?
.value();Please fix this WARNING reported by autoreview issue finding: Calling `.value()` on `std::optional` without checking if it contains a value will crash the test runner with a hard-to-read `std::bad_optional_access` exception if `GetApplicationLocale()` returns something unparseable.
Consider checking `.has_value()` or using `CHECK` for a better error message if this is guaranteed to succeed.
@ashleydp - optional nit: Seems unlikely to occur but it may be beneficial to have cleaner error messaging on the off chance it does.
EXPECT_EQ(GetLanguage(locales[0]), icu::Locale::getDefault().getLanguage());qq: The CL description mentions removing `GetLanguage`, was this call left intentionally or am I missing a local test function this is mapped to?
EXPECT_EQ(GetLanguage(kPublicSessionLocale),Same question as line 756.
EXPECT_EQ(GetLanguage(kSingleRecommendedLocale[0]),Same question as line 756.
EXPECT_EQ(GetLanguage(kPublicSessionLocale),Same question as line 756.
EXPECT_EQ(GetLanguage(kRecommendedLocales1[0]),Same question as line 756.
EXPECT_EQ(GetLanguage(kPublicSessionLocale),Same question as line 756.
EXPECT_EQ(GetLanguage(kPublicSessionLocale),Same question as line 756.
EXPECT_EQ(GetLanguage(kPublicSessionLocale),Same question as line 756.
base::i18n::GetKnownLanguageTag("und"));nit: replace with named constant.
void QuickAnswersState::SetPreferredLanguages(qq: is this only used in tests?
base::i18n::LanguageTag device_language =
QuickAnswersState::Get()->application_locale().WithLanguageSubtagOnly();qq: can this be simplified to `std::string_view device_language = QuickAnswersState::Get()->application_locale().language_subtag();`?
if (QuickAnswersState::Get()->application_locale().tag_string() == "und" ||nit: use constant. Maybe adding constexpr for this would suffice.
base::i18n::LanguageTag device_language =
QuickAnswersState::Get()->application_locale().WithLanguageSubtagOnly();
std::optional<base::i18n::LanguageTag> detected_language =
detected_locale
.and_then([](const std::string& locale) {
return base::i18n::GetLanguageTagFromString(locale);
})
.transform(&base::i18n::LanguageTag::WithLanguageSubtagOnly);qq: I feel like I'm missing context here, it looks like we're taking strings making them into language tags and then making them back into strings. Is this adding some additional type checking which is why we're doing these transforms?
if (detected_language && device_language.tag_string() != "und" &&nit: same as line 340
if (app_locale.tag_string() != "und") {nit: use a constant.
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi voted and added 25 comments![]( "Open in Gerrit")
Votes added by Danilo Tedeschi
| Commit-Queue | +1 |
25 comments
sorry for the large change. I will try to split some stuff out of it.
optional nit: add `Test: <tests run>`.
Done
.value_or(base::i18n::GetKnownLanguageTag("und"))Danilo TedeschiIt is possible to use a named constant for "und"? I had to do a bunch of searching to figure out it meant "undetermined". Also, GetKnownLanguageTag() does not have a function comment, which doesn't help.
James Cookstoped using "und".
Really? I still see it in the latest patchset.
Done
#include "ui/base/l10n/l10n_util.h"Danilo Tedeschiditto and throughout CL
James CookI was not able to remove it.
Just here, or every instance in all the other files you touched?
I'll bet at least some of them aren't needed anymore.
Done
nit: can we invert this check to return early in the false condition instead of having the nested if?
Done
Please fix this WARNING reported by autoreview issue finding: If one of the locales is invalid (causing `from_tag` or `to_tag` to be `std::nullopt`), this will skip to `return false;` instead of evaluating differences.
In the original code, `l10n_util::GetLanguage` would return strings for both, and if one was valid (e.g. `en`) and the other was invalid (e.g. `xyz`), they would be evaluated as different and return `true`. This could cause the notification to be silently ignored if one of the locales is malformed.
@ashleydp - It looks like the logic here is a bit different than the original code, I think it was originally check if they did not match and returning true. Now it appears that if either are not set we're returning false. Is this intentional?
Done
std::string(base::i18n::GetLanguageTagFromString(
g_browser_process->GetApplicationLocale())
.transform(&base::i18n::LanguageTag::language_subtag)
.value_or(std::string_view())));qq: It looks like we're using the same general logic in several files, is this something we can put in a helper function?
Done
.value_or(base::i18n::GetKnownLanguageTag("und"));Danilo TedeschiLike here.
Done
Please fix this WARNING reported by autoreview issue finding: Calling `.value()` on `std::optional` without checking if it contains a value will crash the test runner with a hard-to-read `std::bad_optional_access` exception if `GetApplicationLocale()` returns something unparseable.
Consider checking `.has_value()` or using `CHECK` for a better error message if this is guaranteed to succeed.
@ashleydp - optional nit: Seems unlikely to occur but it may be beneficial to have cleaner error messaging on the off chance it does.
Done
EXPECT_EQ(GetLanguage(locales[0]), icu::Locale::getDefault().getLanguage());qq: The CL description mentions removing `GetLanguage`, was this call left intentionally or am I missing a local test function this is mapped to?
Done
Same question as line 756.
Done
Same question as line 756.
Done
Same question as line 756.
Done
Same question as line 756.
Done
Same question as line 756.
Done
Same question as line 756.
Done
Same question as line 756.
Done
nit: replace with named constant.
Acknowledged
qq: is this only used in tests?
removed it.
.language_subtag() == detected_language) {Danilo TedeschiAfter seeing this code repeated many times, what do you think about adding a helper function in base::i18n to extract the language subtag? It could be called GetLanguage or GetLanguageSubtag. Doing this would probably make all these call sites more readable.
James Cookthe code is long because the outer layer is not migrated to use LanguageTag yet. I prefer leaving the more complex code in as a way to force us a bubble up migration of LanguageTag that will make the code cleaner later on. However, if you prefer, I can add a temporary function that takes in a string and returns a language subtag. WDYT?
Yes, I'd prefer a function that takes a string and returns a subtag. Thanks.
Done
base::i18n::LanguageTag device_language =
QuickAnswersState::Get()->application_locale().WithLanguageSubtagOnly();qq: can this be simplified to `std::string_view device_language = QuickAnswersState::Get()->application_locale().language_subtag();`?
Done
if (QuickAnswersState::Get()->application_locale().tag_string() == "und" ||nit: use constant. Maybe adding constexpr for this would suffice.
Done
base::i18n::LanguageTag device_language =
QuickAnswersState::Get()->application_locale().WithLanguageSubtagOnly();
std::optional<base::i18n::LanguageTag> detected_language =
detected_locale
.and_then([](const std::string& locale) {
return base::i18n::GetLanguageTagFromString(locale);
})
.transform(&base::i18n::LanguageTag::WithLanguageSubtagOnly);qq: I feel like I'm missing context here, it looks like we're taking strings making them into language tags and then making them back into strings. Is this adding some additional type checking which is why we're doing these transforms?
unfortunately, migrating everything to LanguageTag would be a huge change, it is easier to do it step by step. I could do a massive migration here, but I feel like it would not easy to review it.
if (detected_language && device_language.tag_string() != "und" &&nit: same as line 340
Done
if (app_locale.tag_string() != "und") {Danilo Tedeschinit: use a constant.
Done
Related details
- Ashley Prasad
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
sorry for the large change. I will try to split some stuff out of it.
OK. I'll let you split it up first, then I'll review the pieces. Let me know when you want me to look at it.
Related details
- Ashley Prasad
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 1 comment![]( "Open in Gerrit")
James Cooksorry for the large change. I will try to split some stuff out of it.
OK. I'll let you split it up first, then I'll review the pieces. Let me know when you want me to look at it.
- Ashley Prasad
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
1 comment
LGTM for ash. Thanks for splitting it up, this is much more manageable.
Related details
- Ashley Prasad
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ashley Prasad (Gerrit)
Ashley Prasad added 2 comments![]( "Open in Gerrit")
lgtm % AI warning
language_code = base::i18n::GetLanguageSubtagUsingLanguageTag(Please fix this WARNING reported by autoreview issue finding: In the original code, `std::replace` mutated `language_and_country_code` in-place, changing underscores to hyphens. As a result, the hyphenated version was passed to `l10n_util::GetDisplayNameForLocale` around line 64.
By removing this explicit normalization without replacing it, `language_and_country_code` retains its underscores, meaning `l10n_util::GetDisplayNameForLocale` will now receive the raw string (potentially containing underscores if enhanced voices are used). This could lead to incorrect locale names being displayed if `GetDisplayNameForLocale` depends on hyphen separators.
Additionally, if `base::i18n::GetLanguageSubtagUsingLanguageTag` strictly expects BCP 47 compliant tags (which use hyphens), removing this normalization might also break the subtag extraction itself.
Was removing this explicit normalization intended? If not, it is safer to retain the hyphen replacement before extracting the subtag or keep it explicitly for downstream usages.
@ashleydp - I took a quick look at GetDisplayNameForLocale and it does some comparison with values containing "-", so we may want to keep that replace. Is there a test covering this? If there's no test lets try to add one.
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 1 comment![]( "Open in Gerrit")
language_code = base::i18n::GetLanguageSubtagUsingLanguageTag(Please fix this WARNING reported by autoreview issue finding: In the original code, `std::replace` mutated `language_and_country_code` in-place, changing underscores to hyphens. As a result, the hyphenated version was passed to `l10n_util::GetDisplayNameForLocale` around line 64.
By removing this explicit normalization without replacing it, `language_and_country_code` retains its underscores, meaning `l10n_util::GetDisplayNameForLocale` will now receive the raw string (potentially containing underscores if enhanced voices are used). This could lead to incorrect locale names being displayed if `GetDisplayNameForLocale` depends on hyphen separators.
Additionally, if `base::i18n::GetLanguageSubtagUsingLanguageTag` strictly expects BCP 47 compliant tags (which use hyphens), removing this normalization might also break the subtag extraction itself.
Was removing this explicit normalization intended? If not, it is safer to retain the hyphen replacement before extracting the subtag or keep it explicitly for downstream usages.
@ashleydp - I took a quick look at GetDisplayNameForLocale and it does some comparison with values containing "-", so we may want to keep that replace. Is there a test covering this? If there's no test lets try to add one.
LanguageTag supports both formats: the old POSIX/ICU style (using underscores) and the bcp47 one (the one that uses hyphens).
Related details
- Ashley Prasad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ashley Prasad (Gerrit)
Ashley Prasad voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ashley Prasad
| Code-Review | +1 |
1 comment
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[LanguageTag] Migrate l10n_util::GetLanguage callsites in Ash/ChromeOS
This CL replaces legacy, raw-string `l10n_util::GetLanguage` callsites
in Ash, ChromeOS, and Ash-specific Chrome browser components with the
newly introduced type-safe
`base::i18n::GetLanguageSubtagUsingLanguageTag` helper, preparing for
the removal of the deprecated raw-string function from ui/base.
Technical Details:
- Replaced raw-string language extraction with `base::i18n::GetLanguageSubtagUsingLanguageTag`
across Ash system locale views, pod controllers, Quick Answers utilities,
intent generators, spell checkers, language detectors, and device local account
browser tests.
- Included "base/i18n/legacy_language_tag_helpers.h" at each modified callsite.
- M ash/system/locale/locale_detailed_view.cc
- M ash/system/locale/locale_feature_pod_controller.cc
- M ash/system/unified/current_locale_view.cc
- M chrome/browser/ash/accessibility/live_caption/system_live_caption_service.cc
- M chrome/browser/ash/locale/locale_change_guard.cc
- M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.cc
- M chrome/browser/ash/policy/core/device_local_account_browsertest.cc
- M chrome/browser/ui/ash/quick_answers/ui/quick_answers_util.cc
- M chrome/browser/ui/ash/wm/coral_delegate_impl.cc
- M chrome/browser/ui/webui/ash/login/l10n_util.cc
- M chrome/browser/ui/webui/ash/settings/pages/a11y/select_to_speak_handler.cc
- M chrome/browser/ui/webui/ash/settings/pages/a11y/tts_handler.cc
- M chromeos/components/quick_answers/public/cpp/quick_answers_state.cc
- M chromeos/components/quick_answers/understanding/intent_generator.cc
- M chromeos/components/quick_answers/utils/language_detector.cc
- M chromeos/components/quick_answers/utils/spell_checker.cc
- M chromeos/components/quick_answers/utils/translation_v2_utils.cc
Code-Review: +1 by James Cook, +1 by Ashley Prasad
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |