[webkit-reviews] review granted: [Bug 234757] Refactor some Cocoa-specific code in WebCore::FontAttributes to be platform-agnostic : [Attachment 448114] Fix Windows build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 234757: Refactor some Cocoa-specific code in WebCore::FontAttributes to be
platform-agnostic
https://bugs.webkit.org/show_bug.cgi?id=234757

Attachment 448114: Fix Windows build

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 448114
  --> https://bugs.webkit.org/attachment.cgi?id=448114
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.