[webkit-reviews] review granted: [Bug 226159] prevent GPU process from rendering sbix glyphs : [Attachment 429629] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 25 13:27:09 PDT 2021

Darin Adler <darin at apple.com> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 226159: prevent GPU process from rendering sbix glyphs

Attachment 429629: Patch


--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 429629
  --> https://bugs.webkit.org/attachment.cgi?id=429629

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

> Source/WebCore/platform/graphics/Font.h:266
> +	   static ComplexColorFormatGlyphs withNoRelevantTables();
> +	   static ComplexColorFormatGlyphs withRelevantTables(unsigned

Naming here is creative, but a bit peculiar. I am OK with it, but might prefer
more straight ahead names that include the word "create".

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:828
> +    auto sbixTableData = adoptCF(CTFontCopyTable(platformData().ctFont(),
kCTFontTableSbix, kCTFontTableOptionNoOptions));
> +    if (sbixTableData)
> +	   return true;

Put it inside the if please?

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:839
> +	       CFIndex glyphCount = CTFontGetGlyphCount(getCTFont());
> +	       m_glyphsWithComplexColorFormat =
ComplexColorFormatGlyphs::withRelevantTables(std::max(glyphCount, 0L));

Not sure that calling std::max does what we want here if the glyph count is
negative. Understand the desire to not pass a negative number, but passing 0
may not be OK.

Not great to have this code depend on "0L" matching CFIndex. If we do want to
clamp to zero, then more elegant better idiom would be to use

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:848
> +    if (OTSVGTableRef svgTable = otSVGTable().table) {

I like auto for a case like this.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:866
> +    OTSVGTableRef table = otSVGTable().table;

I like auto in a place like this.

More information about the webkit-reviews mailing list