Bug 234757 – Refactor some Cocoa-specific code in WebCore::FontAttributes to be platform-agnostic

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

Some minor cleanup, to support subsequent patches around this area.

[Wenson Hsieh]

Created attachment 448107 [details]
For EWS

[Wenson Hsieh]

Created attachment 448108 [details]
For EWS

[Wenson Hsieh]

Created attachment 448109 [details]
Fix Soup and Playstation builds

[Wenson Hsieh]

Created attachment 448110 [details]
For EWS

[Wenson Hsieh]

Created attachment 448114 [details]
Fix Windows build

[Darin Adler]

Comment on attachment 448114 [details]
Fix Windows build

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

> Source/WebCore/ChangeLog:35
> +        * platform/graphics/cocoa/FontCocoa.h: Added.
> +
> +        Moved here from CocoaFont.h in WebKit. See WebKit/ChangeLog for more details.

Could have sworn Sam Weinig just did something like this in a patch.

> Source/WebCore/editing/Editor.cpp:4044
> +    bool hasMultipleFonts = false; // FIXME: This will be included as a part of FontAttributes in a future patch.

Not sure I understand the FIXME, but sounds like work in the right direction.

> Source/WebCore/editing/cocoa/FontAttributesCocoa.mm:91
> +    if (auto cocoaFont = font ? (__bridge CocoaFont *)font->getCTFont() : nil)

Suprised that WebCore::Font’s function to convert to a CoreText font is named getCTFont since WebKit coding style calls for not using "get" in this kind of function name.

Also, I think the knowledge that the two types are toll-free bridged could also be in WebCore::Font as a separate function so we would not need the bridge cast here. That’s the pattern we follow with WTF::String and WTF::URL, for example, except that the idiom there uses autorelease and here we would not need to do that. I think there should be function named cocoaFont() that returns a CocoaFont *.

> Source/WebKit/Shared/Cocoa/CoreTextHelpers.mm:33
> +using namespace WebCore;

I suggest we add the WebCore prefixes rather than adding the "using namespace WebCore".

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2915
> +    encoder << !!attributes.font;
> +    if (attributes.font)
> +        encoder << Ref(*attributes.font);

Seems strange that we have to write this out and don’t have a RefPtr encoder that does this for us. And, that we have to put a Font& into a Ref<Font> to encode it.

[Wenson Hsieh]

Comment on attachment 448114 [details]
Fix Windows build

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

Thanks for the review!

>> Source/WebCore/ChangeLog:35
>> +        Moved here from CocoaFont.h in WebKit. See WebKit/ChangeLog for more details.
> 
> Could have sworn Sam Weinig just did something like this in a patch.

Indeed! (I took a bit of inspiration from his recent "CocoaColor.h in WebKit" => "ColorCocoa.h in WebCore" refactoring)

>> Source/WebCore/editing/Editor.cpp:4044
>> +    bool hasMultipleFonts = false; // FIXME: This will be included as a part of FontAttributes in a future patch.
> 
> Not sure I understand the FIXME, but sounds like work in the right direction.

Yes — I'll clarify this FIXME by referencing the bug (#190120). My intention is to add `hasMultipleFonts` as a member of `FontAttributes` and plumb it across to the client layer in the next patch.

>> Source/WebCore/editing/cocoa/FontAttributesCocoa.mm:91
>> +    if (auto cocoaFont = font ? (__bridge CocoaFont *)font->getCTFont() : nil)
> 
> Suprised that WebCore::Font’s function to convert to a CoreText font is named getCTFont since WebKit coding style calls for not using "get" in this kind of function name.
> 
> Also, I think the knowledge that the two types are toll-free bridged could also be in WebCore::Font as a separate function so we would not need the bridge cast here. That’s the pattern we follow with WTF::String and WTF::URL, for example, except that the idiom there uses autorelease and here we would not need to do that. I think there should be function named cocoaFont() that returns a CocoaFont *.

Makes sense! I filed <https://bugs.webkit.org/show_bug.cgi?id=234769>.

>> Source/WebKit/Shared/Cocoa/CoreTextHelpers.mm:33
>> +using namespace WebCore;
> 
> I suggest we add the WebCore prefixes rather than adding the "using namespace WebCore".

Changed to use WebCore:: prefixes.

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2915
>> +        encoder << Ref(*attributes.font);
> 
> Seems strange that we have to write this out and don’t have a RefPtr encoder that does this for us. And, that we have to put a Font& into a Ref<Font> to encode it.

Yeah, it sounds like we could at least add templated RefPtr encoder/decoder methods that know how to encode/decode RefPtr<T>, as long as there's already an encoder for Ref<T>.

I think the reason for encoding with Ref<Font> instead of Font& (or even const Font&) is that the canonical ArgumentCoder template assumes that the type when encoding matches the result when decoding:

```
template<> struct ArgumentCoder<T> {
    static void encode(Encoder&, const T&);
    static std::optional<T> decode(Decoder&);
};
```

...whereas in this case, it would be more ideal to be a bit less symmetric by encoding with `const Font&` but decoding to a `Ref<Font>`... i.e. something like this:

```
template<> struct ArgumentCoder<Ref<T>> {
    static void encode(Encoder&, const T&);
    static std::optional<Ref<T>> decode(Decoder&);
};
```

I think for this patch, I'll add a `RefPtr<Font>` encoder/decoder that would allow us to just do `encoder << attributes.font` instead of having to encode the nullity bit separately in the `FontAttributes` coder. I'll file a followup to then see if we can add something to ArgumentCoders.h so that we get `RefPtr<T>` coding for free if `Ref<T>` coders are defined.

[Wenson Hsieh]

Created attachment 448132 [details]
For EWS

[EWS]

Committed r287496 (245631@main): <https://commits.webkit.org/245631@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448132 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87036610>