Bug 206118

<rdar://problem/58498572>

Created attachment 387447 [details] Adjust Win TestExpectations

Comment on attachment 387447 [details] Adjust Win TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=387447&action=review > Source/WebCore/ChangeLog:10 > + Augments and refactors some existing logic for propagating font information to the GPU process when painting > + glyphs using display lists, such that we preserve font feature settings. See below for more details. I'd appreciate a little explanation about why FontCreationParameters, Font, FontHandle (and possibly FontDescription) are different classes. What is the purpose each one needs to serve? > Source/WebCore/loader/cache/CachedFont.cpp:131 > + font->setFontCreationParameters({ *m_data, fontDescription, remoteURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceCapabilities }); Please consider putting all this data inside FontPlatformData instead of Font. That way this code wouldn't have to specify it twice. The existing code maintains the invariant that Font isn't allowed to have any members that aren't derivable from the FontPlatformData. So, if this stuff were retained inside the FontPlatformData, Font could just reach inside whenever it needs it. This seems like a fairly arbitrary place to pack/unpack a FontCreationParameters. I'd recommend either making FontCreationMembers a member field of FontPlatformData, or doing the packing/unpacking as late as possible inside code that knows about the GPU Process (and just packs the data as late as possible, just to send across the wire to the other process) > Source/WebCore/platform/graphics/Font.cpp:720 > + auto customFontData = CachedFont::createCustomFontData(parameters.data, { }, wrapping); > + font = Font::create(CachedFont::platformDataFromCustomData(*customFontData, parameters.description, parameters.syntheticBold, parameters.syntheticItalic, parameters.features, parameters.capabilities), Font::Origin::Remote); Is this code only used for web fonts? If so, can we move it closer to FontCustomPlatformData? If not, shouldn't serializing an installed font use the same infrastructure as serializing a web font? > Source/WebCore/platform/graphics/Font.h:222 > + bool hasFontCreationParameters() const { return m_fontCreationParameters.hasValue(); } Installed fonts need these creation parameters, too. Shouldn't all Fonts get these creation parameters? Why are they optional? > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1156 > + return FontHandle(); I don't understand. Under which situation would we receive a FontHandle but want it to be internally null? > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3143 > + decoder >> description; This just works??? What information from the FontDescription actually gets serialized?

Comment on attachment 387447 [details] Adjust Win TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=387447&action=review >> Source/WebCore/ChangeLog:10 >> + glyphs using display lists, such that we preserve font feature settings. See below for more details. > > I'd appreciate a little explanation about why FontCreationParameters, Font, FontHandle (and possibly FontDescription) are different classes. What is the purpose each one needs to serve? Okay! I'll add some more words to the ChangeLog. Here's my take: - Font is the information used by (among other things) graphics code to paint glyphs. - FontHandle is a convenience wrapper for serializing and deserializing a Font (so if I have a Ref<Font> or RefPtr<Font>, it should be easy to serialize, e.g. over IPC). - As explained below, FontCreationParameters encapsulates the information needed to construct a Font (which mostly ends up being information needed to construct a FontPlatformData). >> Source/WebCore/loader/cache/CachedFont.cpp:131 >> + font->setFontCreationParameters({ *m_data, fontDescription, remoteURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceCapabilities }); > > Please consider putting all this data inside FontPlatformData instead of Font. That way this code wouldn't have to specify it twice. > > The existing code maintains the invariant that Font isn't allowed to have any members that aren't derivable from the FontPlatformData. So, if this stuff were retained inside the FontPlatformData, Font could just reach inside whenever it needs it. > > This seems like a fairly arbitrary place to pack/unpack a FontCreationParameters. I'd recommend either making FontCreationMembers a member field of FontPlatformData, or doing the packing/unpacking as late as possible inside code that knows about the GPU Process (and just packs the data as late as possible, just to send across the wire to the other process) Now that you pointed it out, it does seem like what I really wanted to serialize was the FontPlatformData itself. This would mean I wouldn't need a separate FontCreationParameters/FontCreationMembers class, and I think it would work for installed fonts too. I'll try this approach out and see, thanks! >> Source/WebCore/platform/graphics/Font.cpp:720 >> + font = Font::create(CachedFont::platformDataFromCustomData(*customFontData, parameters.description, parameters.syntheticBold, parameters.syntheticItalic, parameters.features, parameters.capabilities), Font::Origin::Remote); > > Is this code only used for web fonts? If so, can we move it closer to FontCustomPlatformData? If not, shouldn't serializing an installed font use the same infrastructure as serializing a web font? Yes, it's only for web fonts; I will move it over to FontCustomPlatformData. >> Source/WebCore/platform/graphics/Font.h:222 >> + bool hasFontCreationParameters() const { return m_fontCreationParameters.hasValue(); } > > Installed fonts need these creation parameters, too. Shouldn't all Fonts get these creation parameters? Why are they optional? Ok — I'll look into plumbing this information for installed fonts. For installed fonts, we currently just use the serialized platform font descriptor to send the information across, but it seems this is not enough. >> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1156 >> + return FontHandle(); > > I don't understand. Under which situation would we receive a FontHandle but want it to be internally null? FontHandle is an encodable wrapper around a nullable Font; that said, there's no situation currently where a FontHandle should contain a null font, so it's purely for convenience. The alternative would be defining functions like void encode(Encoder&, Font*) and Optional<RefPtr<Font>> decode(Decoder&). >> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3143 >> + decoder >> description; > > This just works??? What information from the FontDescription actually gets serialized? All of the information in FontDescription gets serialized.

