Add SPI for Injected Bundle to provide user agent for a given URL

• Component: WebKit2
• Status: RESOLVED FIXED

[Grant Kennell]

Add SPI for Injected Bundle to provide user agent for a given URL

[Grant Kennell]

Created attachment 232594 [details]
Patch

[WebKit Commit Bot]

Attachment 232594 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:255:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:355:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 232594 [details]
Patch

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.

[Adele Peterson]

<rdar://problem/17188960>

[Grant Kennell]

Created attachment 232622 [details]
Patch

[WebKit Commit Bot]

Attachment 232622 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:255:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:102:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2280:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2274:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2282:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:85:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 6 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 232622 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:102
> +    WKStringRef userAgentForURL(WKBundleFrameRef frame, WKStringRef url);

This should take a WebFrame* and API::String* and return an API::String*

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:85
>  #include <WebCore/Widget.h>
>  #include <WebCore/WindowFeatures.h>
>  #include <wtf/NeverDestroyed.h>
> +#include "WKBundleAPICast.h"

Needs sorting.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2270
> +String WebPage::userAgent(WKBundleFrameRef frame, const URL& url) const

This should take a WebFrame*

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2273
> +        RefPtr<API::String> urlString = API::String::create(url.string());

This should be a RefPtr<API::URL>

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2281
> +    }
> +    else {
> +        return m_userAgent;
> +    }
> +}

No need for the else block here.

[Grant Kennell]

Created attachment 232835 [details]
patch

[WebKit Commit Bot]

Attachment 232835 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grant Kennell]

Created attachment 232869 [details]
patch

[Anders Carlsson]

Comment on attachment 232869 [details]
patch

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

Looks good overall! I think you need to rebase the patch against ToT and address my review comments.

> Source/WebKit2/ChangeLog:435
> [phone]  Grant Kennell  <[e-mail]>

This entry should be moved to the top of the ChangeLog.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:409
> +    WKBundlePageUserAgentForURLCallback                                     userAgentForURL;
>  } WKBundlePageLoaderClientV7;

I think you need to add a new struct version here.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageLoaderClient.h:467
> +    WKBundlePageUserAgentForURLCallback                                     userAgentForURL;
>  } WKBundlePageLoaderClient WK_DEPRECATED("Use an explicit versioned struct instead");

You don't need to add this to the deprecated client.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:363
> +    WKStringRef userAgent = m_client.userAgentForURL(toAPI(frame), toAPI(url.get()), m_client.base.clientInfo);

This needs to go into a WKRetainPtr or it will be leaked.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:103
> +    API::String* userAgentForURL(WebFrame*, API::String*);

I think this should return a RefPtr<API::String>

[Grant Kennell]

Created attachment 233063 [details]
patch

[Sam Weinig]

Comment on attachment 233063 [details]
patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:39
> +#include <WebCore/FileSystem.h>

Why is this being added?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:103
> +    API::String* userAgentForURL(WebFrame*, API::String*);

This should take an API::URL*

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2285
> +String WebPage::userAgent(WebFrame *frame, const URL& webcoreURL) const

WebFrame *frame -> WebFrame* frame

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2289
> +        WKRetainPtr<WKStringRef> string = adoptWK(m_loaderClient.client().userAgentForURL(toAPI(frame), toAPI(url.get()), m_loaderClient.client().base.clientInfo));

This should be calling m_loaderClient.userAgentForURL()

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2290
> +        API::String *apiString = toImpl(string.get());

API::String *apiString -> API::String* apiString

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-2291
> -    String userAgent = platformUserAgent(url);
> -    if (!userAgent.isEmpty())
> -        return userAgent;

Why are you removing this?

[Grant Kennell]

Created attachment 233078 [details]
patch

[Grant Kennell]

Created attachment 233272 [details]
patch

[Anders Carlsson]

Looks like the patch doesn't build against ToT.

[Grant Kennell]

Created attachment 233467 [details]
patch

[Build Bot]

Comment on attachment 233467 [details]
patch

Attachment 233467 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6277214945411072

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 233480 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[WebKit Commit Bot]

Comment on attachment 233467 [details]
patch

Rejecting attachment 233467 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 233467, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit2/ChangeLog is not at the top of the file.

Full output: http://webkit-queues.appspot.com/results/5554172233515008

[Grant Kennell]

Created attachment 233641 [details]
patch

Same as previous patch, but with new entry in ChangeLog moved to top

[WebKit Commit Bot]

Comment on attachment 233641 [details]
patch

Rejecting attachment 233641 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

Comment on attachment 233641 [details]
patch

Hopefully the commit queue will preserve the fact that the reviewer is Sam.

[WebKit Commit Bot]

Comment on attachment 233641 [details]
patch

Clearing flags on attachment: 233641

Committed r170330: <http://trac.webkit.org/changeset/170330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.