Bug 234171

Created attachment 446801 [details] Patch

<rdar://problem/70166552>

Created attachment 446802 [details] Patch

Created attachment 446809 [details] Patch

Created attachment 446811 [details] Patch

Comment on attachment 446811 [details] 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)

Comment on attachment 446811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446811&action=review >> 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>`? CompatibleBufferDescription is defined inside ImageBuffer, and C++ says the scope starts at the word "ImageBuffer::" so because this is on the left of that it would have had to be std::optional<ImageBuffer::CompatibleBufferDescription>. Moving it to the right side means you can drop the ImageBuffer:: >> Source/WebCore/platform/graphics/ImageBuffer.cpp:207 >> + // 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 Good point! Yep, I'll fix it. >> 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]`? It doesn't, because it doesn't use m_internalContext. This draws into the ImageBuffer's context instead. These parameters are pointer type. Do you think &(glyphs[i]) is more readable than glyphs + 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) Let's chat offline about this. I'm curious more more details. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:411 >> + bool state = false; > > NIT: How about `isOTSVGRun`? 🆒

Committed r286889 (245117@trunk): <https://commits.webkit.org/245117@trunk>

Committed r286906 (?): <https://commits.webkit.org/r286906>

Comment on attachment 446811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446811&action=review > Source/WebCore/platform/graphics/ImageBuffer.cpp:225 > +} Maybe I misunderstand something here. But my understanding is this function returns 1. The size of the ImageBuffer in backend coordinates after clamping it if needed 2. The enclosing rectangle of the ImageBuffer in logical coordinates 3. The scaling factor which is the context scaling factor + the clamping scaling factor if applied. If my understanding is correct, I think this function can be written like this: static CompatibleBufferInfo compatibleBufferInfo(const FloatRect& rect, const GraphicsContext& context) { auto scaleFactor = context.scaleFactor(); ImageBuffer::sizeNeedsClamping(rect.size(), scaleFactor); auto backendSize = rect.size() * scaleFactor; return { expandedIntSize(backendSize), enclosingIntRect(rect), scaleFactor }; } In fact I think the function and the struct CompatibleBufferInfo are not needed. These few lines can be moved to the only caller createCompatibleBuffer(). > Source/WebCore/platform/graphics/ImageBuffer.h:76 > + struct CompatibleBufferInfo { > + IntSize physicalSizeInDeviceCoordinates; > + FloatRect inflatedRectInUserCoordinates; > + FloatSize scale; > + }; > + static CompatibleBufferInfo compatibleBufferInfo(const FloatRect&, const GraphicsContext&); This function and this struct can be moved to ImageBuffer.cpp since they are only used internally by ImageBuffer.