[webkit-reviews] review denied: [Bug 100419] String::createCFString should return a RetainPtr : [Attachment 170764] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Anders Carlsson <andersca at apple.com>'s
request for review:
Bug 100419: String::createCFString should return a RetainPtr
https://bugs.webkit.org/show_bug.cgi?id=100419

Attachment 170764: Patch
https://bugs.webkit.org/attachment.cgi?id=170764&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=170764&action=review


Fantastic patch. One big mistake though. You’ll see it.

> Source/WTF/ChangeLog:16
> +	   (AtomicString):

Please remove bogus lines like this even if the script generates them.

> Source/WTF/ChangeLog:18
> +	   (WTF):

Ditto.

> Source/WTF/ChangeLog:19
> +	   (StringImpl):

Ditto.

> Source/WTF/ChangeLog:21
> +	   (String):

Ditto.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:94
> +    if (RetainPtr<CFStringRef> cfURL =
resource->url().string().createCFString())

Does this end up using the move constructor?

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:49
> +    RetainPtr<CFStringRef> bundleIdentifierToCompare(AdoptCF,
bundleIdentifierString.createCFString().get());

Oops!!!!!!! No!

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:380
> +    return RetainPtr<CFStringRef>(AdoptCF,
UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType,
mimeType.createCFString().get(), 0));

Can this call the adoptCF function instead? I noticed you did that a lot.

> Source/WebCore/platform/graphics/cg/ImageSourceCGMac.mm:37
> +    RetainPtr<CFStringRef> mime =
adoptCF(UTTypeCopyPreferredTagWithClass(uti.createCFString().get(),
kUTTagClassMIMEType));
>      return mime.get();

Can this not use a local variable at all?

> Source/WebCore/platform/graphics/cg/ImageSourceCGMac.mm:43
> +    RetainPtr<CFStringRef> extension =
adoptCF(UTTypeCopyPreferredTagWithClass(uti.createCFString().get(),
kUTTagClassFilenameExtension));
>      return extension.get();

Can this not use a local variable at all?

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:285
> +		       runFontData =
fontCache()->getCachedFontData(m_font.fontDescription(),
String(fontName.get()), false, FontCache::DoNotRetain).get();

Not sure I understand why this change was needed.

> Source/WebCore/platform/mac/ClipboardMac.mm:102
> +    RetainPtr<CFStringRef> utiType(AdoptCF,
UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType,
mimeType.createCFString().get(), NULL));

Can this use the adoptCF function?

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.cpp:66
> +    RetainPtr<CFStringRef> result(AdoptCF,
wkSignedPublicKeyAndChallengeString(keySize,
challengeString.createCFString().get(), keyDescription.get()));
>  
>      return result.get();

If you used the adoptCF function here then you would not need the “result”
local variable any more.

> Source/WebCore/platform/network/cf/DNSCFNet.cpp:93
> +    RetainPtr<CFHostRef> host(AdoptCF, CFHostCreateWithName(0,
hostname.createCFString().get()));

Maybe the adoptCF function here?

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210
> +	       m_httpHeaderFields.set(String((CFStringRef)keys[i]),
String((CFStringRef)values[i]));

Why is this now necessary?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:55
>      String key = "com.apple.WebKit.searchField:" + name;
> -    return RetainPtr<CFStringRef>(AdoptCF, key.createCFString());
> +    return key.createCFString();

Probably don’t need a local variable any more here.

> Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:94
> -    RetainPtr<CFPropertyListRef> objectToStore;
> -    objectToStore.adoptCF(setting.createCFString());
> +    RetainPtr<CFPropertyListRef> objectToStore = setting.createCFString();
>      ASSERT(objectToStore);
>  
> -    RetainPtr<CFStringRef> preferencesKey(AdoptCF,
createKeyForPreferences(key));
> +    RetainPtr<CFStringRef> preferencesKey = createKeyForPreferences(key);
>      CFPreferencesSetAppValue(preferencesKey.get(), objectToStore.get(),
kCFPreferencesCurrentApplication);

Don’t really need the local variables here any more.

> Source/WebKit2/Platform/mac/ModuleMac.mm:33
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF,
CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
m_path.createCFString().get(), kCFURLPOSIXPathStyle, FALSE));

Could use the adoptCF function here.

> Source/WebKit2/Shared/Plugins/Netscape/mac/NetscapePluginModuleMac.mm:355
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF,
CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
pluginPath.createCFString().get(), kCFURLPOSIXPathStyle, false));

Could use the adoptCF function here.

> Source/WebKit2/Shared/Plugins/Netscape/mac/NetscapePluginModuleMac.mm:397
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF,
CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
pluginPath.createCFString().get(), kCFURLPOSIXPathStyle, false));

Could use the adoptCF function here.