[webkit-reviews] review granted: [Bug 240422] Avoid the overhead of DrawGlyphsRecorder when drawing cached glyph buffers : [Attachment 459375] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 240422: Avoid the overhead of DrawGlyphsRecorder when drawing cached glyph
buffers
https://bugs.webkit.org/show_bug.cgi?id=240422

Attachment 459375: Patch

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




--- Comment #10 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 459375
  --> https://bugs.webkit.org/attachment.cgi?id=459375
Patch

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

I guess if you want to do it this way it's fine, but I guess I just wanted to
make sure that you thought about doing this in a way that abided by layering a
bit more strictly.

>> Source/WebCore/ChangeLog:25
>> +	      image drawing, or any double glyph drawing (which can happen with
some kind of fonts),
> 
> In what situations do we get this "double glyph drawing", and why does it
mean we can't avoid using DrawGlyphsRecorder? Is this for glyphs with multiple,
colored outlines, or something else?

I believe Simon is talking about COLRv0 glyphs here.

> Source/WebCore/ChangeLog:26
> +	     and GraphicsContext::drawGlyphs() returns a DidDecomposeGlyphs
result indicating this.

GraphicsContext seems like the wrong level of abstraction here. Is there really
no displaylist-specific way to do this?

> Source/WebCore/ChangeLog:27
> +	   * DisplayList gains an optional<bool> indicating whether any glyph
decomposition happened

DisplayList gets the optional<bool>, rather than the DisplayListReplayer?
Shouldn't DisplayLists be immutable once they are recorded?

> Source/WebCore/ChangeLog:30
> +	     on the cached display list when glyph decomposition was requested
by the destination

Did you consider adding a new entry point to the Recorder? Would such a thing
even work?

> Source/WebCore/ChangeLog:42
> +	   Display list logging currently dumps resource identifiers, which is
a problem for 
> +	   layout test because they are't necessarily stable. So we plumb
through a IncludesResourceIdentifiers
> +	   flag, which needs to propagate into all the display item logging
functions, requiring them to
> +	   converted from operator<<(TextStream&, ...) to dumpItem() functions
that take a flags argument.

Surely this could have been done as a separate patch ��

> Source/WebKit/ChangeLog:7
> +

Didn't you tell me years ago not to duplicate ChangeLog prose?

> Source/WebCore/platform/graphics/GraphicsContext.h:295
> +    WEBCORE_EXPORT virtual DidDecomposeGlyphs drawGlyphs(const Font&, const
GlyphBufferGlyph*, const GlyphBufferAdvance*, unsigned numGlyphs, const
FloatPoint&, FontSmoothingMode, std::optional<bool> needsGlyphDecomposition = {
});

I guess I said this in an earlier comment but this feels like a layering
violation.

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:408
> +    if (m_didDrawImage || m_outputGlyphCount != numGlyphs)

I think you should be trying to determine if the decomposition process is the
identity function. E.g. consider a COLRv0 glyph that has a single layer; the
number of glyphs will be equal and the drawImage flag will be false, but the
decomposition process would still have to happen (in the web process).

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:409
> +	   return GraphicsContext::DidDecomposeGlyphs::Yes;

This name is a little misleading. You're not returning information about
whether or not glyphs were decomposed - because all glyphs will get decomposed;
instead, you're returning information about whether the decomposition was the
identity function.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:691
> -TextStream& operator<<(TextStream& ts, const Translate& item)
> +void dumpItem(TextStream& ts, const Translate& item, OptionSet<AsTextFlag>)

Definitely feels like you put 2 patches inside a single patch

> LayoutTests/TestExpectations:5112
> +[ Release ] fast/text/glyph-display-list-emoji.html [ Skip ]
> +[ Release ] fast/text/glyph-display-list-svg-font.html [ Skip ]

Did you forget to add the test files?