[webkit-reviews] review granted: [Bug 189918] Refactor Editor::fontAttributesForSelectionStart to be platform-agnostic : [Attachment 350653] Fix 32-bit macOS build

[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 189918: Refactor Editor::fontAttributesForSelectionStart to be
platform-agnostic
https://bugs.webkit.org/show_bug.cgi?id=189918

Attachment 350653: Fix 32-bit macOS build

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




--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 350653
  --> https://bugs.webkit.org/attachment.cgi?id=350653
Fix 32-bit macOS build

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

> Source/WebCore/editing/Editor.cpp:3895
> +    if (verticalAlign == VerticalAlign::Sub)
> +	   attributes.subscriptOrSuperscript =
SubscriptOrSuperscript::Subscript;
> +
> +    if (verticalAlign == VerticalAlign::Super)
> +	   attributes.subscriptOrSuperscript =
SubscriptOrSuperscript::Superscript;

Should this be a switch? Or do we elsewhere have a function that translates
between the two?

(coming back to this, we don't, because it's new)

> Source/WebCore/editing/FontAttributes.h:39
> +#if PLATFORM(COCOA)
> +OBJC_CLASS NSDictionary;
> +#endif
> +
> +#if PLATFORM(MAC)
> +OBJC_CLASS NSFont;
> +#elif PLATFORM(IOS)
> +OBJC_CLASS UIFont;
> +#endif

None of these #ifs are really necessary.

> Source/WebCore/editing/FontShadow.h:47
> +    double width { 0 };
> +    double height { 0 };

FloatSize offset; ?

> Source/WebCore/editing/cocoa/FontAttributesCocoa.mm:54
> +	   [attributes setObject:font.get() forKey:NSFontAttributeName];

Modern dictionary syntax?

> Source/WebCore/editing/cocoa/FontShadowCocoa.mm:42
> +    [shadow setShadowColor:nsColor(color)];

Wouldn't it be nice if you had your platformColor thing here. I feel like we
must have something similar somewhere.