Move user_agent code from webkit/ to content/.

Move user_agent code from webkit/ to content/.

BUG=265753, 338338, 237249
TEST=content_unittests
R=jam@chromium.org
TBR=darin

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256174

[jam, 2014-03-04 20:49:44]

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
content/public/common/content_client.cc:44: SetUserAgent(client->GetUserAgent(),
false);
this isn't needed anymore either since the user agent code is now in
content/common. so it can call content::GetContentClient()->GetUserAgent()
lazily the first time it's needed

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
content/public/common/content_client.h:76: CONTENT_EXPORT const std::string&
GetUserAgent(const GURL& url);
we actually don't need this wrapper anymore, it was needed before because of
component build (see comment above)

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
File content/public/common/user_agent.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
content/public/common/user_agent.h:18: CONTENT_EXPORT void SetUserAgent(const
std::string& user_agent,
this is only called by code in content, so it shouldn't be in the public api

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
File content/public/common/user_agent_util.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
content/public/common/user_agent_util.h:13: CONTENT_EXPORT std::string
BuildOSCpuInfo();
no need to have user_agent.h and user_agent_util.h. put these methods in
user_agent.h

[tfarina, 2014-03-04 21:49:37]

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
content/public/common/content_client.h:76: CONTENT_EXPORT const std::string&
GetUserAgent(const GURL& url);
On 2014/03/04 20:49:45, jam wrote:
> we actually don't need this wrapper anymore, it was needed before because of
> component build (see comment above)

What we do about the overrides? shell_content_client.h and test_content_client.h

[tfarina, 2014-03-04 22:07:19]

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/co...
content/public/common/content_client.cc:44: SetUserAgent(client->GetUserAgent(),
false);
On 2014/03/04 20:49:45, jam wrote:
> this isn't needed anymore either since the user agent code is now in
> content/common. so it can call content::GetContentClient()->GetUserAgent()
> lazily the first time it's needed

Done.

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
File content/public/common/user_agent.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
content/public/common/user_agent.h:18: CONTENT_EXPORT void SetUserAgent(const
std::string& user_agent,
On 2014/03/04 20:49:45, jam wrote:
> this is only called by code in content, so it shouldn't be in the public api

Done.

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
File content/public/common/user_agent_util.h (right):

https://codereview.chromium.org/186883002/diff/90001/content/public/common/us...
content/public/common/user_agent_util.h:13: CONTENT_EXPORT std::string
BuildOSCpuInfo();
On 2014/03/04 20:49:45, jam wrote:
> no need to have user_agent.h and user_agent_util.h. put these methods in
> user_agent.h

Done.

[jam, 2014-03-05 00:09:26]

https://codereview.chromium.org/186883002/diff/160001/content/common/user_age...
File content/common/user_agent_internal.h (right):

https://codereview.chromium.org/186883002/diff/160001/content/common/user_age...
content/common/user_agent_internal.h:16: void SetUserAgent(const std::string&
user_agent, bool overriding);
so looking at the callsites for this, I think we can get rid of the non-test one
(SetUpTestEnvironmentForUnitTests() in webkit_support.cc). i.e. move that logic
in content_main_runner.cc to user_agent.cc's GetUserAgent's lazy initialization.
this method will have to remain, but can get a ForTesting suffix, for use by the
webkit_support.cc

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (left):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:44:
webkit_glue::SetUserAgent(client->GetUserAgent(), false);
i don't see any code which now calls ContentClient->GetUserAgent(). that still
needs to happen. it should just be lazily initialized in user_agent.cc's
GetUserAgent, i.e. a custom constructor for the lazy instance

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:9: #include
"content/public/common/user_agent.h"
nit: not needed

https://codereview.chromium.org/186883002/diff/160001/content/public/common/u...
File content/public/common/user_agent.h (right):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/u...
content/public/common/user_agent.h:17: // be called prior to calling this
function.
nit: remove the second part of the comment since it doesn't apply anymore

[tfarina, 2014-03-05 00:22:10]

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (left):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:44:
webkit_glue::SetUserAgent(client->GetUserAgent(), false);
On 2014/03/05 00:09:26, jam wrote:
> i don't see any code which now calls ContentClient->GetUserAgent(). that still
> needs to happen. it should just be lazily initialized in user_agent.cc's
> GetUserAgent, i.e. a custom constructor for the lazy instance

https://code.google.com/p/chromium/codesearch#search/&q=%22-%3Egetuseragent(%...

https://code.google.com/p/chromium/codesearch#search/&q=%22.getuseragent(%22&...

So looks like we have one in chrome_content_client_unittest.cc, one in
content/browser/accessibility/browser_accessibility_win.cc and this one here in
content_client.cc

I'm afraid that I'm unsure which change you want in user_agent.cc :(

[tfarina, 2014-03-05 00:42:51]

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:9: #include
"content/public/common/user_agent.h"
On 2014/03/05 00:09:26, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/186883002/diff/160001/content/public/common/u...
File content/public/common/user_agent.h (right):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/u...
content/public/common/user_agent.h:17: // be called prior to calling this
function.
On 2014/03/05 00:09:26, jam wrote:
> nit: remove the second part of the comment since it doesn't apply anymore

Done.

https://codereview.chromium.org/186883002/diff/180001/content/common/user_age...
File content/common/user_agent.cc (right):

https://codereview.chromium.org/186883002/diff/180001/content/common/user_age...
content/common/user_agent.cc:144: const CommandLine& command_line =
*CommandLine::ForCurrentProcess();
If I put this code in UserAgentState::Get() the following error occurs:

../../content/common/user_agent.cc:118:5: error: member function 'Set' not
viable: 'this' argument has type 'const content::<anonymous>::UserAgentState',
but function is not marked const
    Set(command_line.GetSwitchValueASCII(switches::kUserAgent), true);
    ^~~
../../content/common/user_agent.cc:62:22: note: 'Set' declared here
void UserAgentState::Set(const std::string& user_agent, bool overriding) {
                     ^

This is because Get() is const but Set() not.

I moved it over here because that is what I understood from your last comment
about moving the logic from content_main_runner.cc

[jam, 2014-03-05 08:34:55]

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (left):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:44:
webkit_glue::SetUserAgent(client->GetUserAgent(), false);
On 2014/03/05 00:22:11, tfarina wrote:
> On 2014/03/05 00:09:26, jam wrote:
> > i don't see any code which now calls ContentClient->GetUserAgent(). that
still
> > needs to happen. it should just be lazily initialized in user_agent.cc's
> > GetUserAgent, i.e. a custom constructor for the lazy instance
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522-%253Egetuserage...
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522.getuseragent%28...
> 
> So looks like we have one in chrome_content_client_unittest.cc, one in
> content/browser/accessibility/browser_accessibility_win.cc and this one here
in
> content_client.cc
> 
> I'm afraid that I'm unsure which change you want in user_agent.cc :(

sorry I wasn't clear. I mean, user_agent.cc needs to call the equivalent of
SetUserAgent(content::GetContentClient()->GetUserAgent()) because otherwise the
user agent string from the embedder is never fetched by content itself for the
lazy instance. is that clearer?

btw looking at this some more, I think we can really simplify the user agent
code. The reason is that the microsoft/yahoo user agent changing code was there
for the first release. it's not needed anymore. the microsoft silverlight
download page still shows a download without that user agent override. the yahoo
downloads page for silverlight says that site will go away in june. The yahoo
gyao.yahoo.co.jp page doesn't have silverlight on it now.
promotion.shopping.yahoo.co.jp is dead.

[tfarina, 2014-03-05 13:31:55]

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
File content/public/common/content_client.cc (left):

https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
content/public/common/content_client.cc:44:
webkit_glue::SetUserAgent(client->GetUserAgent(), false);
On 2014/03/05 08:34:56, jam wrote:
> On 2014/03/05 00:22:11, tfarina wrote:
> > On 2014/03/05 00:09:26, jam wrote:
> > > i don't see any code which now calls ContentClient->GetUserAgent(). that
> still
> > > needs to happen. it should just be lazily initialized in user_agent.cc's
> > > GetUserAgent, i.e. a custom constructor for the lazy instance
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%252522-%25253Egetuse...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%252522.getuseragent%...
> > 
> > So looks like we have one in chrome_content_client_unittest.cc, one in
> > content/browser/accessibility/browser_accessibility_win.cc and this one here
> in
> > content_client.cc
> > 
> > I'm afraid that I'm unsure which change you want in user_agent.cc :(
> 
> sorry I wasn't clear. I mean, user_agent.cc needs to call the equivalent of
> SetUserAgent(content::GetContentClient()->GetUserAgent()) because otherwise
the
> user agent string from the embedder is never fetched by content itself for the
> lazy instance. is that clearer?
> 
Sorry, I have no idea what you are asking me to do in user_agent.cc :(

[tfarina, 2014-03-06 01:55:12]

John, can we do these simplifications in a follow-up?

[jam, 2014-03-06 02:49:49]

On 2014/03/05 13:31:55, tfarina wrote:
>
https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
> File content/public/common/content_client.cc (left):
> 
>
https://codereview.chromium.org/186883002/diff/160001/content/public/common/c...
> content/public/common/content_client.cc:44:
> webkit_glue::SetUserAgent(client->GetUserAgent(), false);
> On 2014/03/05 08:34:56, jam wrote:
> > On 2014/03/05 00:22:11, tfarina wrote:
> > > On 2014/03/05 00:09:26, jam wrote:
> > > > i don't see any code which now calls ContentClient->GetUserAgent(). that
> > still
> > > > needs to happen. it should just be lazily initialized in user_agent.cc's
> > > > GetUserAgent, i.e. a custom constructor for the lazy instance
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%25252522-%2525253Ege...
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%25252522.getuseragen...
> > > 
> > > So looks like we have one in chrome_content_client_unittest.cc, one in
> > > content/browser/accessibility/browser_accessibility_win.cc and this one
here
> > in
> > > content_client.cc
> > > 
> > > I'm afraid that I'm unsure which change you want in user_agent.cc :(
> > 
> > sorry I wasn't clear. I mean, user_agent.cc needs to call the equivalent of
> > SetUserAgent(content::GetContentClient()->GetUserAgent()) because otherwise
> the
> > user agent string from the embedder is never fetched by content itself for
the
> > lazy instance. is that clearer?
> > 
> Sorry, I have no idea what you are asking me to do in user_agent.cc :(

currently I don't think this will work, have you tried checking that the right
user agent is being used?

there's a ton of complexity in user_agent.cc that we don't need anymore. we
could solve the above bug, or we can remove almost all of it. before, content
and webkit used to have their own cache of user agent to solve around different
bugs. but these bugs, as i mentioned in the previous message, don't need to be
worked around anymore. so things can be much easier with just having a
ContentClient::GetUserAgent for the embedder to reply, and everywhere in content
can use that. for chrome, ChromeContentClient::GetUserAgent should get its
string from a static method, so that other code in chrome can call that
directly. this is because chrome can't call content::GetContentClient.

if this isn't clear still, apologies. I can patch in your change and build upon
it.

[tfarina, 2014-03-10 00:25:49]

John, I have merged with your changes.

PTAL!

[jam, 2014-03-10 20:24:46]

lgtm

https://codereview.chromium.org/186883002/diff/290001/content/build/webkit_ve...
File content/build/webkit_version.h.in (right):

https://codereview.chromium.org/186883002/diff/290001/content/build/webkit_ve...
content/build/webkit_version.h.in:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
let's not create a directory for this one file. just leave it in
content/webkit_version.h.in

[tfarina, 2014-03-10 22:41:14]

https://codereview.chromium.org/186883002/diff/290001/content/build/webkit_ve...
File content/build/webkit_version.h.in (right):

https://codereview.chromium.org/186883002/diff/290001/content/build/webkit_ve...
content/build/webkit_version.h.in:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/03/10 20:24:47, jam wrote:
> let's not create a directory for this one file. just leave it in
> content/webkit_version.h.in

Done.

[tfarina, 2014-03-10 22:42:22]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-03-10 23:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/186883002/320001

[commit-bot: I haz the power, 2014-03-10 23:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/186883002/320001

[commit-bot: I haz the power, 2014-03-11 01:03:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 01:03:56]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tfarina, 2014-03-11 02:13:28]

TBRing Darin for the rest.

[tfarina, 2014-03-11 02:14:30]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-03-11 02:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/186883002/320001

[commit-bot: I haz the power, 2014-03-11 09:57:30]

Change committed as 256174