Bug 188955

Created attachment 348104 [details] Patch

Attachment 348104 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebPageProxy.cpp:7329: Declaration has space between type name and * in NSObject *WebPageProxy [whitespace/declaration] [3] Total errors found: 1 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.

Looks like it’s compiling and passing tests everywhere.

Comment on attachment 348104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348104&action=review > Source/WebKit/Shared/API/c/cf/WKStringCF.mm:55 > + CFRetain(cfString); > + return toAPI(static_cast<API::String*>(&[(WKNSString *)(__bridge NSString *)cfString _apiObject])); I'd put the CFRetain inside the toAPI call instead of on a separate line. > Source/WebKit/Shared/API/c/cf/WKURLCF.mm:58 > + CFRetain(cfURL); Same here. > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2054 > - [[event retain] autorelease]; > + CFRetain((__bridge CFTypeRef)event); > + CFAutorelease((__bridge CFTypeRef)event); I seem to remember that you can do something like __autoreleasing NSEvent *autoreleasedEvent = event; (I think we've done something similar in Safari). > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2074 > - [[event retain] autorelease]; > + CFRetain((__bridge CFTypeRef)event); > + CFAutorelease((__bridge CFTypeRef)event); Same thing here.

Comment on attachment 348104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348104&action=review >> Source/WebKit/Shared/API/c/cf/WKStringCF.mm:55 >> + return toAPI(static_cast<API::String*>(&[(WKNSString *)(__bridge NSString *)cfString _apiObject])); > > I'd put the CFRetain inside the toAPI call instead of on a separate line. Done. The reason I didn’t do that originally is that I wanted a construction where the __bridge cast will warn if we use the wrong toll-free bridged type and I was worried that CFRetain would cause the expression to dumb down its type from CFStringRef to CFTypeRef. But frankly I have no idea if it will do that or not, and I also guess the warning is not so important that I should work so hard to preserve it. >> Source/WebKit/Shared/API/c/cf/WKURLCF.mm:58 >> + CFRetain(cfURL); > > Same here. Done. >> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2054 >> + CFAutorelease((__bridge CFTypeRef)event); > > I seem to remember that you can do something like > > __autoreleasing NSEvent *autoreleasedEvent = event; > > (I think we've done something similar in Safari). I like the idea of moving to a better idiom. The thing I recall in Safari is a macro: #define AUTORELEASING_PROTECT(object) ({ __autoreleasing volatile id protector = object; (void)protector; }) I am not sure that’s better than explicit calls to CFRetain and CFAutorelease. I’d be happy to switch idioms to something we like better: It will be easy to find all the call sites by searching for CFAutorelease; so far there are only 4, these 3 in WebKit and 1 I already landed in WebCore. For now I’d prefer to land these as just CFRetain/CFAutorelease since I’m not sure I prefer an idiom involving volatile and unused local variables.

Committed r235365: <https://trac.webkit.org/changeset/235365>

<rdar://problem/43755547>