[webkit-reviews] review denied: [Bug 133562] Add SPI for Injected Bundle to provide user agent for a given URL : [Attachment 232594] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Grant Kennell <gkennell at apple.com>'s
request for review:
Bug 133562: Add SPI for Injected Bundle to provide user agent for a given URL
https://bugs.webkit.org/show_bug.cgi?id=133562

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232594&action=review


> Source/WebKit2/ChangeLog:24
> +Added API for Injected Bundle to provide user agent for a given URL.
> +
> +Need the bug URL (OOPS!).
> +
> +Reviewed by NOBODY (OOPS!).
> +
> +* WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelegate.h: 
> +  Added delegate method to WebProcess PluIn protocol to provide UserAgent
per URL.
> +* WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:
> +  Added new typedef for function pointer for this new delegate call
> +  and added a function pointer of that type to the struct.
> +*
WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm
:
> +(userAgentForURL): Makes delegate call with the new method.
> +(setUpPageLoaderClient): Sets the struct's new function pointer to the new
method.
> +* WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:
> +(WebKit::InjectedBundlePageLoaderClient::userAgentForURL):
> +* WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:
> +* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
> +(WebKit::WebFrameLoaderClient::userAgent):
> +* WebProcess/WebPage/WebPage.cpp:
> +(WebKit::WebPage::userAgent): Began using the new API to ask for user agent
> +  instead of simply returning what had been stored.
> +* WebProcess/WebPage/WebPage.h:
> +(WebKit::WebPage::userAgent): Deleted.

The changelog is formatted incorrectly.  Please use prepare-changelog for the
right way.  It also should include the url of this bug.

>
Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInLoadDelega
te.h:59
> +- (NSString
*)webProcessPlugInBrowserContextController:(WKWebProcessPlugInBrowserContextCon
troller *)controller userAgentForURL:(NSString *)url;

I would pass the WKWebProcessPlugInFrame here as well, for a bit of context. 
You also should seperate this method from the others with a bit of whitespace,
since it is not really part of the "Resource loading" group of functionality.

This should also pass a NSURL rather than an NSString.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:65
> +typedef WKStringRef (*WKBundlePageUserAgentForURLCallback)(WKStringRef url,
const void *clientInfo);

Again, I would pass the frame.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:408

> +    WKBundlePageUserAgentForURLCallback				      
userAgentForURL;

I'm not 100% sure, but you may need to rev the version. We should ask Anders.

>
Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserConte
xtController.mm:191
> +	   return wkString;
> +	   
> +    }

Extraneous whitespace.

>
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:357

> +WKStringRef InjectedBundlePageLoaderClient::userAgentForURL(WKStringRef url)
{
> +    if (!m_client.userAgentForURL)
> +	   return nullptr;

{ should go on the next line.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1190
> +    return webPage->userAgent(url);

To add the frame to client callback, you will need to pass it here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2270
> +String WebPage::userAgent(const WebCore::URL& url) const {

{ Should go on the next line.  You don't need the WebCore::.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2274
> +    RefPtr<API::String> urlString = API::String::create(url.string());
> +    WKRetainPtr<WKStringRef> string =
adoptWK(m_loaderClient.client().userAgentForURL(toAPI(urlString.get()),
m_loaderClient.client().base.clientInfo));
> +    API::String *apiString = toImpl(string.get());
> +    return apiString->string();

This should return m_userAgent if the client isn't implemented or returns null.
 Since this will be called for each URL request, I would recommend checking for
the an implementation, since then, you can avoid allocating the API::String for
the URL.