[Webkit-unassigned] [Bug 204276] Implement the 'ic' unit from CSS Values 4
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 9 11:02:46 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=204276
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |darin at apple.com
--- Comment #7 from Darin Adler <darin 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/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 "{ }".
--
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/20210909/3634716a/attachment-0001.htm>
More information about the webkit-unassigned
mailing list