[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
bugzilla-daemon at webkit.org
Thu Jun 5 17:18:15 PDT 2014
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.
More information about the webkit-reviews
mailing list