Bug 221744

Created attachment 419948 [details] WIP

<rdar://problem/74222334>

To-do: 1. Figure out how to make it build on non-internal OSes 2. Add ASSERT_WITH_SECURITY_IMPLICATION()s to make sure the GPUP doesn't parse OT-SVG content 3. Split this patch into two: one for the refactoring, and one for the actual change We probably don't want to add any tests, because we don't want to guarantee that OT-SVG doesn't work... Instead, we would just temporarily disable any existing tests we have that make sure that drawing OT-SVG into canvas *does* work.

Created attachment 420036 [details] WIP

isInGPUProcess()

and RELEASE_ASSERT() is better to use here than ASSERT_WITH_SECURITY_IMPLICATION().

Created attachment 420126 [details] Patch

Created attachment 420128 [details] Patch

Created attachment 420137 [details] Patch

Created attachment 420193 [details] Patch

Created attachment 420211 [details] Patch

Created attachment 420213 [details] Patch

Created attachment 420234 [details] Patch

Created attachment 420251 [details] Patch

Created attachment 420254 [details] Patch

Created attachment 420262 [details] Patch

Created attachment 420263 [details] Patch

Created attachment 420264 [details] Patch

Comment on attachment 420264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420264&action=review Presumably this will cause some new test failures in the GPUProcess bot? > Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:197 > + for (unsigned i = 0; i < numGlyphs; ++i) > + RELEASE_ASSERT(!font.glyphIsRenderedWithOTSVG(glyphs[i])); Wouldn't it be much more efficient to pass an array of glyphs to glyphIsRenderedWithOTSV()? This is also adding an extra traversal of the glyph array even in release builds.

Created attachment 420348 [details] Patch

Created attachment 420359 [details] Patch

Created attachment 420399 [details] Patch

Created attachment 420401 [details] Patch

Created attachment 420421 [details] Patch

Created attachment 420426 [details] Patch

Comment on attachment 420426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420426&action=review > Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:34 > +OTSVGTable::OTSVGTable() > +{ > +} = default > Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:44 > + other.table = nullptr; isn't this what std::exchange is for? > Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:58 > + table = other.table; > + other.table = nullptr; std::exchange (or std::swap I forget) > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:785 > + m_otSVGTable = PAL::OTSVGTable(); = { }? > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:793 > + if (PAL::softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph(m_otSVGTable.value().table, glyphs[i]) != kCFNotFound) { Is there any perf hit for calling softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph every time inside the loop? > Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp:376 > +struct GlyphsAndAdvancesStorage { > + Vector<GlyphBufferGlyph> glyphs; > + Vector<GlyphBufferAdvance> advances; > +}; > +struct GlyphsAndAdvances { > + const GlyphBufferGlyph* glyphs; > + const GlyphBufferAdvance* advances; > + unsigned numGlyphs; > + GlyphBufferAdvance initialAdvance; > + Optional<GlyphsAndAdvancesStorage> storage; > +}; > +static GlyphsAndAdvances filterOutOTSVGGlyphs(const Font& font, const GlyphBufferGlyph* glyphs, const GlyphBufferAdvance* advances, unsigned numGlyphs) I'd like a few more blank lines here.

Created attachment 420588 [details] Patch for committing

Created attachment 420800 [details] Patch for committing

The Windows errors look unrelated. I'll land tonight and watch the bots to make sure nothing bad happens.

Committed r273121 (234318@main): <https://commits.webkit.org/234318@main>

Comment on attachment 420426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420426&action=review >> Source/WebCore/PAL/pal/cf/OTSVGTable.cpp:44 >> + other.table = nullptr; > > isn't this what std::exchange is for? std::swap() >> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:793 >> + if (PAL::softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph(m_otSVGTable.value().table, glyphs[i]) != kCFNotFound) { > > Is there any perf hit for calling softLinkOTSVGOTSVGTableGetDocumentIndexForGlyph every time inside the loop? Do you mean a perf hit because the function is soft linked, or a perf hit because we have to call it at all? If you mean because it's soft linked, the macro is actually smart enough to only do the soft linking stuff the first time it's called. The function pointer initially gets set to a stub which will look up the real function and then assign it to the function pointer. So the runtime cost for soft-linking is minimal. If you mean because we have to call it at all, we only call it for SVG fonts (see line 788 above), and almost no fonts are SVG fonts. So we're almost never going to run this code.