[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 #13 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/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.

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