Bug 204276 – Implement the 'ic' unit from CSS Values 4

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

https://drafts.csswg.org/css-values-4/#ic

[Radar WebKit Bug Importer]

<rdar://problem/57256127>

[Kiet Ho]

Created attachment 437711 [details]
Patch

Patch. Reviews appreciated, after which I'll remove all logging

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Kiet Ho]

Created attachment 437712 [details]
Patch

Patch. Reviews appreciated, after which I'll remove all logging

[Kiet Ho]

Created attachment 437717 [details]
Patch

Patch. Removed all logging to avoid compilatione errors.

[Kiet Ho]

Created attachment 437725 [details]
Patch

Fix various runtime crashes.

[Darin Adler]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
> +    case CSSUnitType::CSS_ICS:                          return 110;

Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.

This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.

> Source/WebCore/platform/graphics/Font.cpp:170
> +    auto* glyphPageCjkWater = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater));
> +    if (glyphPageCjkWater) {

Please scope the variable into the if. I suggest naming it just "page".

    if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {

But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.

I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.

> Source/WebCore/platform/graphics/Font.cpp:171
> +        auto cjkWaterGlyph = glyphPageCjkWater->glyphDataForCharacter(cjkWater).glyph;

I suggest naming the local variable just glyphData.

> Source/WebCore/platform/graphics/FontMetrics.h:152
> +        m_ideogramWidth.reset();

We typically write:

    m_ideogramWidth = std::nullopt;

Not sure if it’s better style.

> Source/WebCore/platform/graphics/FontMetrics.h:173
> +    std::optional<float> m_ideogramWidth { };

No need for that "{ }".

[Darin Adler]

Good work, I think this is close to right!

[Darin Adler]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:620
> +        if (std::optional<float> width = fontMetrics->ideogramWidth())
> +            return (*width) * value;

I would write:

    if (auto width = fontMetrics->ideogramWidth())
        return *width * value;

[Kiet Ho]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>> Source/WebCore/css/CSSPrimitiveValue.cpp:620
>> +            return (*width) * value;
> 
> I would write:
> 
>     if (auto width = fontMetrics->ideogramWidth())
>         return *width * value;

This is a C++ habit of mine: wrapping everything in parentheses so I won't have to remember operator precedence. But will change.

>> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
>> +    case CSSUnitType::CSS_ICS:                          return 110;
> 
> Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.
> 
> This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.

To be honest, before your comment I didn't really know which value is right here either. I could've returned CSS_UNKNOWN, but no units did that so I decided to follow the existing "convention". Will change regardless, but in that case we wouldn't need to handle it in DeprecaredCSSOMPrimitiveValue::getFloatValue() right?

>> Source/WebCore/platform/graphics/Font.cpp:170
>> +    if (glyphPageCjkWater) {
> 
> Please scope the variable into the if. I suggest naming it just "page".
> 
>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
> 
> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
> 
> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.

I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.

>> Source/WebCore/platform/graphics/Font.cpp:171
>> +        auto cjkWaterGlyph = glyphPageCjkWater->glyphDataForCharacter(cjkWater).glyph;
> 
> I suggest naming the local variable just glyphData.

Will change.

>> Source/WebCore/platform/graphics/FontMetrics.h:152
>> +        m_ideogramWidth.reset();
> 
> We typically write:
> 
>     m_ideogramWidth = std::nullopt;
> 
> Not sure if it’s better style.

Will change.

>> Source/WebCore/platform/graphics/FontMetrics.h:173
>> +    std::optional<float> m_ideogramWidth { };
> 
> No need for that "{ }".

Will change.

[Darin Adler]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
>>> +    case CSSUnitType::CSS_ICS:                          return 110;
>> 
>> Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.
>> 
>> This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.
> 
> To be honest, before your comment I didn't really know which value is right here either. I could've returned CSS_UNKNOWN, but no units did that so I decided to follow the existing "convention". Will change regardless, but in that case we wouldn't need to handle it in DeprecaredCSSOMPrimitiveValue::getFloatValue() right?

Yes.

But we need to add tests rather than make the changes based on theory.

[Darin Adler]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>> Source/WebCore/platform/graphics/Font.cpp:170
>>> +    if (glyphPageCjkWater) {
>> 
>> Please scope the variable into the if. I suggest naming it just "page".
>> 
>>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
>> 
>> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
>> 
>> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.
> 
> I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.

That sounds like a good idea, although you’d probably have to use a std::optional for the return value?

[Kiet Ho]

Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>>> Source/WebCore/platform/graphics/Font.cpp:170
>>>> +    if (glyphPageCjkWater) {
>>> 
>>> Please scope the variable into the if. I suggest naming it just "page".
>>> 
>>>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
>>> 
>>> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
>>> 
>>> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.
>> 
>> I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.
> 
> That sounds like a good idea, although you’d probably have to use a std::optional for the return value?

Right, I'll keep that in mind.

[Kiet Ho]

Created attachment 437944 [details]
Patch

Notes:
1. I think I'll need more guidance on what to do regarding DeprecatedCSSOMPrimitive Value
2. Font::platformGlyphInit() requires both the glyph number and width of the space character, therefore even if there's a method to get the glyph number then width, we'd have to duplicate the logic to get the glyph number regardless. I decided to just clean up the method and surrounding areas.

[Kiet Ho]

Created attachment 437950 [details]
Patch

Notes:\n1. I think I'll need more guidance on what to do regarding DeprecatedCSSOMPrimitive Value\n2. Font::platformGlyphInit() requires both the glyph number and width of the space character, therefore even if there's a method to get the glyph number then width, we'd have to duplicate the logic to get the glyph number regardless. I decided to just clean up the method and surrounding areas.

[Darin Adler]

(In reply to Kiet Ho from comment #14)
> 1. I think I'll need more guidance on what to do regarding
> DeprecatedCSSOMPrimitive Value

In primitiveType, add a case to the switch statement and return CSS_UNKNOWN. In getFloatValue, getStringValue, getCounterValue, getRectValue, and getRGBColorValue, do not change the code at all.

More importantly, write a test verifying the behavior of these functions. Look at the fast/css/getFloatValueForUnit.html and the tests in fast/css/getComputedStyle and fast/css/resources/CSSPrimitiveValue-exceptions.js for examples of some tests that cover what these functions do.

[Kiet Ho]

Created attachment 439023 [details]
Patch

WIP, making sure that all layout tests pass

[Kiet Ho]

Created attachment 439032 [details]
Patch

[Kiet Ho]

I saw two things while making CSSPrimitiveValue tests:

* In CSSUnits.cpp, WebCore::unitCategory() does not return CSSUnitCategory::Length for a bunch of units (ch, rem, ...)

* CSSPrimitiveValue.getFloatValue() in JS uses CSSPrimitiveValue::doubleValueInternal() in CSSPrimitiveValue.cpp. This relies on CSSPrimitiveValue::conversionToCanonicalUnitsScaleFactor(), which does not have enough information (correct me if I'm wrong) to convert from/to ic units. Thus getFloatValue() is broken for ic.

Opinions? Should I fixed both of them?

[Simon Fraser (smfr)]

Comment on attachment 439032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439032&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:1051
>      case CSSUnitType::CSS_CHS:
>          return formatNumberValue("ch");
> +    case CSSUnitType::CSS_ICS:
> +        return formatNumberValue("ic");

The "CHS"  vs "ch" and "ICS" vs "ic" is confusing. Why aren't the unit types CSS_CH and CSS_IC?

> LayoutTests/imported/w3c/ChangeLog:8
> +

Please describe why you had to change the WPT.

[Kiet Ho]

Comment on attachment 439032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439032&action=review

>> Source/WebCore/css/CSSPrimitiveValue.cpp:1051
>> +        return formatNumberValue("ic");
> 
> The "CHS"  vs "ch" and "ICS" vs "ic" is confusing. Why aren't the unit types CSS_CH and CSS_IC?

I have no explanation other than I just followedcthe existing convention for CSS_CHS. I'll rename CSS_ICS to CSS_IC. As for CSS_CHS, maybe that's for another patch.

>> LayoutTests/imported/w3c/ChangeLog:8
>> +
> 
> Please describe why you had to change the WPT.

Yeah, I'll fill in the changelogs after everything else checks out.

[Kiet Ho]

Created attachment 439128 [details]
Patch

Should be final

[Myles C. Maxfield]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/platform/graphics/FontMetrics.h:173
> +    std::optional<float> m_ideogramWidth;

Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?

[Myles C. Maxfield]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/ChangeLog:8
> +

Ideally there would be a description of the change here.

[Kiet Ho]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>> Source/WebCore/ChangeLog:8
>> +
> 
> Ideally there would be a description of the change here.

I'll put one in, I thought the description above and changes for each method are enough.

>> Source/WebCore/platform/graphics/FontMetrics.h:173
>> +    std::optional<float> m_ideogramWidth;
> 
> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?

I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?

[Myles C. Maxfield]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>> +    std::optional<float> m_ideogramWidth;
>> 
>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
> 
> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?

That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.

If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.

[Kiet Ho]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>>> +    std::optional<float> m_ideogramWidth;
>>> 
>>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
>> 
>> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?
> 
> That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.
> 
> If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.

I agree, I'll fix it as you said.

[Kiet Ho]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>>>> +    std::optional<float> m_ideogramWidth;
>>>> 
>>>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
>>> 
>>> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?
>> 
>> That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.
>> 
>> If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.
> 
> I agree, I'll fix it as you said.

I don't think it's possible to implement the fallback behavior inside Font/FontMetrics, because the fallback metric (1em) comes from FontCascadeDescription.m_specifiedSize, which depends on whatever font size being specified in CSS. This is not available when the Font is initialized.

[Myles C. Maxfield]

Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/platform/graphics/Font.cpp:166
> +    // to support the ic unit.

I think here you would just say m_fontMetrics.setIdeogramWidth(platformData().size()); and that would end up setting it to 1em.

[Kiet Ho]

Created attachment 439579 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

> Source/WebCore/platform/graphics/Font.h:-138
> -    FontMetrics& fontMetrics() { return m_fontMetrics; }

Are these unused?

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-001-expected.html:-6
> -svg { width: 10ic; }

Are these being updated upstream too?

[Kiet Ho]

Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

>> Source/WebCore/platform/graphics/Font.h:-138
>> -    FontMetrics& fontMetrics() { return m_fontMetrics; }
> 
> Are these unused?

It appears to be the case (because we also have a const getter), but if you want to be on the safe side I'll keep it.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-001-expected.html:-6
>> -svg { width: 10ic; }
> 
> Are these being updated upstream too?

Yes, I'll upstream it when this patch passes CQ.

[EWS]

Committed r283279 (242306@main): <https://commits.webkit.org/242306@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439579 [details].

[Tim Nguyen (:ntim)]

Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-013.html:7
> +<link rel="match" href="ic-unit-013-expected.html">

You probably want to use -ref.html when you upstream, for the file names as well, to be consistent with the rest of the folder. You can probably omit putting them in the reference/ subfolder for now, for convenience.

The webkit import script takes care of renaming the references to `-expected.html`

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-014.html:7
> +<link rel="match" href="ic-unit-014-expected.html">

ditto

[Tim Nguyen (:ntim)]

Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

>> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-013.html:7
>> +<link rel="match" href="ic-unit-013-expected.html">
> 
> You probably want to use -ref.html when you upstream, for the file names as well, to be consistent with the rest of the folder. You can probably omit putting them in the reference/ subfolder for now, for convenience.
> 
> The webkit import script takes care of renaming the references to `-expected.html`

Actually, there doesn't seem any reason to not put the new refs in the reference/ folder, the test references you're adding don't use any relative URLs.