Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9623027:
Move --user-agent overriding logic from chrome into content. (Closed)
|
Created:
8 years, 9 months ago by scherkus (not reviewing) Modified:
8 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src Visibility:
Public. |
DescriptionMove --user-agent overriding logic from chrome into content.
BUG=117259
TEST=start chrome and content_shell with and without --user-agent and visit a site like www.whatsmyuseragent.com to verify the expected user agent
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131365
Patch Set 1 #Patch Set 2 : fix indent #
Total comments: 3
Patch Set 3 : rebase #Patch Set 4 : again #Patch Set 5 : one more time #Patch Set 6 : replace SetContentClient() with Initialize() #
Total comments: 8
Patch Set 7 : set UA inside SetContentClient() #Patch Set 8 : cleanup #
Total comments: 5
Patch Set 9 : fixed comment #Patch Set 10 : no moar dcheck #Patch Set 11 : rebase #
Messages
Total messages: 30 (0 generated)
(sorry for spam -- app engine was down) jam: please review dpranke: FYI as I fixed your TODO (a drive-by review wouldn't hurt either!) I've rarely hacked on chrome/content so apologies in advance if I'm done something dumb! http://codereview.chromium.org/9623027/diff/11/content/app/content_main_runne... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/9623027/diff/11/content/app/content_main_runne... content/app/content_main_runner.cc:227: webkit_glue::SetUserAgent( note: previously the only implementation of GetUserAgent() returning true was ChromeContentClient, which simply returned the command line switch http://codereview.chromium.org/9623027/diff/11/content/app/content_main_runne... content/app/content_main_runner.cc:472: // time -- is this a fair assumption? this part concerns me -- any ideas?
oh boy looks like I totally hosed linux browser_tests :\
I'll defer to Dirk since he wrote this. I think the place where you set it now doesn't get called in browser_tests, since the startup process is unfortunately different from chrome. http://codereview.chromium.org/9623027/diff/11/content/app/content_main_runne... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/9623027/diff/11/content/app/content_main_runne... content/app/content_main_runner.cc:472: // time -- is this a fair assumption? sure tha'ts fine
Sorry for the delay in getting to this; I was distracted by car troubles this afternoon. I need to page all of this stuff back in so I won't be able to give it a proper review until tomorrow. By what I can only assume is sheer coincidence, one of the chromium-on-android guys was asking me about this code yesterday because apparently they will need to do some per-site overriding of the user agent, so I'm cc'ing him (Dan) as well so he's aware of this discussion in case this affects them.
On 2012/03/08 07:48:08, Dirk Pranke wrote: > Sorry for the delay in getting to this; I was distracted by car troubles this > afternoon. I need to page all of this stuff back in so I won't be able to give > it a proper review until tomorrow. > > By what I can only assume is sheer coincidence, one of the chromium-on-android > guys was asking me about this code yesterday because apparently they will need > to do some per-site overriding of the user agent, so I'm cc'ing him (Dan) as > well so he's aware of this discussion in case this affects them. No worries -- this isn't blocking me just something I noticed while playing with content_shell :)
On 2012/03/08 21:58:19, scherkus wrote: > On 2012/03/08 07:48:08, Dirk Pranke wrote: > > Sorry for the delay in getting to this; I was distracted by car troubles this > > afternoon. I need to page all of this stuff back in so I won't be able to give > > it a proper review until tomorrow. > > > > By what I can only assume is sheer coincidence, one of the chromium-on-android > > guys was asking me about this code yesterday because apparently they will need > > to do some per-site overriding of the user agent, so I'm cc'ing him (Dan) as > > well so he's aware of this discussion in case this affects them. > > No worries -- this isn't blocking me just something I noticed while playing with > content_shell :) friendly ping (I'm back from vacation!)
On 2012/03/20 11:09:45, scherkus wrote: > On 2012/03/08 21:58:19, scherkus wrote: > > On 2012/03/08 07:48:08, Dirk Pranke wrote: > > > Sorry for the delay in getting to this; I was distracted by car troubles > this > > > afternoon. I need to page all of this stuff back in so I won't be able to > give > > > it a proper review until tomorrow. > > > > > > By what I can only assume is sheer coincidence, one of the > chromium-on-android > > > guys was asking me about this code yesterday because apparently they will > need > > > to do some per-site overriding of the user agent, so I'm cc'ing him (Dan) as > > > well so he's aware of this discussion in case this affects them. > > > > No worries -- this isn't blocking me just something I noticed while playing > with > > content_shell :) > > friendly ping (I'm back from vacation!) lgtm, but presumably you need to figure out why browser_tests failed? Are you sure they call content_main_runner::Initialize() at the right time?
ack -- so it looks like a lot of our various chrome/content unit test helpers (ChromeTestSuiteInitializer, RenderViewTest, etc...) relied on calling content::SetContentClient() to set the user agent. I can either: 1) Duplicate calls to webkit_glue::SetUserAgent() where we currently call content::SetContentClient() 2) Introduce some other new function that does work like setting the user agent and have clients call that 3) [your idea here]
ping jam/dpranke re: what to do about all the test targets that relied on SetContentClient() doing extra work
Sorry for the delay on this ... my head has been in webkit-gardening-land lately ... On Mon, Mar 26, 2012 at 8:47 PM, <scherkus@chromium.org> wrote: > ack -- so it looks like a lot of our various chrome/content unit test > helpers > (ChromeTestSuiteInitializer, RenderViewTest, etc...) relied on calling > content::SetContentClient() to set the user agent. > > I can either: > 1) Duplicate calls to webkit_glue::SetUserAgent() where we currently call > content::SetContentClient() > 2) Introduce some other new function that does work like setting the user > agent and have clients call that > 3) [your idea here] > I don't really like the idea of having client call into glue directly for this, because it feels like we'd be doing and end-run around the layering and it would be hard to understand what's going on. I'd probably introduce a new content::SetUserAgent() call into the API and modify content_main_runner and the test code to call that routine, so that everyone was setting the user agent through a single entry point. -- Dirk > https://chromiumcodereview.appspot.com/9623027/
On Tue, Apr 3, 2012 at 12:16 PM, Dirk Pranke <dpranke@chromium.org> wrote: > Sorry for the delay on this ... my head has been in > webkit-gardening-land lately ... > > On Mon, Mar 26, 2012 at 8:47 PM, <scherkus@chromium.org> wrote: > > ack -- so it looks like a lot of our various chrome/content unit test > > helpers > > (ChromeTestSuiteInitializer, RenderViewTest, etc...) relied on calling > > content::SetContentClient() to set the user agent. > > > > I can either: > > 1) Duplicate calls to webkit_glue::SetUserAgent() where we currently > call > > content::SetContentClient() > > 2) Introduce some other new function that does work like setting the > user > > agent and have clients call that > > 3) [your idea here] > > > > I don't really like the idea of having client call into glue directly > for this, because it feels like we'd be doing and end-run around the > layering and it would be hard to understand what's going on. I'd > probably introduce a new content::SetUserAgent() call into the API and > modify content_main_runner and the test code to call that routine, so > that everyone was setting the user agent through a single entry point. > Hmm... I tried this out but it seemed strange as it forces every consumer of content to have the following duplicated code: content::SetContentClient(my_client); content::SetUserAgent("my user agent"); // Mandatory to call otherwise we DCHECK(). However ContentClient already has a GetUserAgent() method that currently is only used inside of content::SetContentClient() Perhaps "doing work" inside content::SetContentClient() really isn't so bad after all as it does cut down on duplication or would you be OK with forcing clients to call content::SetUserAgent()? > > https://chromiumcodereview.appspot.com/9623027/ >
On Thu, Apr 5, 2012 at 12:12 PM, Andrew Scherkus <scherkus@chromium.org> wrote: > On Tue, Apr 3, 2012 at 12:16 PM, Dirk Pranke <dpranke@chromium.org> wrote: >> >> Sorry for the delay on this ... my head has been in >> webkit-gardening-land lately ... >> >> On Mon, Mar 26, 2012 at 8:47 PM, <scherkus@chromium.org> wrote: >> > ack -- so it looks like a lot of our various chrome/content unit test >> > helpers >> > (ChromeTestSuiteInitializer, RenderViewTest, etc...) relied on calling >> > content::SetContentClient() to set the user agent. >> > >> > I can either: >> > 1) Duplicate calls to webkit_glue::SetUserAgent() where we currently >> > call >> > content::SetContentClient() >> > 2) Introduce some other new function that does work like setting the >> > user >> > agent and have clients call that >> > 3) [your idea here] >> > >> >> I don't really like the idea of having client call into glue directly >> for this, because it feels like we'd be doing and end-run around the >> layering and it would be hard to understand what's going on. I'd >> probably introduce a new content::SetUserAgent() call into the API and >> modify content_main_runner and the test code to call that routine, so >> that everyone was setting the user agent through a single entry point. > > > Hmm... I tried this out but it seemed strange as it forces every consumer of > content to have the following duplicated code: > content::SetContentClient(my_client); > content::SetUserAgent("my user agent"); // Mandatory to call otherwise we > DCHECK(). > > However ContentClient already has a GetUserAgent() method that currently is > only used inside of content::SetContentClient() > > Perhaps "doing work" inside content::SetContentClient() really isn't so bad > after all as it does cut down on duplication or would you be OK with forcing > clients to call content::SetUserAgent()? > I'm not sure there's a great answer to this; it might be that there are only compromises. I don't mind doing work in SetContentClient() too much (that is why I did it that way in the first place). I don't really mind having clients have to call two APIs as they do two different things, but we could collapse them into a content::SetClient(content_client, user_agent) call instead - at which point it might be better to be a content::Init() sort of thing. John might have a more vested opinion here, since he's the keeper of the API ... However, I don't particularly like having clients do *either* GetUserAgent() or SetUserAgent() - that just seems like an unnecessarily ambiguous API. -- Dirk >> >> > https://chromiumcodereview.appspot.com/9623027/ > >
take a look -- it's better in some areas, slightly worse in others :) https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... File chrome/app/chrome_main_delegate.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... chrome/app/chrome_main_delegate.cc:552: std::string user_agent = webkit_glue::BuildUserAgentFromProduct(product); FYI this code was moved here instead of chrome_content_client.cc https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/test/base/ch... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/test/base/ch... chrome/test/base/chrome_test_suite.cc:111: content::Initialize(content_client_.get(), "ChromeTestSuite"); FYI https://chromiumcodereview.appspot.com/9623027/diff/29001/content/browser/sit... File content/browser/site_instance_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/content/browser/sit... content/browser/site_instance_impl_unittest.cc:123: content::Initialize(&client_, "SiteInstanceTest"); is this ContentClient swapping necessary? :( https://chromiumcodereview.appspot.com/9623027/diff/29001/content/browser/sit... content/browser/site_instance_impl_unittest.cc:130: content::GetContentClient()->set_browser(old_browser_client_); is this line actually needed? I don't believe we change the browser of old_browser_client_...
This looks basically okay to me, but I defer to John for the changes to the API; I seem to recall him not liking the Init()/Uninit() design in favor of calling back through the content client ... https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... File chrome/app/chrome_main_delegate.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... chrome/app/chrome_main_delegate.cc:556: InitializeChromeContentClient(process_type); Nit: does it make sense to change this to something like InitializeChromeContentClient(&chrome_content_client_, process_type) and call content::Initialize() from there?. It's a little weird to see content::Initialize() followed by InitializeChromeContentClient(). https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/test/base/ch... File chrome/test/base/chrome_test_suite.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/test/base/ch... chrome/test/base/chrome_test_suite.cc:111: content::Initialize(content_client_.get(), "ChromeTestSuite"); On 2012/04/05 23:20:45, scherkus wrote: > FYI It "shouldn't" be an issue, but it may be that there are tests somewhere that depend on the string being used. I *think* I changed things so that we only cared that the UA was non-null and/or non-empty. https://chromiumcodereview.appspot.com/9623027/diff/29001/content/browser/sit... File content/browser/site_instance_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/content/browser/sit... content/browser/site_instance_impl_unittest.cc:123: content::Initialize(&client_, "SiteInstanceTest"); On 2012/04/05 23:20:45, scherkus wrote: > is this ContentClient swapping necessary? :( I don't know this code; looks like it was added in r126949 by creis, so we might need him or jam to comment.
I do like how calling back through the client centralizes where UA strings are determined (i.e., it's impossible to mix+match ContentClient impls w/ UA strings) jam any thoughts/preferences on how to proceed? https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... File chrome/app/chrome_main_delegate.cc (right): https://chromiumcodereview.appspot.com/9623027/diff/29001/chrome/app/chrome_m... chrome/app/chrome_main_delegate.cc:556: InitializeChromeContentClient(process_type); On 2012/04/05 23:35:54, Dirk Pranke wrote: > Nit: does it make sense to change this to something like > InitializeChromeContentClient(&chrome_content_client_, process_type) and call > content::Initialize() from there?. It's a little weird to see > content::Initialize() followed by InitializeChromeContentClient(). Agreed but will wait to hear back on jam on which way we should proceed with the API changes
I prefer avoiding Intialize/Uninitialize. Right now we don't support uninitializing since there could be other statics that have pointers to the client or to other objects from it (i.e. ResourceDispatcherHostDelegate), so making it seem like switching the client of content is possible would be misleading as for doing work in SetContentClient, I think that's totally acceptable. we don't make any contract about when the ContentClient callbacks can be called (i.e. in the middle of the client calling content).
On 2012/04/06 00:35:06, John Abd-El-Malek wrote: > I prefer avoiding Intialize/Uninitialize. Right now we don't support > uninitializing since there could be other statics that have pointers to the > client or to other objects from it (i.e. ResourceDispatcherHostDelegate), so > making it seem like switching the client of content is possible would be > misleading What about tests that swap clients in-and-out, such as SiteInstanceTest? I originally had some stricter DCHECKs that prevent you from calling SetContentClient() multiple times unit tests rely on that functionality. > as for doing work in SetContentClient, I think that's totally acceptable. we > don't make any contract about when the ContentClient callbacks can be called > (i.e. in the middle of the client calling content). Cool -- yeah I think the TODO led me to believe it was a bad thing but after playing around with a few alternatives I think it's acceptable.
PTAL
On 2012/04/06 00:38:20, scherkus wrote: > On 2012/04/06 00:35:06, John Abd-El-Malek wrote: > > I prefer avoiding Intialize/Uninitialize. Right now we don't support > > uninitializing since there could be other statics that have pointers to the > > client or to other objects from it (i.e. ResourceDispatcherHostDelegate), so > > making it seem like switching the client of content is possible would be > > misleading > > What about tests that swap clients in-and-out, such as SiteInstanceTest? I > originally had some stricter DCHECKs that prevent you from calling > SetContentClient() multiple times unit tests rely on that functionality. those work just because they dont depend on changing the other cached pointers.. > > > as for doing work in SetContentClient, I think that's totally acceptable. we > > don't make any contract about when the ContentClient callbacks can be called > > (i.e. in the middle of the client calling content). > > Cool -- yeah I think the TODO led me to believe it was a bad thing but after > playing around with a few alternatives I think it's acceptable.
http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... content/app/content_main_runner.cc:475: << "SetContentClient() must be called by this point in time"; nit: are you sure this dcheck is necessary? ie. have you tried not setting it and seen that you can get this far without a crash somewhere? http://codereview.chromium.org/9623027/diff/33002/content/public/common/conte... File content/public/common/content_client.cc (right): http://codereview.chromium.org/9623027/diff/33002/content/public/common/conte... content/public/common/content_client.cc:19: // Set the default user agent as provided by the client. nit: please keep the "we need to make sure this is done before 22 // webkit_glue::GetUserAgent() is called (so that the UA doesn't change)."
http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... content/app/content_main_runner.cc:475: << "SetContentClient() must be called by this point in time"; On 2012/04/06 23:13:52, John Abd-El-Malek wrote: > nit: are you sure this dcheck is necessary? ie. have you tried not setting it > and seen that you can get this far without a crash somewhere? if the call to SetContentClient() happens after setting a custom UA the it would get clobbered due to SetContentClient() setting the default UA string so really the DCHECK() is for protecting --user-agent but I suppose it could get moved later down I'll give it a shot http://codereview.chromium.org/9623027/diff/33002/content/public/common/conte... File content/public/common/content_client.cc (right): http://codereview.chromium.org/9623027/diff/33002/content/public/common/conte... content/public/common/content_client.cc:19: // Set the default user agent as provided by the client. On 2012/04/06 23:13:52, John Abd-El-Malek wrote: > nit: please keep the "we need to make sure this is done before > 22 // webkit_glue::GetUserAgent() is called (so that the UA doesn't change)." Done.
http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/9623027/diff/33002/content/app/content_main_ru... content/app/content_main_runner.cc:475: << "SetContentClient() must be called by this point in time"; On 2012/04/06 23:59:41, scherkus wrote: > On 2012/04/06 23:13:52, John Abd-El-Malek wrote: > > nit: are you sure this dcheck is necessary? ie. have you tried not setting it > > and seen that you can get this far without a crash somewhere? > > if the call to SetContentClient() happens after setting a custom UA the it would > get clobbered due to SetContentClient() setting the default UA string > > so really the DCHECK() is for protecting --user-agent but I suppose it could get > moved later down > > I'll give it a shot just to be clear, i meant if there's code in content that's called earlier than this point which does "content::GetContentClient()->". so it'd crash which is as good a signal as this dcheck
overly defensive DCHECK() removed ran tests, content_shell, chrome, etc... and it turns out we really need it
Okay, so given where we now are with this patch, it seems like it can be summarized as "remove the ability for content clients to indicate that a user agent should always be used" and that functionality is now only available through content_main_runner or calling into webkit_glue directly. Is that right? I had thought the android port might need some sort of overriding functionality, so I'm not sure how this will (or won't) affect them ... Dan, is this still you? On Fri, Apr 6, 2012 at 6:32 PM, <scherkus@chromium.org> wrote: > http://codereview.chromium.org/9623027/
On 2012/04/07 01:52:29, Dirk Pranke wrote: > Okay, so given where we now are with this patch, it seems like it can > be summarized as "remove the ability for content clients to indicate > that a user agent should always be used" and that functionality is now > only available through content_main_runner or calling into webkit_glue > directly. > > Is that right? By "should always be used" do mean being able to pass true (as opposed to false) to webkit_glue::SetUserAgent()? If so, yes I removed that ability from content clients. I'm not as familiar with the Android port and it'd be beneficial if such code that uses this functionality would be upstreamed because as it stands today it appears the only user who passed true to SUA() to override the UA was the --user-agent switch code, which this CL moves into content/app/content_main_runner.cc Dan can you provide some insight/pointers? > I had thought the android port might need some sort of overriding > functionality, so I'm not sure how this will (or won't) affect them > ... Dan, is this still you? > > On Fri, Apr 6, 2012 at 6:32 PM, <mailto:scherkus@chromium.org> wrote: > > http://codereview.chromium.org/9623027/
On Fri, Apr 6, 2012 at 6:58 PM, <scherkus@chromium.org> wrote: > On 2012/04/07 01:52:29, Dirk Pranke wrote: >> >> Okay, so given where we now are with this patch, it seems like it can >> be summarized as "remove the ability for content clients to indicate >> that a user agent should always be used" and that functionality is now >> only available through content_main_runner or calling into webkit_glue >> directly. > > >> Is that right? > > > By "should always be used" do mean being able to pass true (as opposed to > false) > to webkit_glue::SetUserAgent()? Yes >If so, yes I removed that ability from content > clients. I'm not as familiar with the Android port and it'd be beneficial if > such code that uses this functionality would be upstreamed because as it > stands > today it appears the only user who passed true to SUA() to override the UA > was > the --user-agent switch code, which this CL moves into > content/app/content_main_runner.cc > Yes, exactly. Of course, the android port isn't fully upstreamed yet and as far as I know doesn't use content at all, but it seems like they should be a target user of content (if they aren't, then it seems like something has gone horribly awry with our design of content). Hence the question ;) -- Dirk
> >If so, yes I removed that ability from content > > clients. I'm not as familiar with the Android port and it'd be beneficial if > > such code that uses this functionality would be upstreamed because as it > > stands > > today it appears the only user who passed true to SUA() to override the UA > > was > > the --user-agent switch code, which this CL moves into > > content/app/content_main_runner.cc > > > > Yes, exactly. Of course, the android port isn't fully upstreamed yet > and as far as I know doesn't use content at all, but it seems like > they should be a target user of content (if they aren't, then it seems > like something has gone horribly awry with our design of content). > > Hence the question ;) > > -- Dirk Hey, yes, I'm definitely the person to ping about it. We didn't end up using this pathway for the user agent swapping though; I understood this to be a global setting. We needed user agent swapping on a per-Tab/per-NavigationEntry basis and had to introduce other ways of dynamically setting it. I've started work on upstreaming the relevant code earlier this week, but I'm taking the time figure out what could be done better (and adding unit tests) before firing off CLs. - Dan
Thanks for the info! So Dan do you have any issues landing this CL as is today? On Apr 6, 2012 7:47 PM, <dfalcantara@chromium.org> wrote: > >If so, yes I removed that ability from content >> > clients. I'm not as familiar with the Android port and it'd be >> beneficial if >> > such code that uses this functionality would be upstreamed because as it >> > stands >> > today it appears the only user who passed true to SUA() to override the >> UA >> > was >> > the --user-agent switch code, which this CL moves into >> > content/app/content_main_**runner.cc >> > >> > > Yes, exactly. Of course, the android port isn't fully upstreamed yet >> and as far as I know doesn't use content at all, but it seems like >> they should be a target user of content (if they aren't, then it seems >> like something has gone horribly awry with our design of content). >> > > Hence the question ;) >> > > -- Dirk >> > > Hey, yes, I'm definitely the person to ping about it. We didn't end up > using > this pathway for the user agent swapping though; I understood this to be a > global setting. We needed user agent swapping on a > per-Tab/per-NavigationEntry > basis and had to introduce other ways of dynamically setting it. > > I've started work on upstreaming the relevant code earlier this week, but > I'm > taking the time figure out what could be done better (and adding unit > tests) > before firing off CLs. > > - Dan > > http://codereview.chromium.**org/9623027/<http://codereview.chromium.org/9623... >
On 2012/04/07 03:01:15, scherkus wrote: > Thanks for the info! > > So Dan do you have any issues landing this CL as is today? I'm pretty sure it'll land cleanly on our end. Thanks for the heads up, though. > On Apr 6, 2012 7:47 PM, <mailto:dfalcantara@chromium.org> wrote: > > > >If so, yes I removed that ability from content > >> > clients. I'm not as familiar with the Android port and it'd be > >> beneficial if > >> > such code that uses this functionality would be upstreamed because as it > >> > stands > >> > today it appears the only user who passed true to SUA() to override the > >> UA > >> > was > >> > the --user-agent switch code, which this CL moves into > >> > content/app/content_main_**runner.cc > >> > > >> > > > > Yes, exactly. Of course, the android port isn't fully upstreamed yet > >> and as far as I know doesn't use content at all, but it seems like > >> they should be a target user of content (if they aren't, then it seems > >> like something has gone horribly awry with our design of content). > >> > > > > Hence the question ;) > >> > > > > -- Dirk > >> > > > > Hey, yes, I'm definitely the person to ping about it. We didn't end up > > using > > this pathway for the user agent swapping though; I understood this to be a > > global setting. We needed user agent swapping on a > > per-Tab/per-NavigationEntry > > basis and had to introduce other ways of dynamically setting it. > > > > I've started work on upstreaming the relevant code earlier this week, but > > I'm > > taking the time figure out what could be done better (and adding unit > > tests) > > before firing off CLs. > > > > - Dan > > > > > http://codereview.chromium.**org/9623027/%3Chttp://codereview.chromium.org/96...> > >
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