Created attachment 409971 [details] WIP

This patch isn't quite sufficient. We need to add CreationParameters to the FontPlatformData in more places (e.g. CachedSVGFont).

(In reply to Myles C. Maxfield from comment #8) > This patch isn't quite sufficient. We need to add CreationParameters to the > FontPlatformData in more places (e.g. CachedSVGFont). Turns out it looks like all the calls get filtered through createFontCustomPlatformData(), which means it actually is sufficient.

Created attachment 410094 [details] Patch

Created attachment 410193 [details] Patch

Created attachment 410269 [details] Patch

Comment on attachment 410269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410269&action=review > Source/WebCore/platform/graphics/mac/FontCustomPlatformData.h:31 > + Nit - stray whitespace. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1185 > + encoder << static_cast<uint8_t>(font->origin()); Nit - do we need all these static casts? IIRC for enums that have an explicit bit width, our Encoder/Decoders should know how to encode/decode them, so on the decoding side we can just have `Optional<Origin>`.

(In reply to Wenson Hsieh from comment #13) > Comment on attachment 410269 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410269&action=review > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1185 > > + encoder << static_cast<uint8_t>(font->origin()); > > Nit - do we need all these static casts? IIRC for enums that have an > explicit bit width, our Encoder/Decoders should know how to encode/decode > them, so on the decoding side we can just have `Optional<Origin>`. I tried the naive thing, but it resulted in a compile error. You must be right, though - there's gotta be a way that does proper bounds checking on enums. I'll see if I can figure it out.

Created attachment 410287 [details] Patch for committing

Created attachment 410288 [details] Patch for committing

Committed r267864: <https://trac.webkit.org/changeset/267864> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410288 [details].

(In reply to EWS from comment #17) > Committed r267864: <https://trac.webkit.org/changeset/267864> This commit added two broken tests: - fast/canvas/fill-text-with-font-variations.html broken on iOS and windows - fast/canvas/fill-text-with-font-features.html broken on windows EWS indicates similar failures on previous versions of patches on this bug. Should have waited for EWS to complete before marking cq+.

Re-opened since this is blocked by bug 217220

Created attachment 410339 [details] Patch for committing

Windows failure is caused by https://bugs.webkit.org/show_bug.cgi?id=160512

Created attachment 410381 [details] Patch for committing

Committed r267930: <https://trac.webkit.org/changeset/267930> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410381 [details].

*** Bug 208878 has been marked as a duplicate of this bug. ***