[webkit-reviews] review granted: [Bug 234171] [Cocoa] OT-SVG glyphs don't draw into canvases (because of the GPU process) : [Attachment 446811] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 234171: [Cocoa] OT-SVG glyphs don't draw into canvases (because of the GPU
process)
https://bugs.webkit.org/show_bug.cgi?id=234171

Attachment 446811: Patch

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




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 446811
  --> https://bugs.webkit.org/attachment.cgi?id=446811
Patch

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

r=me, nice work!  I won't claim to fully understand the motivation, but the
logic looks sound :)

> Source/WebCore/platform/graphics/ImageBuffer.cpp:101
> +auto ImageBuffer::createCompatibleBuffer(const FloatRect& rect, const
GraphicsContext& context) -> std::optional<CompatibleBufferDescription>

Aside: Why use `auto` instead of `std::optional<CompatibleBufferDescription>`?

> Source/WebCore/platform/graphics/ImageBuffer.cpp:195
> +auto ImageBuffer::compatibleBufferInfo(const FloatRect& rect, const
GraphicsContext& context) -> CompatibleBufferInfo

ditto (:101)

> Source/WebCore/platform/graphics/ImageBuffer.cpp:207
> +    // In practice, this should almost never happen - individual glyphs
inside text pretty much
> +    // never end up this big.

NIT: this is assuming that this function is only ever called when dealing with
glyphs, but there's nothing in the header that indicates that

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:379
> +	   if (auto imageBufferInfo =
ImageBuffer::createCompatibleBuffer(bounds, m_owner)) {

NIT: Should this be `imageBufferDescription` since
`ImageBuffer::createCompatibleBuffer` returns a
`std::optional<CompatibleBufferDescription>`, not a `CompatibleBufferInfo`?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:380
> +	       FontCascade::drawGlyphs(imageBufferInfo->imageBuffer->context(),
font, glyphs + i, advances + i, 1, FloatPoint(), smoothingMode);

Does this also need a `prepareInternalContext(font, smoothingMode);` and
`concludeInternalContext();` somewhere? 

Style: Why not `glyphs[i]` and `advances[i]`?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:398
>      auto otsvgGlyphs = font.findOTSVGGlyphs(glyphs, numGlyphs);

Aside: as a (future) optimization it would be nice to avoid this and instead
only have to do one iteration of `glyphs` (i.e. add a
`Font::isOTSVGGlyph(GlyphBufferGlyph)` that you call in the `for` below)

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:411
> +    bool state = false;

NIT: How about `isOTSVGRun`?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:417
> +	       drawOTSVGRun(font, glyphs + i - glyphCountInRun, advances + i -
glyphCountInRun, glyphCountInRun, runOrigin, smoothingMode);

ditto (:380)

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:419
> +	       drawNonOTSVGRun(font, glyphs + i - glyphCountInRun, advances + i
- glyphCountInRun, glyphCountInRun, runOrigin, smoothingMode);

ditto (:380)