Bug 189918 – Refactor Editor::fontAttributesForSelectionStart to be platform-agnostic

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Work towards <rdar://problem/44648705>

[Wenson Hsieh]

Created attachment 350646 [details]
Patch

[Wenson Hsieh]

Created attachment 350651 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 350653 [details]
Fix 32-bit macOS build

[Tim Horton]

Comment on attachment 350653 [details]
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.

[Wenson Hsieh]

Comment on attachment 350653 [details]
Fix 32-bit macOS build

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

>> Source/WebCore/editing/Editor.cpp:3895
>> +        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)

Interestingly enough, it was a switch before the move! I made this just use if statements instead because I thought it make it clearer that it's just these two alignment types that make their way into FontAttributes — I'll turn this back into a switch statement.

>> Source/WebCore/editing/FontAttributes.h:39
>> +#endif
> 
> None of these #ifs are really necessary.

Good point — removed.

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

👍

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

👍

(Also, I made attributes a NSMutableDictionary * instead of a RetainPtr<NSMutableDictionary>, since RetainPtr doesn't support operator[])

>> 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.

Now that you mention it, HTMLConverter does a similar dance, but uses _disambiguated_due_to_CIImage_colorWithCGColor instead (which behaves identically to colorWithCGColor, and as its name suggests, seems to exist to avoid a name conflict with CIColor). Funny enough, the helper there is also called "_platformColor".

I think we should abstract this out into a separate helper so we can use it from these call sites, but I'm not sure where to put it. Perhaps a new ColorCocoa.h?

[Wenson Hsieh]

Created attachment 350681 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 350692 [details]
Patch for EWS #2

[Wenson Hsieh]

Created attachment 350693 [details]
Patch for EWS #2.1

[Wenson Hsieh]

Created attachment 350697 [details]
Patch for EWS #2.1 (rebase on trunk)

[WebKit Commit Bot]

Comment on attachment 350697 [details]
Patch for EWS #2.1 (rebase on trunk)

Clearing flags on attachment: 350697

Committed r236445: <https://trac.webkit.org/changeset/236445>