SSIA
Created attachment 398664 [details] Patch
Created attachment 398665 [details] Fix bug title
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.
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)
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).
(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).
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.
Created attachment 398677 [details] Patch for landing
Patch 398677 does not build
Created attachment 398681 [details] Patch for landing
Committed r261259: <https://trac.webkit.org/changeset/261259> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398681 [details].