String::createCFString should return a RetainPtr
Created attachment 170736 [details] Patch
Comment on attachment 170736 [details] Patch Attachment 170736 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14552746
Comment on attachment 170736 [details] Patch Attachment 170736 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14593382
Created attachment 170764 [details] Patch
Comment on attachment 170764 [details] Patch Attachment 170764 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14564763
Comment on attachment 170764 [details] Patch 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.
Created attachment 170779 [details] Patch
(In reply to comment #6) > (From update of attachment 170764 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170764&action=review > > Does this end up using the move constructor? Yes. > > > Source/WebCore/platform/RuntimeApplicationChecks.cpp:49 > > + RetainPtr<CFStringRef> bundleIdentifierToCompare(AdoptCF, bundleIdentifierString.createCFString().get()); > > Oops!!!!!!! No! Oops!!!!!!! Fixed! > > 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. It's because I removed the AtomicString constructor that takes a CFStringRef. The rationale is in the ChangeLog. > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210 > > + m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i])); > > Why is this now necessary? See my answer above.
Comment on attachment 170779 [details] Patch Attachment 170779 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14546863
Created attachment 170798 [details] Patch
Comment on attachment 170798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170798&action=review > Source/WTF/ChangeLog:13 > + WTFString.h creates a naming conflict between JSC::Debugger and the Debugger function and there's currently > + no AtomicStringCF.cpp where we could safely include RetainPtr.h. Furthermore, these functions were only used > + twice throughout WebCore so just use the StringImpl and String functions instead. Which functions use the AtomicString constructor? I had special plan for those two constructors, I may even have a radar for it. I did not know there were not used a lot.
Created attachment 170802 [details] Patch
Comment on attachment 170802 [details] Patch Attachment 170802 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14555897
Created attachment 171076 [details] Patch
Comment on attachment 171076 [details] Patch Attachment 171076 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14607356
Created attachment 171099 [details] Patch
Comment on attachment 171099 [details] Patch Attachment 171099 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14611561
Created attachment 171110 [details] Patch
Comment on attachment 171110 [details] Patch Attachment 171110 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14616624
Created attachment 171114 [details] Patch
Comment on attachment 171114 [details] Patch Attachment 171114 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14611651
Created attachment 171122 [details] Patch
Comment on attachment 171122 [details] Patch Attachment 171122 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14617649
Created attachment 171137 [details] Patch
Comment on attachment 171137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171137&action=review > Source/WTF/wtf/text/AtomicString.h:-141 > - AtomicString(CFStringRef s) : m_string(add(String(s).impl())) { } Was anyone using this? If so, we should make sure they have an efficient version. This version is not efficient, because it allocates a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table. > Source/WTF/wtf/text/AtomicString.h:-142 > - CFStringRef createCFString() const { return m_string.createCFString(); } Removing this seems fine. > Source/WTF/wtf/text/AtomicString.h:141 > AtomicString(NSString* s) : m_string(add(String(s).impl())) { } If we’re actually using this function, we should change it to not allocate a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table. > Source/WebCore/platform/cf/FileSystemCF.cpp:66 > + return RetainPtr<CFURLRef>(AdoptCF, CFURLCreateWithFileSystemPath(0, path.createCFString().get(), pathStyle, FALSE)); adoptCF function here? > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:285 > + runFontData = fontCache()->getCachedFontData(m_font.fontDescription(), String(fontName.get()), false, FontCache::DoNotRetain).get(); Why is the explicit String conversion needed here, again? Is this a place where we are creating an AtomicString? If so, then it does seem too bad to me that we are allocating and then destroying a StringImp for the font name each time. > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1004 > + RetainPtr<CFStringRef> cfIdentifier = String(base + ".PrivateBrowsing").createCFString(); I don’t think the String is needed around (base + ".PrivateBrowsing"). > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210 > + m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i])); Same question about explicit String conversion as above. Is this for making an AtomicString? And if so, isn’t it a bit wasteful to make and destroy a StringImpl every time? > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:106 > + m_httpHeaderFields.set(String(commonHeaderFields[i]), String(value)); Ditto. > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:124 > + m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i])); Ditto. > Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:390 > + return String("WebKit socket stream, " + handle->m_url.string()).createCFString().leakRef(); I don’t think the String() is needed around the ("WebKit socket stream, " + handle->m_url.string()) expression here. > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:55 > String key = "com.apple.WebKit.searchField:" + name; > - return RetainPtr<CFStringRef>(AdoptCF, key.createCFString()); > + return key.createCFString(); Could be done all on one line. > Source/WebKit2/UIProcess/cf/WebPageProxyCF.cpp:182 > + return String("com.apple.WebKit.searchField:" + name).createCFString(); I don’t think the String is needed before ("com.apple.WebKit.searchField:" + name).
(In reply to comment #25) > (From update of attachment 171137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171137&action=review > > > Source/WTF/wtf/text/AtomicString.h:-141 > > - AtomicString(CFStringRef s) : m_string(add(String(s).impl())) { } > > Was anyone using this? If so, we should make sure they have an efficient version. This version is not efficient, because it allocates a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table. Yes. I made it efficient in #100701, so I won't be removing it. I> > Source/WTF/wtf/text/AtomicString.h:141 > > AtomicString(NSString* s) : m_string(add(String(s).impl())) { } > > If we’re actually using this function, we should change it to not allocate a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table. Ditto. > Why is the explicit String conversion needed here, again? Is this a place where we are creating an AtomicString? If so, then it does seem too bad to me that we are allocating and then destroying a StringImp for the font name each time. > Ditto. > > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1004 > > + RetainPtr<CFStringRef> cfIdentifier = String(base + ".PrivateBrowsing").createCFString(); > > I don’t think the String is needed around (base + ".PrivateBrowsing"). It is - otherwise I get: error: no member named 'createCFString' in 'WTF::StringAppend<const char *, WTF::String>' > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210 > > + m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i])); > > Same question about explicit String conversion as above. Is this for making an AtomicString? And if so, isn’t it a bit wasteful to make and destroy a StringImpl every time? I removed this now since AtomicString(CFStringRef) is efficient. > > > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:106 > > + m_httpHeaderFields.set(String(commonHeaderFields[i]), String(value)); > > Ditto. > Ditto. > > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:124 > > + m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i])); > > Ditto. Ditto. > > > Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:390 > > + return String("WebKit socket stream, " + handle->m_url.string()).createCFString().leakRef(); > > I don’t think the String() is needed around the ("WebKit socket stream, " + handle->m_url.string()) expression here. Again, getting: error: no member named 'createCFString' in 'WTF::StringAppend<const char *, WTF::String>' > > > Source/WebKit2/UIProcess/cf/WebPageProxyCF.cpp:182 > > + return String("com.apple.WebKit.searchField:" + name).createCFString(); > > I don’t think the String is needed before ("com.apple.WebKit.searchField:" + name). Ditto.
Created attachment 171344 [details] Patch
Created attachment 171347 [details] Patch
Created attachment 171348 [details] Patch
Created attachment 171349 [details] Patch
Created attachment 171350 [details] Patch
Committed r132916: <http://trac.webkit.org/changeset/132916>
Created attachment 202820 [details] don't adopt CFStringRefs twice I'm currently running into problems with CFStrings that have apparently been adopted twice. Please check if the proposed patch is correct, and if that's the case I'll prepare a new bug with a committable patch.
> I'm currently running into problems with CFStrings that have apparently been adopted twice. > Please check if the proposed patch is correct, and if that's the case I'll prepare a new bug with a committable patch. Looks reasonable. Please file a separate bug and attach a WebKit patch to it.