[PWA] Add name localization fields for install info during parsing [chromium/src : main]
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 1 comment![]( "Open in Gerrit")
Hey folks, this is the first part of adding support for localized names.
I know we settled on adding "lang" and "dir" to the Web App Install Info class, then in the next CLs using a combination of "title", "lang", and "dir" to build the localized name triplet to save in the WebApp class and proto.
I realized that that would work until I would have to implement localized descriptions as well, since the "lang" and "dir" associated with one key for "name_localized" would not necessarily be the same for the same key for "description_localized".
If we go this route, we'll have to keep adding distinctive InstallInfo fields for each localizable value as we add them, like "title_lang" and "title_dir" like I did for this CL.
Let me know if this is an issue. The other option we discussed would be refactoring each localizable value into a tuple model class.
Related details
- Daniel Murphy
- Dibyajyoti Pal
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 8 comments![]( "Open in Gerrit")
const blink::mojom::ManifestLocalizedTextObject* MatchLocalizedText(Can you share more details about what this function is doing? It seems like we're trying to find if `application_locale` is in the map, and if it isn't, then we try to get the language of the locale, and then try to find it again?
Imo that seems flaky, as we can't know the key of the map just from reading the code, as the key might be a language tag or a whole locale. Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Hmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
void SetTitleFromNonLocalizedManifestFields(
const blink::mojom::Manifest& manifest,
WebAppInstallInfo& info) {
std::u16string name = manifest.name.value_or(std::u16string());
if (!name.empty()) {
info.title = name;
} else if (manifest.short_name) {
info.title = *manifest.short_name;
}
}This file has a lot of legacy code where we update the object in place, but that should not be done. This is a relic of the past, and could be a good code health task for the future for us to not be doing this anymore.
Ideally, we should be returning the output instead of updating an object in the function parameters (which in this case is an `output parameter`). See sentence 2 of [1], which says "Prefer using return values instead of output parameters".
So this would have a return value instead of a function parameter that is updated:
```
std::u16string GetNonLocalizedNameForManifest(const blink::mojom::Manifest& manifest) {
std::u16string name = manifest.name.value_or(std::u16string());
if(name.empty()) {
CHECK(!manifest.short_name.empty());
name = *manifest.short_name;
}
return name;
}
```
and then in the callsites, we can have:
```
install_info.title = GetNonLocalizedNameForManifest(manifest);
```
Wdyt?
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {There is a lot of indentation here that makes it a bit harder to parse. Can we help simplify this area?
So maybe:
```
using LocalizedTitleLang = std::optional<std::u16string>;
using LocalizedDir = std::optional<blink::mojom::Manifest_TextDirection>;
using LocalizedInfo = std::tuple<std::u16string, LocalizedTitleLang,
LocalizedDir>;
LocalizedInfo GetLocalizedNameFromManifest(const Manifest& manifest) {
// Parse manifest and construct a LocalizedInfo.
// If localized fields don't exist, only populated the first tuple field, and return std::nullopt for the other ones.
};// Inside this function:
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
LocalizedInfo localized_name = GetLocalizedNameFromManifest(manifest);
install_info().title = std::get<std::u16string>(localized_name);
install_info().title_lang = std::get<LocalizedTitleLang>(localized_name);
install_info().title_dir = std::get< LocalizedDir >(localized_name);
} else {
install_info().title = GetNonLocalizedNameForManifest(manifest);
}
```
Wdyt about this approach?
localized_name = MatchLocalizedText(manifest_->short_name_localized,Hmmm, I wonder if this will be a lot of work converting the "string" key to an `icu::Locale`. Using a class like `icu::Locale` ensures data correctness, and prevents bugs where we might end up setting malformed keys in the map. Can we do that here?
I guess we can KINDA assume that the string is correct here, otherwise the manifest parser would have errored [1], but I would prefer to be extra careful here still. Let's convert the keys to an `icu::Locale`.
if (localized_name->lang) {
install_info().title_lang = localized_name->lang;
}Same feedback for the `dir` field as well.
If `localized_name->lang` and `localized_name->dir` are std::optionals, then we can just do:
```
install_info().title = localized_name->value;
install_info().title_lang = localized_name->lang;
install_info().title_dir= localized_name->dir;
```
That way, we reduce indentation here and improve readability.
void AddLocalizedText(Please return a `base::flat_map<std::u16string, blink::mojom::ManifestLocalizedTextObjectPtr>` field instead of updating in place as per [1].
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
AddLocalizedText(manifest->name_localized, "en-US", u"American English Name",This is a very good example of why using `icu::Locale` on production code would be a good idea. We pass in a string here, and malformed string could lead to problems where the Locale is incorrect. We wouldn't catch it normally because the manifest we use here doesn't go through the `ManifestParser` as this is an unit-test, but with the `icu::Locale` suggestion, we could test that use-case here if we want!
Related details
- Alexander Kyereboah
- Daniel Murphy
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Hey folks, this is the first part of adding support for localized names.
I know we settled on adding "lang" and "dir" to the Web App Install Info class, then in the next CLs using a combination of "title", "lang", and "dir" to build the localized name triplet to save in the WebApp class and proto.
I realized that that would work until I would have to implement localized descriptions as well, since the "lang" and "dir" associated with one key for "name_localized" would not necessarily be the same for the same key for "description_localized".
If we go this route, we'll have to keep adding distinctive InstallInfo fields for each localizable value as we add them, like "title_lang" and "title_dir" like I did for this CL.
Let me know if this is an issue. The other option we discussed would be refactoring each localizable value into a tuple model class.
The tuple model class (or struct) kinda seems like a good idea imo? We could have a separate model in [1] that is:
```
struct LocalizedString {
std::u16string value;
std::optional<std::u16string> lang;
std::optional<blink::mojom::Manifest_TextDirection> dir;
};
```
And then in the WebAppInstallInfo, we can use:
```
LocalizedString title_localized;
LocalizedString description_localized;
```
The only hard part would be what to do for callsites of `WebAppInstallInfo::title` [2]. My suggestion would be to add a `title()` getter that returns `title_localized.value` if it's populated, or `title` if it's not, and update all the callsites to use that getter :shrug:. If you can come up with better options, that would work too!
[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/model/
[2] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_install_info.h;bpv=1;bpt=1?q=WebAppInstallinfo
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
field[base::UTF8ToUTF16(locale)] = std::move(localized_text);To fix the test failures, you might need to include `base/strings/utf_string_conversions.h` here.
Robert Paveza (Gerrit)
Robert Paveza added 1 comment![]( "Open in Gerrit")
Dibyajyoti PalHey folks, this is the first part of adding support for localized names.
I know we settled on adding "lang" and "dir" to the Web App Install Info class, then in the next CLs using a combination of "title", "lang", and "dir" to build the localized name triplet to save in the WebApp class and proto.
I realized that that would work until I would have to implement localized descriptions as well, since the "lang" and "dir" associated with one key for "name_localized" would not necessarily be the same for the same key for "description_localized".
If we go this route, we'll have to keep adding distinctive InstallInfo fields for each localizable value as we add them, like "title_lang" and "title_dir" like I did for this CL.
Let me know if this is an issue. The other option we discussed would be refactoring each localizable value into a tuple model class.
The tuple model class (or struct) kinda seems like a good idea imo? We could have a separate model in [1] that is:
```
struct LocalizedString {
std::u16string value;
std::optional<std::u16string> lang;
std::optional<blink::mojom::Manifest_TextDirection> dir;
};
```And then in the WebAppInstallInfo, we can use:
```
LocalizedString title_localized;
LocalizedString description_localized;
```The only hard part would be what to do for callsites of `WebAppInstallInfo::title` [2]. My suggestion would be to add a `title()` getter that returns `title_localized.value` if it's populated, or `title` if it's not, and update all the callsites to use that getter :shrug:. If you can come up with better options, that would work too!
[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/model/
[2] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_install_info.h;bpv=1;bpt=1?q=WebAppInstallinfo
I'm sorry for the drive-by, but maybe it'd be preferable to just store the localized data-list as a blob and deserialize it and/or recalculate it at the triggered-update time?
The problem is that the list of languages is unbounded. In a typical relational database, you'd store this as a foreign key, e.g.
```sql
CREATE TABLE app (
id INT PRIMARY KEY,
...
)
CREATE TABLE localized_titles (
app_id INT PRIMARY KEY,
language_key VARCHAR(10) PRIMARY KEY,
...
CONSTRAINT app_id FOREIGN KEY app.id
)
```
But I don't think we have a reasonable way to do this with what we're working with. A document database-like analogy here might be more reasonable, especially since we can just throw the entire set of localized strings into a single location and parse it on demand?
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
The localized information passed from the manifest will be stored as [protocol buffer messages](https://protobuf.dev/programming-guides/proto2/), since that is how we store the [web app on disk](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/proto/web_app.proto) anyway, so we can serialize and desrialize on Chrome startup. This is kind of the trusted and chosen option for data storage, instead of blobs or other options like JSON.
Can you share why we would need to store the entire set of localized strings into a single location? The design ensures that we only store info for a single locale at any time, so that if the locale does change and the manifest has an entry for that locale, we can always fetch the metadata and update it instead of keeping a list on memory/disk. During updates, the locale Chrome is running with is available from the [browser process](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_process.h;l=221;bpv=1;bpt=1?q=browser_process&ss=chromium%2Fchromium%2Fsrc), so we can use that to make decisions, right?
base::UTF8ToUTF16(g_browser_process->GetApplicationLocale());I just saw that we should be using `g_browser_process->GetFeatures()->application_locale_storage()->Get()` instead of `g_browser_process->GetApplicationLocale()` here, can we do that [1]?
Related details
- Alexander Kyereboah
- Daniel Murphy
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. |
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 9 comments![]( "Open in Gerrit")
I read Dan's recommendation in the design doc [1] as we'd prefer to not go the triplet model route, but perhaps that recommendation was made with the thought that we'd only need the two `lang` and `dir` members for all localized fields?
My only concern with the triplet in WebAppInstallInfo was that modifying `title` would require us to deal with the many callsites of `title`. If that concern is smaller than the overcrowding from adding additional `lang` and `dir` members for each localized value, I can go the `title()` getter route and have a separate `title_localized` instead of `title_lang` and `title_dir`. I'm also leaning towards the tuple route despite the migration of callsites that'll have to happen.
[1] "WebAppInstallInfo really only needs to either:
- Do a refactor (not necessary) to turn the title into a triplet struct thing
- Just add a lang and dir member to the WebAppInstallInfo, which would be used to set all 3 values of our new triplet type in the proto"
const blink::mojom::ManifestLocalizedTextObject* MatchLocalizedText(Can you share more details about what this function is doing? It seems like we're trying to find if `application_locale` is in the map, and if it isn't, then we try to get the language of the locale, and then try to find it again?
Imo that seems flaky, as we can't know the key of the map just from reading the code, as the key might be a language tag or a whole locale. Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
This function was trying to find the exact BCP47 language match, then falling back to trying to find a subtag match. I realize now that I had a mix up in assuming the keys for localized members were all ISO639 Locale tags, but the BCP47 language tags can be formed differently. Taking that into account, it's definitely flaky to do it this way, and using the `icu::Locale` library makes more sense.
Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
Do you mean during manifest parsing changing the HashMap to be `<icu::Locale, mojom::blink::ManifestLocalizedTextObjectPtr>` instead of `<String, mojom::blink::ManifestLocalizedTextObjectPtr>`?
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Hmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
I definitely agree with not doing it by hand! Considering the language tags that are keys for localized objects are BCP47[1], wouldn't converting `application_locale` here to ISO-639 mean it wouldn't match any keys? Or should we be calling `getLanguage()` on the keys as well?
void SetTitleFromNonLocalizedManifestFields(
const blink::mojom::Manifest& manifest,
WebAppInstallInfo& info) {
std::u16string name = manifest.name.value_or(std::u16string());
if (!name.empty()) {
info.title = name;
} else if (manifest.short_name) {
info.title = *manifest.short_name;
}
}This file has a lot of legacy code where we update the object in place, but that should not be done. This is a relic of the past, and could be a good code health task for the future for us to not be doing this anymore.
Ideally, we should be returning the output instead of updating an object in the function parameters (which in this case is an `output parameter`). See sentence 2 of [1], which says "Prefer using return values instead of output parameters".
So this would have a return value instead of a function parameter that is updated:
```
std::u16string GetNonLocalizedNameForManifest(const blink::mojom::Manifest& manifest) {
std::u16string name = manifest.name.value_or(std::u16string());
if(name.empty()) {
CHECK(!manifest.short_name.empty());
name = *manifest.short_name;
}
return name;
}
```and then in the callsites, we can have:
```
install_info.title = GetNonLocalizedNameForManifest(manifest);
```Wdyt?
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
This makes sense to me, noted that we prefer to stay away from updating in place now.
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {There is a lot of indentation here that makes it a bit harder to parse. Can we help simplify this area?
So maybe:
```
using LocalizedTitleLang = std::optional<std::u16string>;
using LocalizedDir = std::optional<blink::mojom::Manifest_TextDirection>;
using LocalizedInfo = std::tuple<std::u16string, LocalizedTitleLang,
LocalizedDir>;LocalizedInfo GetLocalizedNameFromManifest(const Manifest& manifest) {
// Parse manifest and construct a LocalizedInfo.
// If localized fields don't exist, only populated the first tuple field, and return std::nullopt for the other ones.
};// Inside this function:
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
LocalizedInfo localized_name = GetLocalizedNameFromManifest(manifest);
install_info().title = std::get<std::u16string>(localized_name);
install_info().title_lang = std::get<LocalizedTitleLang>(localized_name);
install_info().title_dir = std::get< LocalizedDir >(localized_name);
} else {
install_info().title = GetNonLocalizedNameForManifest(manifest);
}
```Wdyt about this approach?
Will take a crack at this, though this would look a different if we go the tuple model in WebAppInstallInfo route.
base::UTF8ToUTF16(g_browser_process->GetApplicationLocale());I just saw that we should be using `g_browser_process->GetFeatures()->application_locale_storage()->Get()` instead of `g_browser_process->GetApplicationLocale()` here, can we do that [1]?
Good catch!
localized_name = MatchLocalizedText(manifest_->short_name_localized,Hmmm, I wonder if this will be a lot of work converting the "string" key to an `icu::Locale`. Using a class like `icu::Locale` ensures data correctness, and prevents bugs where we might end up setting malformed keys in the map. Can we do that here?
I guess we can KINDA assume that the string is correct here, otherwise the manifest parser would have errored [1], but I would prefer to be extra careful here still. Let's convert the keys to an `icu::Locale`.
What do you think about having manifest_parser store the string keys as `icu::Locale` in the first place as opposed to converting the keys here?
if (localized_name->lang) {
install_info().title_lang = localized_name->lang;
}Same feedback for the `dir` field as well.
If `localized_name->lang` and `localized_name->dir` are std::optionals, then we can just do:
```
install_info().title = localized_name->value;
install_info().title_lang = localized_name->lang;
install_info().title_dir= localized_name->dir;
```That way, we reduce indentation here and improve readability.
Acknowledged
void AddLocalizedText(Please return a `base::flat_map<std::u16string, blink::mojom::ManifestLocalizedTextObjectPtr>` field instead of updating in place as per [1].
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
- Daniel Murphy
- Dibyajyoti Pal
- Robert Paveza
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 4 comments![]( "Open in Gerrit")
Aaah, I just realized something as to why we don't need a triplet.
We don't need to store separate `lang` and `dir` fields for title and description because what we are storing on the app corresponds to the application locale Chrome is running on. As such, it can never be that the `lang` (or `dir`) is different for title and description for a specific locale, because they should both correspond to the same language that matches the locale.
One flaw with this implementation is the following use-case:
1. Developer provides name and localized name for French.
2. Developer forgets to provide a localized description for French.
3. Chrome launches in French.
In that case we'll get a French name but english description, but I think that could be fine, since one "value" of the `lang`/`dir` per language is enough to reason about how the `lang` or `dir` will work, since both of those fields are optional anyway.
Wdyt?
const blink::mojom::ManifestLocalizedTextObject* MatchLocalizedText(Alexander KyereboahCan you share more details about what this function is doing? It seems like we're trying to find if `application_locale` is in the map, and if it isn't, then we try to get the language of the locale, and then try to find it again?
Imo that seems flaky, as we can't know the key of the map just from reading the code, as the key might be a language tag or a whole locale. Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
This function was trying to find the exact BCP47 language match, then falling back to trying to find a subtag match. I realize now that I had a mix up in assuming the keys for localized members were all ISO639 Locale tags, but the BCP47 language tags can be formed differently. Taking that into account, it's definitely flaky to do it this way, and using the `icu::Locale` library makes more sense.
Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
Do you mean during manifest parsing changing the HashMap to be `<icu::Locale, mojom::blink::ManifestLocalizedTextObjectPtr>` instead of `<String, mojom::blink::ManifestLocalizedTextObjectPtr>`?
Yep, that was what I was thinking. It does seem like we can still have the key be just the language without the country tag [1]. That way, we can support both `en-US` and `en`.
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Alexander KyereboahHmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
I definitely agree with not doing it by hand! Considering the language tags that are keys for localized objects are BCP47[1], wouldn't converting `application_locale` here to ISO-639 mean it wouldn't match any keys? Or should we be calling `getLanguage()` on the keys as well?
Hmmmm, assuming that this map represents [localized data members](https://www.w3.org/TR/appmanifest/#x_localized-members), then the key, or the language tag always has to be BCP47, as per [this](https://www.w3.org/TR/appmanifest/#dfn-language-tag), right? So if both are `icu::Locale`, we should be able to [compare directly it seems](https://source.chromium.org/chromium/chromium/src/+/main:third_party/icu/source/common/unicode/locid.h;l=324-331?q=locid.h)?
localized_name = MatchLocalizedText(manifest_->short_name_localized,Alexander KyereboahHmmm, I wonder if this will be a lot of work converting the "string" key to an `icu::Locale`. Using a class like `icu::Locale` ensures data correctness, and prevents bugs where we might end up setting malformed keys in the map. Can we do that here?
I guess we can KINDA assume that the string is correct here, otherwise the manifest parser would have errored [1], but I would prefer to be extra careful here still. Let's convert the keys to an `icu::Locale`.
What do you think about having manifest_parser store the string keys as `icu::Locale` in the first place as opposed to converting the keys here?
Hmmmm that's definitely better, but I don't think we can add an `icu::Locale` inside `manifest.mojom`. I think we might have to parse it into a string, but then use StructTraits to convert it into an `icu::Locale` [2], which imo is non trivial amount of work. I haven't done that in a long time though, so not sure if that is also doable?
@dmu...@chromium.org wdyt? Instead of storing this map [3] as string to "values", what if we stored to "icu::Locale" to values? Imo that gives better data correctness guarantees, and faulty strings are not read, making it much simpler to reason about.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/manifest/manifest.mojom
[2] https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#type-mapping-defining-1
[3] https://www.w3.org/TR/appmanifest/#x_localized-members
Related details
- Alexander Kyereboah
- Daniel Murphy
- Robert Paveza
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. |
Daniel Murphy (Gerrit)
Daniel Murphy added 2 comments![]( "Open in Gerrit")
We don't need to store separate lang and dir fields for title and description because what we are storing on the app corresponds to the application locale Chrome is running on. As such, it can never be that the lang (or dir) is different for title and description for a specific locale, because they should both correspond to the same language that matches the locale.
I don't think this true. The developer specifies a 'key' for the locale - so if our system is en-US, then it would look there, then 'en' if that isn't there, then go to the deafult (the normal field in the manifest without _localized, for example). each key has a value, and that value can have up to 3 members - lang, dir, and value.
So - a manifest can say the title for english speaking countries is still rendered in the french language (and.... rtl, if they want). This is why we need all 3 fields here.
const blink::mojom::ManifestLocalizedTextObject* MatchLocalizedText(Can you share more details about what this function is doing? It seems like we're trying to find if `application_locale` is in the map, and if it isn't, then we try to get the language of the locale, and then try to find it again?
Imo that seems flaky, as we can't know the key of the map just from reading the code, as the key might be a language tag or a whole locale. Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
I haven't looked closely, but I agree that it possible we should parse the items first into a supported type that can act as a key - this way we have all of the constraints afforded to that key type, AND we can error/exit early if the parsing fails.
base/containers/readme.md recommends absl::flat_hash_map<K,V,Hash> for the default map type. the Hash function needs to be provided in the template as the object doesn't define a AbslHash[something] function next to the type.
I expect that, when searching the map, we start looking at the most fine-grained version (en-US), then just "en", then fall back to the default value in manifest.
Related details
- Alexander Kyereboah
- Robert Paveza
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
each key has a value, and that value can have up to 3 members - lang, dir, and value.
Hmmm, then having a struct that stores all 3 fields make sense.
Dibyajyoti Pal (Gerrit)
I took a deeper look and there are roughly ~1000 use-cases of `WebAppInstallInfo::title`. Migrating all of them is going to be a huge pain [1], and is not trivial imo.
My suggestion is to:
1. Add a class inside `chrome/browser/web_applications/model` [2] to better store the localized information. Let's call this `LocalizedTextInfo` (or something similar, would leave it to you). This class has 3 vairables, the `value`, `lang` and `dir` fields. The `lang` and `dir` are optional fields, while `value` always needs to be populated. We can enforce a data correctness check here by ensuring that any calls to construct this class HAS to have the `value` field populated with an input string. This can be the first CL. We can add a "static" function like: `LocalizedTextInfo CreateWithValueOnly()` that sets the `value` field and returns the class by value to be populated. We can iteratively build on this to also support "proto" functionality, so that we can store this on the web app.
2. Add that field in the WebAppInstallInfo for every localized text field that we add (title, description etc). This is what is happening in this CL.
3. Whenever we write the WebAppInstallInfo to the web_app (which I believe happens [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_install_finalizer.cc;l=591;drc=128f35b3fb019f2fa67e1e798e113ed95f766096;bpv=1;bpt=1?q=WebAppInstallFinalizer&ss=chromium%2Fchromium%2Fsrc)), we do 2 things:
a. If the Localized class is populated, then we write that to the web app.
b. If the localized class is not populated (for existing callsites which just set the install_info->title directly), then create a Localized class for them with that value, and write that to the web app. This will happen as a follow up.
This way, we don't have to migrate thousands of callsites, and still ensure that the web app that we create and write is correct.
Wdyt?
[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_install_info.h;l=325-326;bpv=1;bpt=1
[2] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/model/?q=chrome%2Fbrowser%2Fweb_applications%2Fmodel&ss=chromium%2Fchromium%2Fsrc
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
I don't love having two field that can both be set and needing a CHECK
I prefer implicit construction. I checked out that and variant here:
https://godbolt.org/z/4777evqWz
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 4 comments![]( "Open in Gerrit")
This sounds like a workable solution that avoids refactoring hundreds of call sites and avoids unnecessary duplicated fields. I need to look a little more into step 3., but for now step 1 and 2 sound reasonable to me. I'll push that first LocalizedText class change up first and rebase this one to use that class.
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Alexander KyereboahHmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
Dibyajyoti PalI definitely agree with not doing it by hand! Considering the language tags that are keys for localized objects are BCP47[1], wouldn't converting `application_locale` here to ISO-639 mean it wouldn't match any keys? Or should we be calling `getLanguage()` on the keys as well?
Hmmmm, assuming that this map represents [localized data members](https://www.w3.org/TR/appmanifest/#x_localized-members), then the key, or the language tag always has to be BCP47, as per [this](https://www.w3.org/TR/appmanifest/#dfn-language-tag), right? So if both are `icu::Locale`, we should be able to [compare directly it seems](https://source.chromium.org/chromium/chromium/src/+/main:third_party/icu/source/common/unicode/locid.h;l=324-331?q=locid.h)?
I think I was confused at how we would compare the key against the `application_locale` when you said we should call `getLanguage()` on the `icu::Locale` cast `application_locale`, since that returns an ISO-639 language code and the key is BCP47.
I see now that you're saying compare the `icu::Locale` directly (en-US), then fallback to `getLanguage()` since that should return the more "generic" version (en)?
localized_name = MatchLocalizedText(manifest_->short_name_localized,Alexander KyereboahHmmm, I wonder if this will be a lot of work converting the "string" key to an `icu::Locale`. Using a class like `icu::Locale` ensures data correctness, and prevents bugs where we might end up setting malformed keys in the map. Can we do that here?
I guess we can KINDA assume that the string is correct here, otherwise the manifest parser would have errored [1], but I would prefer to be extra careful here still. Let's convert the keys to an `icu::Locale`.
Dibyajyoti PalWhat do you think about having manifest_parser store the string keys as `icu::Locale` in the first place as opposed to converting the keys here?
Hmmmm that's definitely better, but I don't think we can add an `icu::Locale` inside `manifest.mojom`. I think we might have to parse it into a string, but then use StructTraits to convert it into an `icu::Locale` [2], which imo is non trivial amount of work. I haven't done that in a long time though, so not sure if that is also doable?
@dmu...@chromium.org wdyt? Instead of storing this map [3] as string to "values", what if we stored to "icu::Locale" to values? Imo that gives better data correctness guarantees, and faulty strings are not read, making it much simpler to reason about.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/manifest/manifest.mojom
[2] https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#type-mapping-defining-1
[3] https://www.w3.org/TR/appmanifest/#x_localized-members
It sounds like Dan from the other thread[1] is in favor of having icu::Locale be the key in the parsing stage? I can try and see how much work that would be.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7114825/comment/b6b3a608_75208341/
field[base::UTF8ToUTF16(locale)] = std::move(localized_text);To fix the test failures, you might need to include `base/strings/utf_string_conversions.h` here.
- Dibyajyoti Pal
- Robert Paveza
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
Btw, Dan's suggestion above is a good idea too. I fixed it by adding the corresponding C++ "operator=" overload to support `std::u16string` here: https://godbolt.org/z/rcGcff8bW
If you make your type like this then we no longer need to do option 3 AND no longer need to update all the callsites, because then `install_info->title = u"ABC";` would still work. It WOULD require some C++ hackery though.
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Alexander KyereboahHmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
Dibyajyoti PalI definitely agree with not doing it by hand! Considering the language tags that are keys for localized objects are BCP47[1], wouldn't converting `application_locale` here to ISO-639 mean it wouldn't match any keys? Or should we be calling `getLanguage()` on the keys as well?
Alexander KyereboahHmmmm, assuming that this map represents [localized data members](https://www.w3.org/TR/appmanifest/#x_localized-members), then the key, or the language tag always has to be BCP47, as per [this](https://www.w3.org/TR/appmanifest/#dfn-language-tag), right? So if both are `icu::Locale`, we should be able to [compare directly it seems](https://source.chromium.org/chromium/chromium/src/+/main:third_party/icu/source/common/unicode/locid.h;l=324-331?q=locid.h)?
I think I was confused at how we would compare the key against the `application_locale` when you said we should call `getLanguage()` on the `icu::Locale` cast `application_locale`, since that returns an ISO-639 language code and the key is BCP47.
I see now that you're saying compare the `icu::Locale` directly (en-US), then fallback to `getLanguage()` since that should return the more "generic" version (en)?
- Alexander Kyereboah
- Robert Paveza
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
To be more clear, what this will enable is that if your class is of type `LocalizedTextInfo`, we can do both:
```
install_info->title = LocalizedTextInfo("en_US", "En", LTR);
```
and
```
install_info->title = u"ABC";
```
which is cool, and what we want.
Dibyajyoti Pal (Gerrit)
Fixed that type a bit more by removing some string copy operations and cleaning up: https://godbolt.org/z/feGhqj53W
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 1 comment![]( "Open in Gerrit")
Thanks for the references and cleaning up the class example. I've taken a crack at it here: https://chromium-review.googlesource.com/c/chromium/src/+/7124289
Related details
- Dibyajyoti Pal
- Robert Paveza
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
- Alexander Kyereboah
- Daniel Murphy
- Robert Paveza
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. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
AddLocalizedText(manifest->name_localized, "en-US", u"American English Name",Daniel MurphyThis is a very good example of why using `icu::Locale` on production code would be a good idea. We pass in a string here, and malformed string could lead to problems where the Locale is incorrect. We wouldn't catch it normally because the manifest we use here doesn't go through the `ManifestParser` as this is an unit-test, but with the `icu::Locale` suggestion, we could test that use-case here if we want!
I think we need a prereq CL here to make these correctly typed.
To do this, please:
- Create a CL that modifies the manifest.mojom to add new message types, `LocalizableTextMap` and `LocalizableImageMap`. These would then contain the `map<mojo_base.mojom.String16, ManifestLocalizedTextObject>` and `map<mojo_base.mojom.String16, array<ManifestImageResource>>` respectively.
- Modify the [manifest.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest.h?q=manifest.h&ss=chromium) and corresponding [type traits](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest_mojom_traits.h) [cc](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/manifest/manifest_mojom_traits.cc;l=218;drc=c8dc70b538f1bb0862f1be58237d6e945ee81819;bpv=1;bpt=1). To parse/deparse into custom C++ types for us that use the icu::Locale key type in the map. This can then enforce a failure to parse before it even gets to the browser, ellimating a lot of validity checking. That all happens in the Read method of the struct traits.
If you have questions, I recommend working with a local MSFT reviewer on your team before sending the CL to us - as per https://source.chromium.org/chromium/chromium/src/+/main:docs/cl_respect.md?q=cl_respect.md&ss=chromium
Prefer writing tests on public APIs where possible, which means using the scheduler to execute something like FetchManifestAndInstall on a web contents that hosts the manifest (for real for a browsertests, which we should have at least one of, or using FakeWebContentsManager in unittests, which we have infra set up for already).
These tests are 'fine' for the 'choosing' of the name, but are a bit internalls-y.
Also - we need to be SUPER careful about setting process globals like application locale, as mulitple tests are run on the same process. So it should usually be a base::AutoReset.
Related details
- Alexander Kyereboah
- Robert Paveza
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 1 comment![]( "Open in Gerrit")
AddLocalizedText(manifest->name_localized, "en-US", u"American English Name",Daniel MurphyThis is a very good example of why using `icu::Locale` on production code would be a good idea. We pass in a string here, and malformed string could lead to problems where the Locale is incorrect. We wouldn't catch it normally because the manifest we use here doesn't go through the `ManifestParser` as this is an unit-test, but with the `icu::Locale` suggestion, we could test that use-case here if we want!
I think we need a prereq CL here to make these correctly typed.
To do this, please:
- Create a CL that modifies the manifest.mojom to add new message types, `LocalizableTextMap` and `LocalizableImageMap`. These would then contain the `map<mojo_base.mojom.String16, ManifestLocalizedTextObject>` and `map<mojo_base.mojom.String16, array<ManifestImageResource>>` respectively.
- Modify the [manifest.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest.h?q=manifest.h&ss=chromium) and corresponding [type traits](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest_mojom_traits.h) [cc](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/manifest/manifest_mojom_traits.cc;l=218;drc=c8dc70b538f1bb0862f1be58237d6e945ee81819;bpv=1;bpt=1). To parse/deparse into custom C++ types for us that use the icu::Locale key type in the map. This can then enforce a failure to parse before it even gets to the browser, ellimating a lot of validity checking. That all happens in the Read method of the struct traits.
If you have questions, I recommend working with a local MSFT reviewer on your team before sending the CL to us - as per https://source.chromium.org/chromium/chromium/src/+/main:docs/cl_respect.md?q=cl_respect.md&ss=chromium
Prefer writing tests on public APIs where possible, which means using the scheduler to execute something like FetchManifestAndInstall on a web contents that hosts the manifest (for real for a browsertests, which we should have at least one of, or using FakeWebContentsManager in unittests, which we have infra set up for already).
These tests are 'fine' for the 'choosing' of the name, but are a bit internalls-y.
Also - we need to be SUPER careful about setting process globals like application locale, as mulitple tests are run on the same process. So it should usually be a base::AutoReset.
That makes sense, after the LocalizedText class lands I'll create a CL for adding the new message types and the custom struct traits to serialize to `icu::Locale`. I'll come back to this CL after those pre-reqs land.
Thanks for the reviewer reminder, at some point I had fallen out of habit of doing this, but I'll make sure to do so for my next CLs. Apologies for any avoidable overhead thus far.
Noted for the test advice, and I'll add a reset when setting process globals for testing.
Related details
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. |
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 10 comments![]( "Open in Gerrit")
(Adding Lu as a local reviewer)
Addressed comments now that the locale-keyed manifests and LocalizedText class pre-req CLs are in.
We now use `icu::Locale` for the keys, and we're storing the corresponding localized name in the `LocalizedText title` field of the WebAppInstallInfo based on application locale.
const blink::mojom::ManifestLocalizedTextObject* MatchLocalizedText(Daniel MurphyCan you share more details about what this function is doing? It seems like we're trying to find if `application_locale` is in the map, and if it isn't, then we try to get the language of the locale, and then try to find it again?
Imo that seems flaky, as we can't know the key of the map just from reading the code, as the key might be a language tag or a whole locale. Can we perhaps simplify this by making the map be keyed to an `icu::Locale`?
I haven't looked closely, but I agree that it possible we should parse the items first into a supported type that can act as a key - this way we have all of the constraints afforded to that key type, AND we can error/exit early if the parsing fails.
base/containers/readme.md recommends absl::flat_hash_map<K,V,Hash> for the default map type. the Hash function needs to be provided in the template as the object doesn't define a AbslHash[something] function next to the type.
I expect that, when searching the map, we start looking at the most fine-grained version (en-US), then just "en", then fall back to the default value in manifest.
Completed CL changing HashMaps to use icu::Locale keys for earlier validation.
This function looks at the fine-grained version (en-US), then calls `.getLanguage` to get the less granular version (en)
// Fall back to language-only match (match the part before the first '-').
size_t dash_pos = application_locale.find(u'-');
if (dash_pos != std::u16string::npos) {
std::u16string language_only = application_locale.substr(0, dash_pos);
it = localized_map.find(language_only);
if (it != localized_map.end()) {
return it->second.get();
}
}Alexander KyereboahHmmm, can we instead convert the `application_locale` to an `icu::Locale` and then call `getLanguage()` on it [1]?
The issue with doing these by hand is that there are always edge-cases or languages that we might be missing that are speced in a different way, and we'll handle them incorrectly. Using a library like `icu::Locale` ensures that we don't have to care about it, wdyt?
Dibyajyoti PalI definitely agree with not doing it by hand! Considering the language tags that are keys for localized objects are BCP47[1], wouldn't converting `application_locale` here to ISO-639 mean it wouldn't match any keys? Or should we be calling `getLanguage()` on the keys as well?
Alexander KyereboahHmmmm, assuming that this map represents [localized data members](https://www.w3.org/TR/appmanifest/#x_localized-members), then the key, or the language tag always has to be BCP47, as per [this](https://www.w3.org/TR/appmanifest/#dfn-language-tag), right? So if both are `icu::Locale`, we should be able to [compare directly it seems](https://source.chromium.org/chromium/chromium/src/+/main:third_party/icu/source/common/unicode/locid.h;l=324-331?q=locid.h)?
Dibyajyoti PalI think I was confused at how we would compare the key against the `application_locale` when you said we should call `getLanguage()` on the `icu::Locale` cast `application_locale`, since that returns an ISO-639 language code and the key is BCP47.
I see now that you're saying compare the `icu::Locale` directly (en-US), then fallback to `getLanguage()` since that should return the more "generic" version (en)?
Yep, that's right.
Done
void SetTitleFromNonLocalizedManifestFields(
const blink::mojom::Manifest& manifest,
WebAppInstallInfo& info) {
std::u16string name = manifest.name.value_or(std::u16string());
if (!name.empty()) {
info.title = name;
} else if (manifest.short_name) {
info.title = *manifest.short_name;
}
}Alexander KyereboahThis file has a lot of legacy code where we update the object in place, but that should not be done. This is a relic of the past, and could be a good code health task for the future for us to not be doing this anymore.
Ideally, we should be returning the output instead of updating an object in the function parameters (which in this case is an `output parameter`). See sentence 2 of [1], which says "Prefer using return values instead of output parameters".
So this would have a return value instead of a function parameter that is updated:
```
std::u16string GetNonLocalizedNameForManifest(const blink::mojom::Manifest& manifest) {
std::u16string name = manifest.name.value_or(std::u16string());
if(name.empty()) {
CHECK(!manifest.short_name.empty());
name = *manifest.short_name;
}
return name;
}
```and then in the callsites, we can have:
```
install_info.title = GetNonLocalizedNameForManifest(manifest);
```Wdyt?
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
This makes sense to me, noted that we prefer to stay away from updating in place now.
Done
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {Alexander KyereboahThere is a lot of indentation here that makes it a bit harder to parse. Can we help simplify this area?
So maybe:
```
using LocalizedTitleLang = std::optional<std::u16string>;
using LocalizedDir = std::optional<blink::mojom::Manifest_TextDirection>;
using LocalizedInfo = std::tuple<std::u16string, LocalizedTitleLang,
LocalizedDir>;LocalizedInfo GetLocalizedNameFromManifest(const Manifest& manifest) {
// Parse manifest and construct a LocalizedInfo.
// If localized fields don't exist, only populated the first tuple field, and return std::nullopt for the other ones.
};// Inside this function:
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
LocalizedInfo localized_name = GetLocalizedNameFromManifest(manifest);
install_info().title = std::get<std::u16string>(localized_name);
install_info().title_lang = std::get<LocalizedTitleLang>(localized_name);
install_info().title_dir = std::get< LocalizedDir >(localized_name);
} else {
install_info().title = GetNonLocalizedNameForManifest(manifest);
}
```Wdyt about this approach?
Will take a crack at this, though this would look a different if we go the tuple model in WebAppInstallInfo route.
Broke out the logic into a helper so that ParseManifestAndPopulateInfo method looks clean, and the helper method now uses the LocalizedText class from the pre-req CL so the logic is easier to parse.
base::UTF8ToUTF16(g_browser_process->GetApplicationLocale());Alexander KyereboahI just saw that we should be using `g_browser_process->GetFeatures()->application_locale_storage()->Get()` instead of `g_browser_process->GetApplicationLocale()` here, can we do that [1]?
Good catch!
Done
localized_name = MatchLocalizedText(manifest_->short_name_localized,Alexander KyereboahHmmm, I wonder if this will be a lot of work converting the "string" key to an `icu::Locale`. Using a class like `icu::Locale` ensures data correctness, and prevents bugs where we might end up setting malformed keys in the map. Can we do that here?
I guess we can KINDA assume that the string is correct here, otherwise the manifest parser would have errored [1], but I would prefer to be extra careful here still. Let's convert the keys to an `icu::Locale`.
Dibyajyoti PalWhat do you think about having manifest_parser store the string keys as `icu::Locale` in the first place as opposed to converting the keys here?
Alexander KyereboahHmmmm that's definitely better, but I don't think we can add an `icu::Locale` inside `manifest.mojom`. I think we might have to parse it into a string, but then use StructTraits to convert it into an `icu::Locale` [2], which imo is non trivial amount of work. I haven't done that in a long time though, so not sure if that is also doable?
@dmu...@chromium.org wdyt? Instead of storing this map [3] as string to "values", what if we stored to "icu::Locale" to values? Imo that gives better data correctness guarantees, and faulty strings are not read, making it much simpler to reason about.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/manifest/manifest.mojom
[2] https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#type-mapping-defining-1
[3] https://www.w3.org/TR/appmanifest/#x_localized-members
It sounds like Dan from the other thread[1] is in favor of having icu::Locale be the key in the parsing stage? I can try and see how much work that would be.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7114825/comment/b6b3a608_75208341/
Now uses `icu::Locale` keys
https://chromium-review.googlesource.com/c/chromium/src/+/7182261
if (localized_name->lang) {
install_info().title_lang = localized_name->lang;
}Alexander KyereboahSame feedback for the `dir` field as well.
If `localized_name->lang` and `localized_name->dir` are std::optionals, then we can just do:
```
install_info().title = localized_name->value;
install_info().title_lang = localized_name->lang;
install_info().title_dir= localized_name->dir;
```That way, we reduce indentation here and improve readability.
Acknowledged
We now use the `LocalizedText` class here.
void AddLocalizedText(Alexander KyereboahPlease return a `base::flat_map<std::u16string, blink::mojom::ManifestLocalizedTextObjectPtr>` field instead of updating in place as per [1].
[1] https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
Acknowledged
Done
AddLocalizedText(manifest->name_localized, "en-US", u"American English Name",Daniel MurphyThis is a very good example of why using `icu::Locale` on production code would be a good idea. We pass in a string here, and malformed string could lead to problems where the Locale is incorrect. We wouldn't catch it normally because the manifest we use here doesn't go through the `ManifestParser` as this is an unit-test, but with the `icu::Locale` suggestion, we could test that use-case here if we want!
Alexander KyereboahI think we need a prereq CL here to make these correctly typed.
To do this, please:
- Create a CL that modifies the manifest.mojom to add new message types, `LocalizableTextMap` and `LocalizableImageMap`. These would then contain the `map<mojo_base.mojom.String16, ManifestLocalizedTextObject>` and `map<mojo_base.mojom.String16, array<ManifestImageResource>>` respectively.
- Modify the [manifest.h](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest.h?q=manifest.h&ss=chromium) and corresponding [type traits](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/manifest/manifest_mojom_traits.h) [cc](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/manifest/manifest_mojom_traits.cc;l=218;drc=c8dc70b538f1bb0862f1be58237d6e945ee81819;bpv=1;bpt=1). To parse/deparse into custom C++ types for us that use the icu::Locale key type in the map. This can then enforce a failure to parse before it even gets to the browser, ellimating a lot of validity checking. That all happens in the Read method of the struct traits.
If you have questions, I recommend working with a local MSFT reviewer on your team before sending the CL to us - as per https://source.chromium.org/chromium/chromium/src/+/main:docs/cl_respect.md?q=cl_respect.md&ss=chromium
Prefer writing tests on public APIs where possible, which means using the scheduler to execute something like FetchManifestAndInstall on a web contents that hosts the manifest (for real for a browsertests, which we should have at least one of, or using FakeWebContentsManager in unittests, which we have infra set up for already).
These tests are 'fine' for the 'choosing' of the name, but are a bit internalls-y.
Also - we need to be SUPER careful about setting process globals like application locale, as mulitple tests are run on the same process. So it should usually be a base::AutoReset.
That makes sense, after the LocalizedText class lands I'll create a CL for adding the new message types and the custom struct traits to serialize to `icu::Locale`. I'll come back to this CL after those pre-reqs land.
Thanks for the reviewer reminder, at some point I had fallen out of habit of doing this, but I'll make sure to do so for my next CLs. Apologies for any avoidable overhead thus far.
Noted for the test advice, and I'll add a reset when setting process globals for testing.
Now using `icu::Locale` as a result of https://chromium-review.googlesource.com/c/chromium/src/+/7182261
Made sure tests settings global application locale is within a ScopedClosureRunner to ensure reset after it goes out of scope (base::AutoReset didn't work since it needs a direct pointer to the variable)
Keeping in mind adding a feature e2e test for text localization in my next CL for description that tests the public FetchManifestAndInstall API call.
Related details
- Dibyajyoti Pal
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 6 comments![]( "Open in Gerrit")
// Try to find a localized name that matches the system locale if the feature
// is enabled.
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
install_info().title = GetTitleFromLocalizedManifestFields(*manifest_);
} else {
install_info().title = GetTitleFromNonLocalizedManifestFields(*manifest_);
}nit: The current implementation works pretty good. One way we can simplify this further is by keeping this area as simple as possible like:
```
install_info().title = ParseTitleFromManifestMaybeLocalize(*manifest_);
```
And then inside `ParseTitleFromManifestMaybeLocalize()`, we do:
```
LocalizedText ParseTitleFromManifestMaybeLocalize(
const blink::mojom::Manifest& manifest) {
if(base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
// Try to localize and return early if needed.
}
// Return the non-localized titles from the manifest as fallback.
return ParseTitleFromNonLocalizedManifestFields(manifest);
}
```
Wdyt? This way, in the future, it's very easy to clean up this area as all we need to do is to remove the scoped braces.
auto web_app_info = GetWebAppInstallInfoFromJob(*manifest);
EXPECT_EQ(u"American English Name", web_app_info->title);
EXPECT_EQ(u"en-US", web_app_info->title.lang());
EXPECT_EQ(blink::mojom::Manifest_TextDirection::kLTR,
web_app_info->title.dir());
// Language-only fallback (en-US should match "en")
manifest->name_localized->clear();
manifest->name_localized->insert(
AddLocalizedText("en", u"Generic English Name", u"en",
blink::mojom::Manifest_TextDirection::kLTR));
g_browser_process->GetFeatures()->application_locale_storage()->Set("en-US");Let's make lines 1468-1479 another test, called `FallbackToLanguageLocaleNotFound`?
web_app_info = GetWebAppInstallInfoFromJob(*manifest);
EXPECT_EQ(u"Generic English Name", web_app_info->title);
EXPECT_EQ(u"en", web_app_info->title.lang());
EXPECT_EQ(blink::mojom::Manifest_TextDirection::kLTR,
web_app_info->title.dir());
// No match uses default name
manifest->name_localized->clear();
manifest->name_localized->insert(
AddLocalizedText("de-DE", u"Deutscher Name", u"de-DE",
blink::mojom::Manifest_TextDirection::kLTR));
g_browser_process->GetFeatures()->application_locale_storage()->Set("en-US");
web_app_info = GetWebAppInstallInfoFromJob(*manifest);
EXPECT_EQ(u"Default App Name", web_app_info->title);
EXPECT_FALSE(web_app_info->title.lang().has_value());
EXPECT_FALSE(web_app_info->title.dir().has_value());Let's break this test down into different tests, for readability and simplicity.
L1481-1491 is testing that the "default" name is used in case the locale does not exist in the manifest, so let's make it a separate test called `FallbackToDefaultLocaleNotFound`?
// Save and restore the application locale to avoid polluting other tests.nitty-nit: Let's only add this comment once, probably fine to remove it for the other cases.
// Empty value is ignored
manifest->name_localized->clear();
manifest->name_localized->insert(AddLocalizedText(
"en-US", u"", u"en-US", blink::mojom::Manifest_TextDirection::kLTR));
web_app_info = GetWebAppInstallInfoFromJob(*manifest);
EXPECT_EQ(u"Default App Name", web_app_info->title);
EXPECT_FALSE(web_app_info->title.lang().has_value());
EXPECT_FALSE(web_app_info->title.dir().has_value());Can we also add a test where only `lang` or `dir` is specified, but not both? And then that is parsed correctly at the end?
BASE_FEATURE(kWebAppManifestPolicyAppIdentityUpdate,This was just removed in crrev.com/c/7302686, was this added during a rebase?
Related details
- Alexander Kyereboah
- Lu Huang
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kWebAppManifestPolicyAppIdentityUpdate,This was just removed in crrev.com/c/7302686, was this added during a rebase?
If so, can we remove? Thanks!
Alexander Kyereboah (Gerrit)
Alexander Kyereboah added 2 comments![]( "Open in Gerrit")
// Try to find a localized name that matches the system locale if the feature
// is enabled.
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
install_info().title = GetTitleFromLocalizedManifestFields(*manifest_);
} else {
install_info().title = GetTitleFromNonLocalizedManifestFields(*manifest_);
}nit: The current implementation works pretty good. One way we can simplify this further is by keeping this area as simple as possible like:
```
install_info().title = ParseTitleFromManifestMaybeLocalize(*manifest_);
```And then inside `ParseTitleFromManifestMaybeLocalize()`, we do:
```
LocalizedText ParseTitleFromManifestMaybeLocalize(
const blink::mojom::Manifest& manifest) {
if(base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
// Try to localize and return early if needed.
}
// Return the non-localized titles from the manifest as fallback.
return ParseTitleFromNonLocalizedManifestFields(manifest);
}
```Wdyt? This way, in the future, it's very easy to clean up this area as all we need to do is to remove the scoped braces.
I can see how that would streamline the `install_info().title` line! I am a little bit conscious of if we are going overboard with breaking out code into helper methods for the sake of simplicity. When we ship this, we'd also be removing everything in the hypothetical `ParseTitleFromManifestMaybeLocalize` method besides `GetTitleFromLocalizedManifestFields` since that method itself handles the fallback call to `GetTitleFromNonLocalizedManifestFields`. Considering that when on by default that means this method would just have a call to `GetTitleFromLocalizedManifestFields` in it, I'm thinking it might not be necessary to break it out further.
If we want to clarify that the fallback already happens in `GetTitleFromLocalizedManifestFields`, maybe an alternative could be renaming that function to `TryGetTitleFromLocalizedManifestFields`? Wdyt?
BASE_FEATURE(kWebAppManifestPolicyAppIdentityUpdate,Dibyajyoti PalThis was just removed in crrev.com/c/7302686, was this added during a rebase?
If so, can we remove? Thanks!
Ack, the rebase gave me the impression that it was a newly added feature flag 🤦, I'll remove it next patch!
Related details
- Dibyajyoti Pal
- Lu Huang
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
// Try to find a localized name that matches the system locale if the feature
// is enabled.
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
install_info().title = GetTitleFromLocalizedManifestFields(*manifest_);
} else {
install_info().title = GetTitleFromNonLocalizedManifestFields(*manifest_);
}Alexander Kyereboahnit: The current implementation works pretty good. One way we can simplify this further is by keeping this area as simple as possible like:
```
install_info().title = ParseTitleFromManifestMaybeLocalize(*manifest_);
```And then inside `ParseTitleFromManifestMaybeLocalize()`, we do:
```
LocalizedText ParseTitleFromManifestMaybeLocalize(
const blink::mojom::Manifest& manifest) {
if(base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
// Try to localize and return early if needed.
}
// Return the non-localized titles from the manifest as fallback.
return ParseTitleFromNonLocalizedManifestFields(manifest);
}
```Wdyt? This way, in the future, it's very easy to clean up this area as all we need to do is to remove the scoped braces.
I can see how that would streamline the `install_info().title` line! I am a little bit conscious of if we are going overboard with breaking out code into helper methods for the sake of simplicity. When we ship this, we'd also be removing everything in the hypothetical `ParseTitleFromManifestMaybeLocalize` method besides `GetTitleFromLocalizedManifestFields` since that method itself handles the fallback call to `GetTitleFromNonLocalizedManifestFields`. Considering that when on by default that means this method would just have a call to `GetTitleFromLocalizedManifestFields` in it, I'm thinking it might not be necessary to break it out further.
If we want to clarify that the fallback already happens in `GetTitleFromLocalizedManifestFields`, maybe an alternative could be renaming that function to `TryGetTitleFromLocalizedManifestFields`? Wdyt?
I think we're on the same page haha, I didn't ask to add `ParseTitleFromManifestMaybeLocalize()` as a new method or breaking out code into small helper methods, I was suggesting we use the same function that you created and build on top of it like:
1. Rename `GetTitleFromLocalizedManifestFields()` to `ParseTitleFromManifestMaybeLocalize()` or `GetLocalizedTitleFromManifestFields()`. Adding `Try...` to the function name could indicate that it fails while running, but that is not what happens in the code here.
2. Move the `base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)` check there, and take advantage of the fallback mechanism you created.
The whole function would look something like the following:
```
LocalizedText GetLocalizedTitleFromManifestFields(
const blink::mojom::Manifest& manifest) {
if (base::FeatureList::IsEnabled(features::kWebAppManifestLocalization)) {
const icu::Locale application_locale(g_browser_process->GetFeatures()
->application_locale_storage()
->Get()
.c_str());
const blink::mojom::ManifestLocalizedTextObject* localized_name = nullptr;
if (manifest.name_localized.has_value()) {
localized_name =
MatchLocalizedText(*manifest.name_localized, application_locale);
}
if (!localized_name && manifest.short_name_localized.has_value()) {
localized_name = MatchLocalizedText(*manifest.short_name_localized,
application_locale);
}
if (localized_name && !localized_name->value.empty()) {
return LocalizedText(localized_name->value, localized_name->lang,
localized_name->dir);
}
}// Fall back to non-localized fields. Use assignment operator which handles
// clearing lang/dir fields.
LocalizedText result;
result = GetTitleFromNonLocalizedManifestFields(manifest);
return result;
}
```
We could also remove `GetTitleFromNonLocalizedManifestFields()` totally and inline it before `return result;`, that way we have less helper methods.
Related details
- Alexander Kyereboah
- Lu Huang
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. |