Bug 115307 – Move from constructor and member function adoptCF/NS to free function adoptCF/NS.

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

[Darin Adler]

Move from constructor and member function adoptCF/NS to free function adoptCF/NS.

[Darin Adler]

Created attachment 199909 [details]
Patch

[Geoffrey Garen]

Comment on attachment 199909 [details]
Patch

r=me

[Sam Weinig]

Comment on attachment 199909 [details]
Patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:200
> -        RetainPtr<CFStringRef> supportedType(AdoptCF, reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));
> +        RetainPtr<CFStringRef> supportedType = adoptCF(reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));

Is this one right? It's not a Copy or Create.

> Source/WebCore/platform/MIMETypeRegistry.cpp:287
> -        RetainPtr<CFStringRef> supportedType(AdoptCF, reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));
> +        RetainPtr<CFStringRef> supportedType = adoptCF(reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));

Same question.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:167
> -        m_animation.adoptNS([[CABasicAnimation animationWithKeyPath:keyPath] retain]);
> +        m_animation = adoptNS([[CABasicAnimation animationWithKeyPath:keyPath] retain]);
>      else
> -        m_animation.adoptNS([[CAKeyframeAnimation animationWithKeyPath:keyPath] retain]);
> +        m_animation = adoptNS([[CAKeyframeAnimation animationWithKeyPath:keyPath] retain]);

I think this might read cleaner without the retain and adoption.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:351
> -        value.adoptNS([[NSNumber numberWithDouble:amount] retain]);
> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);

This should use alloc / initWithDouble:

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:392
> -        value.adoptNS([[NSNumber numberWithDouble:amount] retain]);
> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);

This should use alloc / initWithDouble:

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:404
> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);

This should use alloc / initWithDouble:

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:491
> -        value.adoptNS([[NSNumber numberWithDouble:amount] retain]);
> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);

This should use alloc / initWithDouble:

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:505
> -        value.adoptNS([[NSNumber numberWithDouble:amount] retain]);
> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);

This should use alloc / initWithDouble:

> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:357
> -        bitmapContext.adoptCF(CGBitmapContextCreate(0, m_imageWidth, m_imageHeight, 8, m_imageWidth * 4,
> +        bitmapContext = adoptCF(CGBitmapContextCreate(0, m_imageWidth, m_imageHeight, 8, m_imageWidth * 4,
>                                                      deviceRGBColorSpaceRef(),
>                                                      kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host));

We should de-indent this stuff.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:84
> -            RetainPtr<CFRunLoopSourceRef> runLoopSource(AdoptCF, CFNetworkExecuteProxyAutoConfigurationURL(scriptURL, url, proxyAutoConfigurationResultCallback, &context));
> +            RetainPtr<CFRunLoopSourceRef> runLoopSource = adoptCF(CFNetworkExecuteProxyAutoConfigurationURL(scriptURL, url, proxyAutoConfigurationResultCallback, &context));

We should probably double check this is supposed to be adopted. It doesn't adhere to naming rules.

> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:154
> +    m_pacRunLoopSource = adoptCF(CFNetworkExecuteProxyAutoConfigurationURL(pacFileURL, m_httpsURL.get(), pacExecutionCallback, &clientContext));

Same note as above.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:83
> -        m_nsResponse.adoptNS([[NSURLResponse _responseWithCFURLResponse:m_cfResponse.get()] retain]);
> +        m_nsResponse = adoptNS([[NSURLResponse _responseWithCFURLResponse:m_cfResponse.get()] retain]);

We could remove the adopt / retain.

> Source/WebKit/mac/Plugins/WebNetscapePluginStream.mm:575
> -        m_path.adoptNS([[NSString stringWithUTF8String:temporaryFileName] retain]);
> +        m_path = adoptNS([[NSString stringWithUTF8String:temporaryFileName] retain]);

This should use alloc / initWithUTF8String:

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:280
> -    result.adoptCF(resultString.leakRef());
> +    result = adoptCF(resultString.leakRef());

I think this should probably be adoptNS.

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:347
>  
> -    result.adoptCF(dictionary.leakRef());
> +    result = adoptCF(dictionary.leakRef());

adoptNS?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:382
> -    result.adoptCF((NSNumber *)number.leakRef());
> +    result = adoptCF((NSNumber *)number.leakRef());

adoptNS?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:397
> -    result.adoptCF((NSString *)string.leakRef());
> +    result = adoptCF((NSString *)string.leakRef());

adoptNS?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:432
> -    result.adoptNS(array.leakRef());
> +    result = adoptNS(array.leakRef());

adoptNS?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:447
> -    result.adoptCF((NSDate *)date.leakRef());
> +    result = adoptCF((NSDate *)date.leakRef());

adoptNS?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:462
> -    result.adoptCF((NSData *)data.leakRef());
> +    result = adoptCF((NSData *)data.leakRef());

adoptNS?

[Darin Adler]

Comment on attachment 199909 [details]
Patch

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

I’m not going to make these changes since I don’t want to combine bug fixes and handwritten tweaks with a style change that is a global replace, but I do think you spotted some bugs and opportunities for improvement that I will tackle.

>> Source/WebCore/platform/MIMETypeRegistry.cpp:200
>> +        RetainPtr<CFStringRef> supportedType = adoptCF(reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));
> 
> Is this one right? It's not a Copy or Create.

Might be wrong. If so, not newly wrong. Need to read CGImageSourceCopyTypeIdentifiers documentation to be sure.

>> Source/WebCore/platform/MIMETypeRegistry.cpp:287
>> +        RetainPtr<CFStringRef> supportedType = adoptCF(reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(supportedTypes.get(), i)));
> 
> Same question.

