Bug 83959 - [chromium] Add ability to override user agent string per-WebFrameClient
Summary: [chromium] Add ability to override user agent string per-WebFrameClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 86057 86141
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-04-13 16:34 PDT by dfalcantara
Modified: 2012-05-10 14:22 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2012-04-19 10:56 PDT, dfalcantara
no flags Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2012-05-07 15:16 PDT, dfalcantara
no flags Details | Formatted Diff | Diff
Patch for landing (7.39 KB, patch)
2012-05-09 21:45 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dfalcantara 2012-04-13 16:34:31 PDT
Chromium needs a way to override the user agent on a per-WebFrameClient basis, which would allow clients to dynamically swap what user agent is being used on any given page.
Comment 1 dfalcantara 2012-04-19 10:56:21 PDT
Created attachment 137923 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-19 11:00:03 PDT
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.
Comment 3 Eric Seidel (no email) 2012-04-19 14:35:43 PDT
Safari must do something similar?  I guess this is a chromium-level issue only since this is just changing our implementation of the WebCore hook in question.
Comment 4 dfalcantara 2012-04-19 14:57:12 PDT
(In reply to comment #3)
> Safari must do something similar?  I guess this is a chromium-level issue only since this is just changing our implementation of the WebCore hook in question.

I'm honestly not sure how Safari does it, but the only hook I could find along Chromium's current code pathways involves going through InspectorInstrumentation::applyUserAgentOverride.  It didn't seem correct to add an InstrumentingAgent, so I went with this approach.
Comment 5 Tony Chang 2012-04-26 17:03:44 PDT
Can you provide more context on how this will be used?  We already have hooks for overriding the user agent for site compat (see webkit/glue/webkit_glue.cc) and the web inspector also has the ability to override the user agent.  Maybe one of those methods would work for you?
Comment 6 Dirk Pranke 2012-04-26 17:08:51 PDT
(In reply to comment #5)
> Can you provide more context on how this will be used?  We already have hooks for overriding the user agent for site compat (see webkit/glue/webkit_glue.cc) and the web inspector also has the ability to override the user agent.  Maybe one of those methods would work for you?

There's been some email exchanged between Dan, Darin, and myself ... they need to override the user agent per-frame in order to "show the desktop version" of a page. The global version isn't good enough. I don't know if the inspector approach would work or not.
Comment 7 dfalcantara 2012-04-26 17:18:05 PDT
(In reply to comment #5)
> Can you provide more context on how this will be used?  We already have hooks for overriding the user agent for site compat (see webkit/glue/webkit_glue.cc) and the web inspector also has the ability to override the user agent.  Maybe one of those methods would work for you?

We need this for "Request desktop site" on Chrome for Android as we upstream our changes.  The feature calls for a per-NavigationEntry user agent override that can be toggled on and off on the fly.  Adding this hook allows us to override it in RenderViewImpl, which communicates with the WebContents about what user agent should be used as navigations are performed.

* A global override won't work because of the way the RenderViews can share the same WebKit instance.  Moreover, the call in webkit_glue.cc takes in only a URL, so having the same URL loaded in two different tabs would always need to share the same user agent.
* It seemed odd to add an inspector to do this, so I didn't explore this route too deeply.

There's a WIP internal doc describing the bigger picture on what we had to do; the link is on an email thread you're CCed on with me and Dirk ("Desktop user agents, redux").
Comment 8 Tony Chang 2012-04-27 14:37:05 PDT
The inspector hook is in FrameLoader::userAgent (calls into InspectorInstrumentation::applyUserAgentOverride).  I think it would be nice if we could share some code with Inspector (e.g., the override code would live in WebCore and Inspector would call into this code), but not a requirement.  The benefit of sharing would be we wouldn't have to check twice for an override.

Rather than keep track of the override in NavigationEntry, is it possible to set a bool on FrameLoaderClientImpl (by adding a method on WebFrame that you call when the menu is checked)?  That may use less memory and keep most of the code change in a single repository.
Comment 9 dfalcantara 2012-04-27 15:55:56 PDT
(In reply to comment #8)
> The inspector hook is in FrameLoader::userAgent (calls into InspectorInstrumentation::applyUserAgentOverride).  I think it would be nice if we could share some code with Inspector (e.g., the override code would live in WebCore and Inspector would call into this code), but not a requirement.  The benefit of sharing would be we wouldn't have to check twice for an override.

I agree that it'd be nice, but it seems that a couple of the other platforms also introduce their own override/custom user agents aside from the Inspector.

> Rather than keep track of the override in NavigationEntry, is it possible to set a bool on FrameLoaderClientImpl (by adding a method on WebFrame that you call when the menu is checked)?  That may use less memory and keep most of the code change in a single repository.

I don't think this will work with the way the feature is defined.  The flag doesn't change on a per-FrameLoaderClientImpl basis; different NavigationEntries can require the flag be set differently, depending on what the user wants it to be set as.  Since multiple NavigationEntries can share the same FrameLoaderClientImpl, we need to store it higher.

If memory is important, one possibility is to globally set a user agent override string like the one currently inside webkit_glue::UserAgentState instead of setting it on a per-Tab basis.  We really only need a single desktop UA string on Android, so it'd still work for us.
Comment 10 Tony Chang 2012-04-27 16:22:15 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > The inspector hook is in FrameLoader::userAgent (calls into InspectorInstrumentation::applyUserAgentOverride).  I think it would be nice if we could share some code with Inspector (e.g., the override code would live in WebCore and Inspector would call into this code), but not a requirement.  The benefit of sharing would be we wouldn't have to check twice for an override.
> 
> I agree that it'd be nice, but it seems that a couple of the other platforms also introduce their own override/custom user agents aside from the Inspector.

I suspect if we were able to add this feature to WebCore proper, other platforms would start using it.  It's unfortunate that everyone has to reimplement the same code.  I'm not saying you have to do this.  It could be left as a future cleanup.

> > Rather than keep track of the override in NavigationEntry, is it possible to set a bool on FrameLoaderClientImpl (by adding a method on WebFrame that you call when the menu is checked)?  That may use less memory and keep most of the code change in a single repository.
> 
> I don't think this will work with the way the feature is defined.  The flag doesn't change on a per-FrameLoaderClientImpl basis; different NavigationEntries can require the flag be set differently, depending on what the user wants it to be set as.  Since multiple NavigationEntries can share the same FrameLoaderClientImpl, we need to store it higher.

Does the affect of the checkbox last for the entire duration of the tab or does it uncheck when you navigate to a different domain (e.g., by clicking a link)?

Hooking into NavigationEntry seems like a more invasive change than just setting a bool on WebFrame or WebFrameClient (if that'll meet your use case).  I'm not that worried about the memory, just the complexity of changing NavigationEntry.

If we're going to do a callback to WebFrameClient, we may be able to remove Platform::userAgent from Platform.h and roll that into the chrome side code directly.
Comment 11 dfalcantara 2012-04-27 17:34:26 PDT
(In reply to comment #10)
> I suspect if we were able to add this feature to WebCore proper, other platforms would start using it.  It's unfortunate that everyone has to reimplement the same code.  I'm not saying you have to do this.  It could be left as a future cleanup.
I agree.  I was going to suggest opening a different bug to track it, but I wasn't clear on how the process works in the WebKit repo.

> Does the affect of the checkbox last for the entire duration of the tab or does it uncheck when you navigate to a different domain (e.g., by clicking a link)?
The effect of the checkbox carries over from previous navigations on the same tab: if it's on for one page and the user clicks a link, the flag stays on.  When the user goes backwards or forwards in their history, the flag tracks what UA was used to load the page when they were last on the page and re-sets it accordingly.  This helps avoid issues with saved zoom levels and scroll positions when the user reloads a page with a different UA than it is expecting.

> Hooking into NavigationEntry seems like a more invasive change than just setting a bool on WebFrame or WebFrameClient (if that'll meet your use case).  I'm not that worried about the memory, just the complexity of changing NavigationEntry.
Yeah, I understand.  I'm definitely open to suggestions for cleaner ways to satisfy the requirements...

> If we're going to do a callback to WebFrameClient, we may be able to remove Platform::userAgent from Platform.h and roll that into the chrome side code directly.
That makes sense.  We'd be able to pull it out of webkit_glue, if I'm understanding the implications correctly.
Comment 12 Tony Chang 2012-05-02 14:59:45 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Does the affect of the checkbox last for the entire duration of the tab or does it uncheck when you navigate to a different domain (e.g., by clicking a link)?
> The effect of the checkbox carries over from previous navigations on the same tab: if it's on for one page and the user clicks a link, the flag stays on.  When the user goes backwards or forwards in their history, the flag tracks what UA was used to load the page when they were last on the page and re-sets it accordingly.  This helps avoid issues with saved zoom levels and scroll positions when the user reloads a page with a different UA than it is expecting.

Is this setting remembered across browsing sessions?  If I set it for foo.com and I visit foo.com a few days later, will it remember that I want to use the desktop site?

Anyway, using NavigationEntry seems reasonable to me since it's tied to specific pages, not the lifetime of a WebFrame.

This change is fine with me; it needs approval from one of the WebKit API maintainers.
Comment 13 dfalcantara 2012-05-04 10:16:08 PDT
(In reply to comment #12)
> Is this setting remembered across browsing sessions?  If I set it for foo.com and I visit foo.com a few days later, will it remember that I want to use the desktop site?
The current spec discards the flag across sessions, though there is an open feature request to have sticky per domain.

> Anyway, using NavigationEntry seems reasonable to me since it's tied to specific pages, not the lifetime of a WebFrame.
> 
> This change is fine with me; it needs approval from one of the WebKit API maintainers.
Cool, thanks!
Comment 14 Adam Barth 2012-05-04 10:39:14 PDT
Comment on attachment 137923 [details]
Patch

Can we write a unit test for this change?
Comment 15 dfalcantara 2012-05-07 15:16:38 PDT
Created attachment 140602 [details]
Patch
Comment 16 dfalcantara 2012-05-07 15:25:22 PDT
Comment on attachment 140602 [details]
Patch

Added a unit test; please take a look.
Comment 17 Adam Barth 2012-05-09 19:56:43 PDT
Comment on attachment 140602 [details]
Patch

Thanks for the test.
Comment 18 WebKit Review Bot 2012-05-09 20:43:37 PDT
Comment on attachment 140602 [details]
Patch

Clearing flags on attachment: 140602

Committed r116602: <http://trac.webkit.org/changeset/116602>
Comment 19 WebKit Review Bot 2012-05-09 20:43:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2012-05-09 21:10:34 PDT
Re-opened since this is blocked by 86057
Comment 21 Kent Tamura 2012-05-09 21:11:16 PDT
(In reply to comment #18)
> (From update of attachment 140602 [details])
> Clearing flags on attachment: 140602
> 
> Committed r116602: <http://trac.webkit.org/changeset/116602>

I'm rolling it out because of a build error on Windows Debug.


1>.\tests\FrameLoaderClientImplTest.cpp(91) :error C2665: 'WebKit::WebString::fromUTF8' : none of the 2 overloads could convert all the argument types
1>        c:\b\build\slave\webkit-win-latest-dbg\build\src\third_party\webkit\source\webkit\chromium\public\platform\../../../../Platform/chromium/public/WebString.h(87): could be 'WebKit::WebString WebKit::WebString::fromUTF8(const char *)'
1>        while trying to match the argument list '(WTF::CString)'
Comment 22 Adam Barth 2012-05-09 21:20:15 PDT
/me will fix
Comment 23 Adam Barth 2012-05-09 21:45:14 PDT
Created attachment 141082 [details]
Patch for landing
Comment 24 WebKit Review Bot 2012-05-09 23:41:28 PDT
Comment on attachment 141082 [details]
Patch for landing

Clearing flags on attachment: 141082

Committed r116612: <http://trac.webkit.org/changeset/116612>
Comment 25 WebKit Review Bot 2012-05-09 23:41:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2012-05-10 14:06:00 PDT
Re-opened since this is blocked by 86141