[webkit-reviews] review denied: [Bug 54109] [Qt][WK2]Unbreak InjectedBundle on Qt : [Attachment 83899] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 54109: [Qt][WK2]Unbreak InjectedBundle on Qt
https://bugs.webkit.org/show_bug.cgi?id=54109

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83899&action=review

I think this is really close! Just a few comments.

>> Source/WebKit2/Shared/API/c/WKURL.h:43
>> +WK_EXPORT WKStringRef WKURLCopyHostName(WKURLRef url);
> 
> The parameter name "url" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I think you should put these new functions just beneath WKURLCopyString, all in
the same paragraph.

> Source/WebKit2/Shared/WebURL.h:62
> +    String host() const
> +    {
> +	   WebCore::KURL url(WebCore::KURL(), m_string);
> +	   return url.isValid() ? url.host() : String();
> +    }
> +    
> +    String protocol() const
> +    {
> +	   WebCore::KURL url(WebCore::KURL(), m_string);
> +	   return url.isValid() ? url.protocol() : String();
> +    }

Maybe it would be better to have an OwnPtr<KURL> data member. It would be
lazily initialized using m_string. That way only the first call to one of these
functions would have to pay the cost of parsing the string.

> Tools/WebKitTestRunner/StringFunctions.h:101
> +    WKRetainPtr<WKStringRef> urlString(AdoptWK, WKURLCopyString(urlRef));
> +    return out << toSTD(urlString.get());

Can you use the adoptWK function here?

No need for the .get().