[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 #12 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/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?

-- 
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/eda5dd78/attachment-0001.htm>