[Webkit-unassigned] [Bug 204276] Implement the 'ic' unit from CSS Values 4

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=204276

--- Comment #10 from Kiet Ho <tho22 at apple.com> ---
Comment on attachment 437725
  --> https://bugs.webkit.org/attachment.cgi?id=437725
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210910/bdbf8ed6/attachment.htm>