[Webkit-unassigned] [Bug 133562] Add SPI for Injected Bundle to provide user agent for a given URL

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 5 17:18:16 PDT 2014


https://bugs.webkit.org/show_bug.cgi?id=133562


Sam Weinig <sam at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232594|review?                     |review-
               Flag|                            |




--- Comment #3 from Sam Weinig <sam at webkit.org>  2014-06-05 17:18:40 PST ---
(From update of attachment 232594)
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/WKWebProcessPlugInLoadDelegate.h:59
> +- (NSString *)webProcessPlugInBrowserContextController:(WKWebProcessPlugInBrowserContextController *)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/WKWebProcessPlugInBrowserContextController.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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list