Bug 234757 - Refactor some Cocoa-specific code in WebCore::FontAttributes to be platform-agnostic
Summary: Refactor some Cocoa-specific code in WebCore::FontAttributes to be platform-a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-30 14:26 PST by Wenson Hsieh
Modified: 2021-12-31 15:36 PST (History)
14 users (show)

See Also:


Attachments
For EWS (32.03 KB, patch)
2021-12-30 14:45 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (32.06 KB, patch)
2021-12-30 14:52 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Soup and Playstation builds (34.64 KB, patch)
2021-12-30 15:21 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (34.50 KB, patch)
2021-12-30 16:16 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix Windows build (35.72 KB, patch)
2021-12-30 16:44 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (37.44 KB, patch)
2021-12-31 13:25 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-12-30 14:26:13 PST
Some minor cleanup, to support subsequent patches around this area.
Comment 1 Wenson Hsieh 2021-12-30 14:45:40 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-12-30 14:52:50 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-12-30 15:21:14 PST Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-12-30 16:16:58 PST Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2021-12-30 16:44:18 PST
Created attachment 448114 [details]
Fix Windows build
Comment 6 Darin Adler 2021-12-30 17:41:41 PST
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.
Comment 7 Wenson Hsieh 2021-12-31 12:32:21 PST
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.
Comment 8 Wenson Hsieh 2021-12-31 13:25:57 PST
Created attachment 448132 [details]
For EWS
Comment 9 EWS 2021-12-31 15:35:00 PST
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].
Comment 10 Radar WebKit Bug Importer 2021-12-31 15:36:29 PST
<rdar://problem/87036610>