[webkit-reviews] review denied: [Bug 100419] String::createCFString should return a RetainPtr : [Attachment 170764] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 25 17:36:54 PDT 2012
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.
More information about the webkit-reviews
mailing list