[webkit-reviews] review canceled: [Bug 206118] [GPU Process] Support drawing text in 2D canvas with font features : [Attachment 387447] Adjust Win TestExpectations

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 206118: [GPU Process] Support drawing text in 2D canvas with font features
https://bugs.webkit.org/show_bug.cgi?id=206118

Attachment 387447: Adjust Win TestExpectations

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




--- Comment #6 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 387447
  --> https://bugs.webkit.org/attachment.cgi?id=387447
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.