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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=226159

Attachment 429629: Patch

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




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

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
glyphCount);

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
clampTo<destination-integer-type>.

> 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.