Work towards <rdar://problem/44648705>
Created attachment 350646 [details] Patch
Created attachment 350651 [details] Rebase on trunk
Created attachment 350653 [details] Fix 32-bit macOS build
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.
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?
Created attachment 350681 [details] Patch for EWS
Created attachment 350692 [details] Patch for EWS #2
Created attachment 350693 [details] Patch for EWS #2.1
Created attachment 350697 [details] Patch for EWS #2.1 (rebase on trunk)
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>