Bug 211527 – Use CocoaColor in more places instead of platform defines

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

SSIA

[Wenson Hsieh]

Created attachment 398664 [details]
Patch

[Wenson Hsieh]

Created attachment 398665 [details]
Fix bug title

[Darin Adler]

Comment on attachment 398665 [details]
Fix bug title

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

Nice clean up. This is the kind of abstraction I really like.

Not sure each of these (CocoaColor, CocoaFont/Descriptor, CocoaImage) should have its own header. Seems like they could share.

> Source/WebKit/Platform/cocoa/CocoaFont.h:29
> +@class NSFont, NSFontDescriptor;

Didn’t even know you could use commas in these! Not sure I would have.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:103
> +    if ([object isKindOfClass:[CocoaColor class]])

I’m tempted in new code to write ".class" instead.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:434
> +        encodeColorInternal(encoder, static_cast<CocoaColor *>(object));

Seems like for the future we need a Objective-C pointer cast that, at least in debug builds, checks the class.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.h:48
>  using ViewType = NSView;
>  using RectType = NSRect;

Other candidates.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:57
>  using TextViewType = NSTextView;
>  using ButtonType = NSButton;
>  using AlignmentType = NSLayoutAttribute;
>  using SizeType = NSSize;

Other candidates.

[Tim Horton]

Comment on attachment 398665 [details]
Fix bug title

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

> Source/WebKit/Platform/cocoa/CocoaFont.h:29
> +@class NSFont, NSFontDescriptor;

1) I know these aren't variable declarations but the multi-line thing still looks surprising.
2) Why not OBJC_CLASS in case these get included in C++ code? (which seems ... likely)

[Wenson Hsieh]

Comment on attachment 398665 [details]
Fix bug title

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

>>> Source/WebKit/Platform/cocoa/CocoaFont.h:29
>>> +@class NSFont, NSFontDescriptor;
>> 
>> Didn’t even know you could use commas in these! Not sure I would have.
> 
> 1) I know these aren't variable declarations but the multi-line thing still looks surprising.
> 2) Why not OBJC_CLASS in case these get included in C++ code? (which seems ... likely)

(1) We use it in a few public headers! (JSContext.h, WKDOMRange.h)
(2) Good point! I’ll change these to be OBJC_CLASS instead.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:103
>> +    if ([object isKindOfClass:[CocoaColor class]])
> 
> I’m tempted in new code to write ".class" instead.

👍🏻 Will make that change here, and below (CocoaFont.class).

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 398665 [details]
> Fix bug title
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398665&action=review
> 
> >>> Source/WebKit/Platform/cocoa/CocoaFont.h:29
> >>> +@class NSFont, NSFontDescriptor;
> >> 
> >> Didn’t even know you could use commas in these! Not sure I would have.
> > 
> > 1) I know these aren't variable declarations but the multi-line thing still looks surprising.
> > 2) Why not OBJC_CLASS in case these get included in C++ code? (which seems ... likely)
> 
> (1) We use it in a few public headers! (JSContext.h, WKDOMRange.h)
> (2) Good point! I’ll change these to be OBJC_CLASS instead.

I’m also going to change existing `@class`s in CocoaImage.h to be OBJC_CLASS as well, for consistency.

> 
> >> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:103
> >> +    if ([object isKindOfClass:[CocoaColor class]])
> > 
> > I’m tempted in new code to write ".class" instead.
> 
> 👍🏻 Will make that change here, and below (CocoaFont.class).

[Wenson Hsieh]

Comment on attachment 398665 [details]
Fix bug title

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

>>>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:103
>>>> +    if ([object isKindOfClass:[CocoaColor class]])
>>> 
>>> I’m tempted in new code to write ".class" instead.
>> 
>> 👍🏻 Will make that change here, and below (CocoaFont.class).
> 
> 

Unfortunately, I can’t make this change since CocoaColor is a “using" declaration :(

I put together this simple snippet in a standalone test case:

@class NSString;
using MyString = NSString;

static Class foo()
{
    return MyString.class;
}

…attempting to compile this results in:

/Volumes/…/foo.mm:16:12: error: expected identifier or '('
    return MyString.class;
           ^
1 error generated.

[Wenson Hsieh]

Created attachment 398677 [details]
Patch for landing

[EWS]

Patch 398677 does not build

[Wenson Hsieh]

Created attachment 398681 [details]
Patch for landing

[EWS]

Committed r261259: <https://trac.webkit.org/changeset/261259>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398681 [details].