[webkit-reviews] review requested: [Bug 185986] URL::host should return a StringView to reduce allocations : [Attachment 341310] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has asked  for review:
Bug 185986: URL::host should return a StringView to reduce allocations
https://bugs.webkit.org/show_bug.cgi?id=185986

Attachment 341310: Patch

https://bugs.webkit.org/attachment.cgi?id=341310&action=review




--- Comment #6 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 341310
  --> https://bugs.webkit.org/attachment.cgi?id=341310
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341310&action=review

> Source/WebCore/loader/ResourceLoadStatistics.cpp:336
> -    return primaryDomain(url.host());
> +    return primaryDomain(url.host().toString());

Seems like the ResourceLoadStatistics::primaryDomain() that takes a String
could be converted to take a StringView, avoiding the allocation in some cases.

> Source/WebCore/platform/network/cf/SocketStreamHandleImplCFNet.cpp:315
> +    RetainPtr<CFStringRef> host = m_url.host().toString().createCFString();

Seems unfortunate to do two allocations here, one for toString(), one for
createCFString(). Can we add a createCFString() to StringView to avoid the
extraneous allocation?

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:248
> +    String
requestHost(requestUrlObj.host().toString().convertToASCIILowercase());

Seems unfortunate to do two allocations here, one for toString(), one for
convertToASCIILowercase(). Can we add a convertToASCIILowercase() to StringView
to avoid the extraneous allocation?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4852
> +    auto host = url.host();
>      return equalLettersIgnoringASCIICase(host, "docs.google.com");

I think avoiding the local would be nicer here.

> Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:62
> -    EXPECT_EQ(String("www.example.com"), kurl.host());
> +    EXPECT_EQ(String("www.example.com"), kurl.host().toString());

lol, kurl.