Bug 188955 – [Cocoa] Adapt more WebKit code to be ARC-compatible

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Darin Adler]

[Cocoa] Adapt more WebKit code to be ARC-compatible

[Darin Adler]

Created attachment 348101 [details]
Patch

[EWS Watchlist]

Attachment 348101 [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.

[Darin Adler]

Created attachment 348104 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Looks like it’s compiling and passing tests everywhere.

[Anders Carlsson]

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.

[Darin Adler]

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.

[Darin Adler]

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

[Radar WebKit Bug Importer]

<rdar://problem/43755547>