This would allow things that require custom user agents (e.g., some Pepper plugins -- see https://chromiumcodereview.appspot.com/10701119 ) to set them.
Created attachment 151482 [details] Pretty much the minimal patch that allows me to do what I need....
Comment on attachment 151482 [details] Pretty much the minimal patch that allows me to do what I need.... This patch requires a test. You might want to write a Chromium unit test that calls the WebKit API in a similar manner to how Pepper does.
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests has a testing framework that might be useful.
Created attachment 151519 [details] Added test. Now dependent on https://chromiumcodereview.appspot.com/10735037 .
Comment on attachment 151519 [details] Added test. Now dependent on https://chromiumcodereview.appspot.com/10735037 . LGTM. Thanks for the test.
Created attachment 151528 [details] Updated ChangeLog and rolled DEPS.
Attachment 151528 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:621: user_agent_header_name is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:641: sent_request is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:642: default_user_agent_value is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:650: user_agent_value is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:663: sent_user_agent_value is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 151528 [details] Updated ChangeLog and rolled DEPS. Please fix these style issues before landing. WebKit uses camelCase for variable names.
Created attachment 151534 [details] variableNameStyleFixes.
Comment on attachment 151534 [details] variableNameStyleFixes. Attachment 151534 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13208148 New failing tests: http/tests/inspector/extensions-useragent.html
Created attachment 151559 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 151528 [details] Updated ChangeLog and rolled DEPS. Attachment 151528 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13166603 New failing tests: http/tests/inspector/extensions-useragent.html
Created attachment 151560 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 151534 [details] variableNameStyleFixes. Attachment 151534 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13198163 New failing tests: http/tests/inspector/extensions-useragent.html
Created attachment 151566 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 151528 [details] Updated ChangeLog and rolled DEPS. Attachment 151528 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13209166 New failing tests: http/tests/inspector/extensions-useragent.html
Created attachment 151567 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 151534 [details] variableNameStyleFixes. View in context: https://bugs.webkit.org/attachment.cgi?id=151534&action=review > Source/WebCore/loader/FrameLoader.cpp:2870 > void FrameLoader::applyUserAgent(ResourceRequest& request) > { > + if (!request.httpUserAgent().isNull()) > + return; This function no longer just applies user agent. You should rename it to not be misleading.
Ugh. The layout test failure revealed a fatal flaw with my plan: my change resulted in InspectorInstrumentation::applyUserAgentOverride() (done in FrameLoader::userAgent()) not getting a chance for reloads. Arguably, in the long term, a more coherent way of applying user agents to requests would be desirable. But, in the short term, I'll take another approach (new patch coming up shortly).
Created attachment 152090 [details] New approach: add custom user agent to ResourceRequest, to be handled by the FrameLoaderClientImpl.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Maybe the embedder should do this itself using ExtraData?
(In reply to comment #22) > Maybe the embedder should do this itself using ExtraData? Assuming you mean: - Have the owner/user/caller/whatever of the AssociatedURLLoader set an appropriate WebURLRequest::ExtraData. - Then have the AssociatedURLLoader's WebURLLoaderClient use that to set the user agent (in willSendRequest()). I agree that that'd be the most natural thing. Unfortunately, it has a fatal flaw (the reason for which I don't fully understand, but Nate knows more about this[*]): the AssociatedURLLoader::ClientAdapter::willSendRequest() (which would call its clients willSendRequest()) doesn't get called for the initial request -- only for redirects. At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious).... [*] I believe he said that the AssociatedURLLoader doesn't get set up as a client in time. Why, I don't know....
Interesting. That's probably the core of this bug (although it might be tough to fix it there).
Comment on attachment 152090 [details] New approach: add custom user agent to ResourceRequest, to be handled by the FrameLoaderClientImpl. Attachment 152090 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13201870
(In reply to comment #23) > At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious).... Definitely the case that AssociatedURLLoader::ClientAdapter::willSendRequest() only gets called for redirects, and not the original request. :(
Created attachment 152120 [details] Updated to ToT.
(In reply to comment #23) > (In reply to comment #22) > > Maybe the embedder should do this itself using ExtraData? > > Assuming you mean: > > - Have the owner/user/caller/whatever of the AssociatedURLLoader set an appropriate WebURLRequest::ExtraData. > - Then have the AssociatedURLLoader's WebURLLoaderClient use that to set the user agent (in willSendRequest()). > > I agree that that'd be the most natural thing. Unfortunately, it has a fatal flaw (the reason for which I don't fully understand, but Nate knows more about this[*]): the AssociatedURLLoader::ClientAdapter::willSendRequest() (which would call its clients willSendRequest()) doesn't get called for the initial request -- only for redirects. > > At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious).... > > > [*] I believe he said that the AssociatedURLLoader doesn't get set up as a client in time. Why, I don't know.... The reason is the timing of starting a request for a CachedResource. Basically, when DocumentThreadableLoader calls CachedResourceLoader::requestRawResource(), the decision on whether to start a resource load is made synchronously, and the load is actually started synchronously. The CachedResourceClient, therefore, doesn't have the opportunity to call CachedResource::addClient() to add itself until after the request has started and willSendRequest() has been called. I guess we could change this, and it shouldn't be too hard to re-plumb, but I feel like it might make the code a little less readable. The main downside is that there would need to be a CachedResourceClient* parameter on a whole bunch of functions in CachedResourceLoader, but since only CachedRawResource exposes anything analogous to willSendrequest(), it would only be used in a small subset of cases.
That makes sense. My main worry is that we'll want to do other things during will send request, but I guess we can solve that problem when it comes up.
Comment on attachment 152120 [details] Updated to ToT. This patch is fine with me. My only question is whether we're applying the custom user agent at the right layer. It's slightly odd for the state to be in WebCore but to use the state at the WebKit layer. I'd like to defer to Nate for the final review, if that's ok.
Comment on attachment 152120 [details] Updated to ToT. View in context: https://bugs.webkit.org/attachment.cgi?id=152120&action=review > Source/Platform/chromium/public/WebURLRequest.h:176 > + WEBKIT_EXPORT void setCustomUserAgent(const WebString&); question: which takes precedence? WebFrameClient::userAgentOverride or WebURLRequest::setCustomUserAgent? which should take precedence?
(In reply to comment #31) > (From update of attachment 152120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152120&action=review > > > Source/Platform/chromium/public/WebURLRequest.h:176 > > + WEBKIT_EXPORT void setCustomUserAgent(const WebString&); > > question: > > which takes precedence? WebFrameClient::userAgentOverride or WebURLRequest::setCustomUserAgent? > > which should take precedence? WebURLRequest::setCustomUserAgent(), since it's more specific (applying to a request, as opposed to a URL).
Suppose someone wants to force all URLs to load with a given User-Agent. Do we need to have some other mechanism to implement that?
(In reply to comment #33) > Suppose someone wants to force all URLs to load with a given User-Agent. Do we need to have some other mechanism to implement that? Anyways, we should at least just document that setCustomUserAgent overrides both WebKit::Platform::userAgent() as well as WebKit::WebFrameClient::userAgentOverride(). What about the WebInspector's override UA capability? I think setCustomUserAgent overrides that too. (see InspectorInstrumentation::applyUserAgentOverride.) Is that desirable too?
By the way, it occurs to me that you could also implement this patch entirely on the Chromium side. WebURLRequest has an extraData field that could be extended with a CustomUserAgent field. The implementation of willSendRequest in render_view_impl.cc could read that field and apply it to the WebURLRequest. This might be slightly cleaner since the embedder (render_view_impl.cc) implements both userAgentOverride as well as willSendRequest, so if it wants to make code in willSendRequest override code in userAgentOverride, then OK. The advantage is that we avoid having to make WebKit understand N different ways to override the UA string.
(In reply to comment #35) > By the way, it occurs to me that you could also implement this patch entirely on the Chromium side. > > WebURLRequest has an extraData field that could be extended with a CustomUserAgent field. The implementation of willSendRequest in render_view_impl.cc could read that field and apply it to the WebURLRequest. > > This might be slightly cleaner since the embedder (render_view_impl.cc) implements both userAgentOverride as well as willSendRequest, so if it wants to make code in willSendRequest override code in userAgentOverride, then OK. > > The advantage is that we avoid having to make WebKit understand N different ways to override the UA string. This is what I did (https://chromiumcodereview.appspot.com/10780019/, landed Chromium r147060).