Bug 240422

Created attachment 459373 [details]
Patch

Created attachment 459374 [details]
Patch

Created attachment 459375 [details]
Patch

Comment on attachment 459375 [details]
Patch

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

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

Comment on attachment 459375 [details]
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?

Created attachment 459501 [details]
Patch

<rdar://problem/93710477>

Subsumed by bug 240497.