Summary: | Move from constructor and member function adoptCF/NS to free function adoptCF/NS. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||
Component: | WebCore Misc. | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Darin Adler
2013-04-27 12:51:27 PDT
Created attachment 199909 [details]
Patch
Comment on attachment 199909 [details]
Patch
r=me
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? 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. 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. (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!
> >> 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.
|