[webkit-reviews] review granted: [Bug 190119] [Cocoa] Add SPI to expose typing attributes at the current selection on WKWebView : [Attachment 351547] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 190119: [Cocoa] Add SPI to expose typing attributes at the current
selection on WKWebView
https://bugs.webkit.org/show_bug.cgi?id=190119

Attachment 351547: Patch

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




--- Comment #5 from Tim Horton <thorton at apple.com> ---
Comment on attachment 351547
  --> https://bugs.webkit.org/attachment.cgi?id=351547
Patch

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

> Source/WebKit/ChangeLog:9
> +	   Add support new WebKit2 SPI to notify the UI delegate about font
attributes when the editor state changes (e.g.

"add support new"?

> Source/WebKit/Shared/EditorState.cpp:141
> +    encoder << !!fontAttributes;

Don't we have the ability to directly encode optionals now?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1255
> +    // FIXME: We should either rename -_webView:editorStateDidChange: to
clarify that it's only intended for use when testing,
> +    // or remove it entirely and use -_webView:fontAttributesDidChange:
instead once text alignment is supported in the set of

Are there any clients other than our testing code? If not, maybe just rename it
right away?

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:156
> +    m_delegateMethods.webViewFontAttributesDidChange = [delegate
respondsToSelector:@selector(_webView:fontAttributesDidChange:)];

I think "webView:didChangeFontAttributes:" is more traditional style, but I see
you were copying an existing method.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:94
> +enum class Nullability : uint8_t { Null, NonNull };

Is this about nullability (can it be null) or nullity (is it null)