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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 171137: Patch
https://bugs.webkit.org/attachment.cgi?id=171137&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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).