Might be wrong. If so, not newly wrong. Need to read CGImageDestinationCopyTypeIdentifiers documentation to be sure.

>> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:167
>> +        m_animation = adoptNS([[CAKeyframeAnimation animationWithKeyPath:keyPath] retain]);
> 
> I think this might read cleaner without the retain and adoption.

It definitely would.

>> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:351
>> +        value = adoptNS([[NSNumber numberWithDouble:amount] retain]);
> 
> This should use alloc / initWithDouble:

I wonder if it could just use @ syntax instead.

>> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:357
>>                                                      kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host));
> 
> We should de-indent this stuff.

There was an earlier version of this patch where I did these changes by hand instead of with a global replace. In that one, I had de-indented this.

>> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:280
>> +    result = adoptCF(resultString.leakRef());
> 
> I think this should probably be adoptNS.

I think it should just be a straight assignment or a call to a RetainPtr release function rather than an adoptCF/leakRef pair. Same applies to all the other cases like this in this file.

[Darin Adler]

http://trac.webkit.org/changeset/149255

[Darin Adler]

Comment on attachment 199909 [details]
Patch

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

>> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:84
>> +            RetainPtr<CFRunLoopSourceRef> runLoopSource = adoptCF(CFNetworkExecuteProxyAutoConfigurationURL(scriptURL, url, proxyAutoConfigurationResultCallback, &context));
> 
> We should probably double check this is supposed to be adopted. It doesn't adhere to naming rules.

I double checked. It is supposed to be adopted.

>> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:154
>> +    m_pacRunLoopSource = adoptCF(CFNetworkExecuteProxyAutoConfigurationURL(pacFileURL, m_httpsURL.get(), pacExecutionCallback, &clientContext));
> 
> Same note as above.

I double checked. It is supposed to be adopted.

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 199909 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199909&action=review
> 
> I’m not going to make these changes since I don’t want to combine bug fixes and handwritten tweaks with a style change that is a global replace, but I do think you spotted some bugs and opportunities for improvement that I will tackle.

Though bugzilla doesn't show it, these comments were originally accompanied by and r+. I was just using the opportunity of reviewing that much code to look for past mistakes we could fix, but I certainly agree they shouldn't happen at the same time.  Thanks for making this big change!

[Sam Weinig]

> >> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:280
> >> +    result = adoptCF(resultString.leakRef());
> > 
> > I think this should probably be adoptNS.
> 
> I think it should just be a straight assignment or a call to a RetainPtr release function rather than an adoptCF/leakRef pair. Same applies to all the other cases like this in this file.

Oh, duh, yeah.