Implement css property 'font-language-override' [chromium/src : main]
Sejal Anand (Gerrit)
Sejal Anand voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Since this is a feature request won't this require a I2S ??
Related details
- Dileep Maurya
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Shouldn't the changes be behind a feature flag? Atleast those which are not architectural in nature?
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
#include "third_party/blink/renderer/platform/heap/member.h"is this needed?
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
#include "third_party/blink/renderer/platform/heap/member.h"could it be forward declared here, if it's needed?
Sejal Anand (Gerrit)
Sejal Anand added 1 comment![]( "Open in Gerrit")
Sejal AnandSince this is a feature request won't this require a I2S ??
Could you clarify the purpose of this comment? If runtime flag is added, what should be included in the CL if an I2S is required?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Sejal Anand (Gerrit)
Sejal Anand added 2 comments![]( "Open in Gerrit")
#include "third_party/blink/renderer/platform/heap/member.h"could it be forward declared here, if it's needed?
Done
#include "third_party/blink/renderer/platform/heap/member.h"Sejal Anandis this needed?
Done
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Sejal AnandSince this is a feature request won't this require a I2S ??
Could you clarify the purpose of this comment? If runtime flag is added, what should be included in the CL if an I2S is required?
My understanding of the bug https://issues.chromium.org/issues/41170551 is that it's a feature request and hence it requires a I2S so that we can get the go head from blink-dev.
Since an I2S is required, runtime flag addition may be required, your changes should be such that when the runtime feature is off your feature should not work and when it is on your feature should work.
That being said if such a runtime-time feature is not feasible to add (due to the nature of the feature) an I2S might still be required if it's a feature request and in that case you can only merge your changes once you get approvals from blink-dev
Related details
- Dileep Maurya
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Sejal Anand (Gerrit)
Sejal Anand added 1 comment![]( "Open in Gerrit")
Shouldn't the changes be behind a feature flag? Atleast those which are not architectural in nature?
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Sejal Anand (Gerrit)
Sejal Anand added 1 comment![]( "Open in Gerrit")
Sejal AnandSince this is a feature request won't this require a I2S ??
Divyansh MangalCould you clarify the purpose of this comment? If runtime flag is added, what should be included in the CL if an I2S is required?
My understanding of the bug https://issues.chromium.org/issues/41170551 is that it's a feature request and hence it requires a I2S so that we can get the go head from blink-dev.
Since an I2S is required, runtime flag addition may be required, your changes should be such that when the runtime feature is off your feature should not work and when it is on your feature should work.
That being said if such a runtime-time feature is not feasible to add (due to the nature of the feature) an I2S might still be required if it's a feature request and in that case you can only merge your changes once you get approvals from blink-dev
there is already a comment about adding feature flag, I have resolved it. To clarify yes, this feature will require I2S.
Gaurav Kumar (Gerrit)
Gaurav Kumar added 2 comments![]( "Open in Gerrit")
This CL introduces support for the 'font-language-override' CSS
property, which allows authors to override the language system by
specifying a four-character OpenType language tag.Please specify whether this is a CSS property or a @font-face descriptor. Also, kindly mention what has been implemented as part of this CL.
Related details
- Dileep Maurya
- Divyansh Mangal
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaurav Kumar (Gerrit)
Gaurav Kumar added 1 comment![]( "Open in Gerrit")
This CL introduces support for the 'font-language-override' CSS
property, which allows authors to override the language system by
specifying a four-character OpenType language tag.Please specify whether this is a CSS property or a @font-face descriptor. Also, kindly mention what has been implemented as part of this CL.
Please ignore
Gaurav Kumar (Gerrit)
Gaurav Kumar added 1 comment![]( "Open in Gerrit")
This CL introduces support for the 'font-language-override' CSS
property, which allows authors to override the language system by
specifying a four-character OpenType language tag.Do we need this property as @font-face descriptor ?
Sejal Anand (Gerrit)
Sejal Anand voted Commit-Queue+1![]( "Open in Gerrit")
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya added 3 comments![]( "Open in Gerrit")
const size_t kMaxLanguageOverrideLength = 4;Can we add a comment here explaining why MaxLength is 4?
StyleResolverState& state,`state` is not being used here, can we remove this?
font_description.NeedLanguageOverride()
? hb_ot_tag_to_language(hb_tag_from_string(
font_description.FontLanguageOverride().Utf8().data(), -1))Can we add a comment here to explain the intent? Also, why are we passing `-1` here? A comment would be helpful maybe.
Related details
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Sejal Anand (Gerrit)
Sejal Anand added 7 comments![]( "Open in Gerrit")
Sejal AnandSince this is a feature request won't this require a I2S ??
Divyansh MangalCould you clarify the purpose of this comment? If runtime flag is added, what should be included in the CL if an I2S is required?
Sejal AnandMy understanding of the bug https://issues.chromium.org/issues/41170551 is that it's a feature request and hence it requires a I2S so that we can get the go head from blink-dev.
Since an I2S is required, runtime flag addition may be required, your changes should be such that when the runtime feature is off your feature should not work and when it is on your feature should work.
That being said if such a runtime-time feature is not feasible to add (due to the nature of the feature) an I2S might still be required if it's a feature request and in that case you can only merge your changes once you get approvals from blink-dev
there is already a comment about adding feature flag, I have resolved it. To clarify yes, this feature will require I2S.
Done
This CL introduces support for the 'font-language-override' CSS
property, which allows authors to override the language system by
specifying a four-character OpenType language tag.Do we need this property as @font-face descriptor ?
yes, descriptor can be implemented after we ship this CSS property.
priority: 1,How was it decided ?
const size_t kMaxLanguageOverrideLength = 4;Can we add a comment here explaining why MaxLength is 4?
it is mentioned in spec - 'single four-character case-sensitive OpenType language system tag, specifies the OpenType language system to be used instead of the language system implied by the language of the element. If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched'
https://www.w3.org/TR/css-fonts-4/#font-language-override-prop
Do we need to add comment to explain this, especially given that other properties in this class also follow spec-driven parsing rules? wdyt?
const size_t kMaxLanguageOverrideLength = 4;Can we add a comment here explaining why MaxLength is 4?
it is mentioned in spec - 'single four-character case-sensitive OpenType language system tag, specifies the OpenType language system to be used instead of the language system implied by the language of the element. If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched'
https://www.w3.org/TR/css-fonts-4/#font-language-override-prop
Do we need to add comment to explain this, especially given that other properties in this class also follow spec-driven parsing rules. wdyt ?
`state` is not being used here, can we remove this?
the parameter is retained for consistency across all converter function signatures in this class, which simplifies code generation and dispatch logic. you may find many such functions where 'state' is not used but it exists in function definition.
font_description.NeedLanguageOverride()
? hb_ot_tag_to_language(hb_tag_from_string(
font_description.FontLanguageOverride().Utf8().data(), -1))Can we add a comment here to explain the intent? Also, why are we passing `-1` here? A comment would be helpful maybe.
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
const size_t kMaxLanguageOverrideLength = 4;Can we add a comment here explaining why MaxLength is 4?
it is mentioned in spec - 'single four-character case-sensitive OpenType language system tag, specifies the OpenType language system to be used instead of the language system implied by the language of the element. If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched'
https://www.w3.org/TR/css-fonts-4/#font-language-override-propDo we need to add comment to explain this, especially given that other properties in this class also follow spec-driven parsing rules. wdyt ?
`especially given that other properties in this class also follow spec-driven parsing rules` - you're right here.
I think you can provide this spec link here, but it's not necessary. `// https://www.w3.org/TR/css-fonts-4/#font-language-override-prop`
Related details
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 15 comments![]( "Open in Gerrit")
property, which allows authors to override the language system by"...override the system language used for glyph substitution at the CSS level by specifying..."
converter: "ConvertFontLanguageOverride",I think this can just be `ConvertString<CSSValueID::kNormal>` and you can remove your custom `ConvertFontLanguageOverride` method.
const size_t kMaxLanguageOverrideLength = 4;`constexpr` so this gets substituted at compile time (and thus doesn't take up memory)
CSSValue* ParseFontLanguageOverrideString(CSSParserTokenStream& stream) {This should be named `ConsumeFontLanguageOverrideString`, since it consumes a token from the stream.
size_t end = language_override.length() - 1;Make sure to `const` all locals that shouldn't change their value
size_t end = language_override.length() - 1;
while (end >= 0 && IsCSSSpace(language_override[end])) {
--end;
}Why are trailing spaces trimmed? According to the spec, it should actually be *adding* spaces if it's less than 4 characters long:
"If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched."
https://www.w3.org/TR/css-fonts-4/#font-language-override-prop
If the author specifies trailing spaces, that should be fine.
for (size_t index = 0; index < language_override.length(); ++index) {
if (!IsASCII(language_override[index])) {
return nullptr;
}
}Can you add a comment describing why this is fails to parse when non-ASCII characters are encountered?
```
// "All tags are four-character strings composed of a limited set of ASCII characters" per https://learn.microsoft.com/en-us/typography/opentype/spec/languagetags
```
Also, are you sure this should be a parse error?
if (language_override.length()) {This needs a section that adds spaces per https://www.w3.org/TR/css-fonts-4/#font-language-override-prop (see comment above)
auto* identifier_value = DynamicTo<CSSIdentifierValue>(value);`const auto*` all locals that can be made `const`
bool NeedLanguageOverride() const {I think `HasLanguageOverride` is more consistent with the naming in this file
if (!language_override_.IsNull()) {Should this be `HasLanguageOverride() / NeedLanguageOverride()` so it checks for empty and `normal` values?
// If the font-language-override property is present, its value (a`font-language-override` (use backticks for programming-language-specific terms)
? hb_ot_tag_to_language(hb_tag_from_string(`hb_tag_from_string` can return `HB_TAG_NONE` - do you need to check for this?
? hb_ot_tag_to_language(hb_tag_from_string(This is a testharness.js-based test.
[FAIL] e.style['font-language-override'] = "normal" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"KSW\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"APPH\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"ENG \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"ksw\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"tr\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"en \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\" en \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"1 %\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
Harness: the test ran to completion.Are there any tests for invalid values? I can think of a few, including empty, non-ASCII, longer than 4 characters, disallowed keywords. Please add a `font-language-override-invalid` test similar to this one but with invalid values and compare behaviors in other browsers.
Related details
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sejal Anand (Gerrit)
Sejal Anand added 15 comments![]( "Open in Gerrit")
property, which allows authors to override the language system by"...override the system language used for glyph substitution at the CSS level by specifying..."
Done
I think this can just be `ConvertString<CSSValueID::kNormal>` and you can remove your custom `ConvertFontLanguageOverride` method.
Done
`constexpr` so this gets substituted at compile time (and thus doesn't take up memory)
Done
CSSValue* ParseFontLanguageOverrideString(CSSParserTokenStream& stream) {This should be named `ConsumeFontLanguageOverrideString`, since it consumes a token from the stream.
There is already a function `ConsumeFontLanguageOverride` calling `ParseFontLanguageOverrideString`
Make sure to `const` all locals that shouldn't change their value
cannot mark `end` const here as at line 6208 variable value is being updated.
size_t end = language_override.length() - 1;
while (end >= 0 && IsCSSSpace(language_override[end])) {
--end;
}Why are trailing spaces trimmed? According to the spec, it should actually be *adding* spaces if it's less than 4 characters long:
"If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched."
https://www.w3.org/TR/css-fonts-4/#font-language-override-prop
If the author specifies trailing spaces, that should be fine.
done
for (size_t index = 0; index < language_override.length(); ++index) {
if (!IsASCII(language_override[index])) {
return nullptr;
}
}Can you add a comment describing why this is fails to parse when non-ASCII characters are encountered?
```
// "All tags are four-character strings composed of a limited set of ASCII characters" per https://learn.microsoft.com/en-us/typography/opentype/spec/languagetags
```Also, are you sure this should be a parse error?
mentioned in spec that 'single four-character case-sensitive OpenType language system tag, specifies the OpenType language system to be used instead of the language system implied by the language of the element'
here is the link - https://www.w3.org/TR/css-values-4/#string-value, so we should support non-ASCII characters as per spec.
This needs a section that adds spaces per https://www.w3.org/TR/css-fonts-4/#font-language-override-prop (see comment above)
done
auto* identifier_value = DynamicTo<CSSIdentifierValue>(value);`const auto*` all locals that can be made `const`
Done
I think `HasLanguageOverride` is more consistent with the naming in this file
Done
Should this be `HasLanguageOverride() / NeedLanguageOverride()` so it checks for empty and `normal` values?
Done
// If the font-language-override property is present, its value (a`font-language-override` (use backticks for programming-language-specific terms)
Done
addressed
`hb_tag_from_string` can return `HB_TAG_NONE` - do you need to check for this?
it returns NONE in case string is null, but this case is already covered in font_description.NeedLanguageOverride()
This is a testharness.js-based test.
[FAIL] e.style['font-language-override'] = "normal" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"KSW\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"APPH\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"ENG \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"ksw\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"tr\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"en \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\" en \\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
[FAIL] e.style['font-language-override'] = "\\"1 %\\"" should set the property value
assert_not_equals: property should be set got disallowed value ""
Harness: the test ran to completion.Are there any tests for invalid values? I can think of a few, including empty, non-ASCII, longer than 4 characters, disallowed keywords. Please add a `font-language-override-invalid` test similar to this one but with invalid values and compare behaviors in other browsers.
yes, there is a test to cover invalid values -https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-fonts/parsing/font-language-override-invalid.html
Related details
- Divyansh Mangal
- Gaurav Kumar
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sejal Anand (Gerrit)
Sejal Anand added 4 comments![]( "Open in Gerrit")
converter: "ConvertFontLanguageOverride",Sejal AnandI think this can just be `ConvertString<CSSValueID::kNormal>` and you can remove your custom `ConvertFontLanguageOverride` method.
Done
Thanks! I initially tried using ConvertString<CSSValueID::kNormal>, but it led to crashes in multiple test cases.(https://chromium-review.googlesource.com/c/chromium/src/+/6873763/25?checksPatchset=23&checksRunsSelected=%F0%9F%9F%AA%E2%80%85%E2%80%85win-rel&tab=checks). The issue is that template function ConvertString assumes the value is either a CSSStringValue or a CSSIdentifierValue with exactly the IdForNone value and DCHECKs otherwise.
I kept the custom ConvertFontLanguageOverride function to safely handle cases.
size_t end = language_override.length() - 1;
while (end >= 0 && IsCSSSpace(language_override[end])) {
--end;
}Sejal AnandWhy are trailing spaces trimmed? According to the spec, it should actually be *adding* spaces if it's less than 4 characters long:
"If the string is shorter than four characters, it is padded at the end with space (U+0020) characters such that the length is 4, before being matched."
https://www.w3.org/TR/css-fonts-4/#font-language-override-prop
If the author specifies trailing spaces, that should be fine.
done
As discussed, added a comment.
for (size_t index = 0; index < language_override.length(); ++index) {
if (!IsASCII(language_override[index])) {
return nullptr;
}
}Sejal AnandCan you add a comment describing why this is fails to parse when non-ASCII characters are encountered?
```
// "All tags are four-character strings composed of a limited set of ASCII characters" per https://learn.microsoft.com/en-us/typography/opentype/spec/languagetags
```Also, are you sure this should be a parse error?
mentioned in spec that 'single four-character case-sensitive OpenType language system tag, specifies the OpenType language system to be used instead of the language system implied by the language of the element'
here is the link - https://www.w3.org/TR/css-values-4/#string-value, so we should support non-ASCII characters as per spec.
added comment
if (language_override.length()) {Sejal AnandThis needs a section that adds spaces per https://www.w3.org/TR/css-fonts-4/#font-language-override-prop (see comment above)
done
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sejal Anand (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 5 comments![]( "Open in Gerrit")
Looks like the bots are stuck (seems to be unrelated to this change), so I'm sending some feedback while it's still WIP.
// The CSS Fonts Level 4 spec:Nit: add a blank line before this big comment (and the one below that starts with `"All tags are four-character strings co...`
while (end >= 0 && IsCSSSpace(language_override[end])) {
--end;
}Do you actually need to trim trailing spaces? It looks like this needs a test case. Please add a test case that matches Firefox's current behavior with trailing spaces.
// https://learn.microsoft.com/en-us/typography/opentype/spec/languagetagsI would add another comment here that rejecting non-ASCII characters is also done to match Firefox (which is a bit inconsistent with them allowing < 4 characters, but it's explained in the bug).
for (size_t index = 0; index < language_override.length(); ++index) {
if (!IsASCII(language_override[index])) {
return nullptr;
}
}Can this loop be replaced with `language_override.ContainsOnlyASCIIOrEmpty()`? You can adjust the `if` below to be:
`if (language_override.length() && language_override.ContainsOnlyASCIIOrEmpty()) {`
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Sejal Anand (Gerrit)
Sejal Anand added 4 comments![]( "Open in Gerrit")
Nit: add a blank line before this big comment (and the one below that starts with `"All tags are four-character strings co...`
done
while (end >= 0 && IsCSSSpace(language_override[end])) {
--end;
}Do you actually need to trim trailing spaces? It looks like this needs a test case. Please add a test case that matches Firefox's current behavior with trailing spaces.
// https://learn.microsoft.com/en-us/typography/opentype/spec/languagetagsI would add another comment here that rejecting non-ASCII characters is also done to match Firefox (which is a bit inconsistent with them allowing < 4 characters, but it's explained in the bug).
done
for (size_t index = 0; index < language_override.length(); ++index) {
if (!IsASCII(language_override[index])) {
return nullptr;
}
}Can this loop be replaced with `language_override.ContainsOnlyASCIIOrEmpty()`? You can adjust the `if` below to be:
`if (language_override.length() && language_override.ContainsOnlyASCIIOrEmpty()) {`
The issue is that this condition mistakenly rejects empty strings, even though ContainsOnlyASCIIOrEmpty() already handles them correctly—making the language_override.length() check redundant and overly strict
I have added if(language_override.ContainsOnlyASCIIOrEmpty()), anyways we are checking if language_override is empty or not
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Dominik Röttsches (Gerrit)
Dominik Röttsches added 3 comments![]( "Open in Gerrit")
Some comments below, otherwise core implementation looks good, thank you. Adding Koji to take a look.
"font-variation-settings", "font-language-override"Are there tests in this CL that confirm that the `font` property resets `font-language-override` - compare https://www.w3.org/TR/css-fonts-4/#reset-implicitly
const hb_language_t language = [&]() {I find this hard to read, please factor this out into an inline function that takes just a font description and returns `hb_language_t` and give it a descriptive name.
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Koji Ishii
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sejal Anand (Gerrit)
Sejal Anand added 1 comment![]( "Open in Gerrit")
"font-variation-settings", "font-language-override"Are there tests in this CL that confirm that the `font` property resets `font-language-override` - compare https://www.w3.org/TR/css-fonts-4/#reset-implicitly
there are existing tests to make sure subproperties are reset implicitly and font-language-override is also covered here - https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-fonts/font-shorthand-subproperties-reset.html
Related details
- Dileep Maurya
- Divyansh Mangal
- Dominik Röttsches
- Gaurav Kumar
- Koji Ishii
- Kurt Catti-Schmidt
- Ragvesh Sharma
- Vinay Singh
- Virali Purbey
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
1 comment
- Dileep Maurya
- Divyansh Mangal
- Dominik Röttsches
- Gaurav Kumar
- Koji Ishii
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review