Remove webkit_glue::BuildUserAgent(), remove windows spoofing

Remove webkit_glue::BuildUserAgent(), change the contract in webkit_glue so that SetUserAgent() must be called before GetUserAgent().

This was causing a dependency inversion between webkit_support and its clients, and was needed for the content component build.

For content users, calling SetContentClient() will automatically initialize the user agent (retrieved from client->GetUserAgent()). 

As a bonus, fixing this allowed me to re-test the "mimic_windows" code path and it looks like we no longer need it.

R=jam@chromium.org,tony@chromium.org
BUG=11136, 90442
TEST=visit yahoo! mail using Chromium on Linux, ensure that we don't get an "unsupported browser" warning.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102336

[Dirk Pranke, 2011-09-21 20:02:48]

Please take a look. I think this patch is finally close to done, but I need to
run this version through the try bots again to ensure I didn't miss something.

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:304: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserAgent)) {
I'm not sure if we can be sure that CommandLine::ForCurrentProcess() is safe to
call here, but it seems to be okay.

[tony, 2011-09-21 20:15:05]

+evan is better reviewer for this code than I am

[Evan Martin, 2011-09-21 20:27:23]

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:303: std::string
ChromeContentClient::GetUserAgent(bool *overriding) const {
star on the left

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:304: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserAgent)) {
On 2011/09/21 20:02:48, Dirk Pranke wrote:
> I'm not sure if we can be sure that CommandLine::ForCurrentProcess() is safe
to
> call here, but it seems to be okay.

I think we don't have any real contract for it, so this is surely fine.

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const;
star on the left

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const;
Can you add OVERRIDE here to help ensure that you match the parent?

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:407: chrome_ua =
content::GetContentClient()->GetUserAgent(&overriding);
Is it guaranteed that there's a ContentClient here?

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:411: EXPECT_EQ(0,
chrome_ua.compare(ua));
EXPECT_EQ(ua, chrome_ua)  ?

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
File content/common/content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
content/common/content_client.cc:26: ua = client->GetUserAgent(&custom);
can merge this with the above line, if you want

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
File content/common/content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
content/common/content_client.h:81: virtual std::string GetUserAgent(bool
*overriding) const = 0;
star on the left

I don't quite get the doc: when overriding is true, it's saying I should always
obey the returned user agent?  As opposed to it being optional?

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
File content/shell/shell_content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
content/shell/shell_content_client.cc:32: std::string
ShellContentClient::GetUserAgent(bool *overriding) const {
star on the left

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
File content/shell/shell_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
content/shell/shell_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const OVERRIDE;
star on the left

http://codereview.chromium.org/7922023/diff/17023/content/test/test_content_c...
File content/test/test_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/test/test_content_c...
content/test/test_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const OVERRIDE;
star on the left

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.cc
File webkit/glue/user_agent.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.cc#n...
webkit/glue/user_agent.cc:130: // Replace Safari's Version/X string with the
product name/version passed in.
This comment appears to be obsolete.  Your call as to whether you want to change
it.

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.h
File webkit/glue/user_agent.h (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.h#ne...
webkit/glue/user_agent.h:25: std::string BuildUserAgentHelper(const std::string&
product);
It's not exactly your problem, but the word "Helper" in this function name is
completely uninformative.  I pre-LGTM any change you have that might make it
clearer.  (As well as documenting what "product" means here.)

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
webkit/glue/webkit_glue.cc:356: bool user_agent_is_set_;
Is it possible to remove this and instead use user_agent_.empty() in the tests
below?  (Along with disallowing setting an empty user agent.)

Not a suggestion, it's a question I'm asking you.  :)

[Dirk Pranke, 2011-09-21 21:02:35]

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:304: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserAgent)) {
On 2011/09/21 20:27:24, Evan Martin wrote:
> On 2011/09/21 20:02:48, Dirk Pranke wrote:
> > I'm not sure if we can be sure that CommandLine::ForCurrentProcess() is safe
> to
> > call here, but it seems to be okay.
> 
> I think we don't have any real contract for it, so this is surely fine.

ok.

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const;
On 2011/09/21 20:27:24, Evan Martin wrote:
> Can you add OVERRIDE here to help ensure that you match the parent?

Will fix the star ... I forgot about OVERRIDE. All of the functions should be
OVERRIDE, right?

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:407: chrome_ua =
content::GetContentClient()->GetUserAgent(&overriding);
On 2011/09/21 20:27:24, Evan Martin wrote:
> Is it guaranteed that there's a ContentClient here?

Nope :) In fact, this crashes. I will post a fix.

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:411: EXPECT_EQ(0,
chrome_ua.compare(ua));
On 2011/09/21 20:27:24, Evan Martin wrote:
> EXPECT_EQ(ua, chrome_ua)  ?

Will fix.

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
File content/common/content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
content/common/content_client.cc:26: ua = client->GetUserAgent(&custom);
On 2011/09/21 20:27:24, Evan Martin wrote:
> can merge this with the above line, if you want

Will do.

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
File content/common/content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
content/common/content_client.h:81: virtual std::string GetUserAgent(bool
*overriding) const = 0;
On 2011/09/21 20:27:24, Evan Martin wrote:
> star on the left
> 
> I don't quite get the doc: when overriding is true, it's saying I should
always
> obey the returned user agent?  As opposed to it being optional?

That is correct. I will attempt to make it clearer, but suggestions are welcome.

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
File content/shell/shell_content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
content/shell/shell_content_client.cc:32: std::string
ShellContentClient::GetUserAgent(bool *overriding) const {
On 2011/09/21 20:27:24, Evan Martin wrote:
> star on the left

Will fix.

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
File content/shell/shell_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/shell/shell_content...
content/shell/shell_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const OVERRIDE;
On 2011/09/21 20:27:24, Evan Martin wrote:
> star on the left

Will fix.

http://codereview.chromium.org/7922023/diff/17023/content/test/test_content_c...
File content/test/test_content_client.h (right):

http://codereview.chromium.org/7922023/diff/17023/content/test/test_content_c...
content/test/test_content_client.h:24: virtual std::string GetUserAgent(bool
*overriding) const OVERRIDE;
On 2011/09/21 20:27:24, Evan Martin wrote:
> star on the left

Will fix.

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.cc
File webkit/glue/user_agent.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.cc#n...
webkit/glue/user_agent.cc:130: // Replace Safari's Version/X string with the
product name/version passed in.
On 2011/09/21 20:27:24, Evan Martin wrote:
> This comment appears to be obsolete.  Your call as to whether you want to
change
> it.

Will fix.

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.h
File webkit/glue/user_agent.h (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/user_agent.h#ne...
webkit/glue/user_agent.h:25: std::string BuildUserAgentHelper(const std::string&
product);
On 2011/09/21 20:27:24, Evan Martin wrote:
> It's not exactly your problem, but the word "Helper" in this function name is
> completely uninformative.  I pre-LGTM any change you have that might make it
> clearer.  (As well as documenting what "product" means here.)

Agreed it's not a great name. I will attempt to fix it.

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
webkit/glue/webkit_glue.cc:356: bool user_agent_is_set_;
On 2011/09/21 20:27:24, Evan Martin wrote:
> Is it possible to remove this and instead use user_agent_.empty() in the tests
> below?  (Along with disallowing setting an empty user agent.)
> 
> Not a suggestion, it's a question I'm asking you.  :)

Yes, I think this is possible; it wasn't at some point during the work on this
patch because some clients had empty user agents, but I think that's no longer
true. I will attempt to make it so.

[jam, 2011-09-21 21:27:36]

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:407: chrome_ua =
content::GetContentClient()->GetUserAgent(&overriding);
On 2011/09/21 21:02:35, Dirk Pranke wrote:
> On 2011/09/21 20:27:24, Evan Martin wrote:
> > Is it guaranteed that there's a ContentClient here?
> 
> Nope :) In fact, this crashes. I will post a fix.

to be honest, this seems like not a very useful test

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
File content/common/content_client.cc (right):

http://codereview.chromium.org/7922023/diff/17023/content/common/content_clie...
content/common/content_client.cc:17: // TODO(dpranke): It's ugly to call
webkit_glue::SetUserAgent here, but
nit: reading this, it's not clear why is it ugly?

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
webkit/glue/webkit_glue.cc:401: // Should be removed if the sniffing is removed:
http://crbug.com/88211
this whole overriding thing seems like a lot of complexity just to support
testing this one block of code, on mac, when a user specifies a user agent flag.
why don't we just get rid of it?

[Dirk Pranke, 2011-09-21 22:19:59]

Changes uploaded. Please take another look :).

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/7922023/diff/17023/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:407: chrome_ua =
content::GetContentClient()->GetUserAgent(&overriding);
On 2011/09/21 21:27:36, John Abd-El-Malek wrote:
> On 2011/09/21 21:02:35, Dirk Pranke wrote:
> > On 2011/09/21 20:27:24, Evan Martin wrote:
> > > Is it guaranteed that there's a ContentClient here?
> > 
> > Nope :) In fact, this crashes. I will post a fix.
> 
> to be honest, this seems like not a very useful test

I agree, but I'm not gonna delete it in this change unless someone tells me I
should :).

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
webkit/glue/webkit_glue.cc:401: // Should be removed if the sniffing is removed:
http://crbug.com/88211
On 2011/09/21 21:27:36, John Abd-El-Malek wrote:
> this whole overriding thing seems like a lot of complexity just to support
> testing this one block of code, on mac, when a user specifies a user agent
flag.
> why don't we just get rid of it?

It's a good question; I'm not sure what the impact would be, but probably almost
minimal. I will follow up on this in a separate bug if that's okay.

[jam, 2011-09-21 22:26:29]

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
File webkit/glue/webkit_glue.cc (right):

http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
webkit/glue/webkit_glue.cc:401: // Should be removed if the sniffing is removed:
http://crbug.com/88211
On 2011/09/21 22:19:59, Dirk Pranke wrote:
> On 2011/09/21 21:27:36, John Abd-El-Malek wrote:
> > this whole overriding thing seems like a lot of complexity just to support
> > testing this one block of code, on mac, when a user specifies a user agent
> flag.
> > why don't we just get rid of it?
> 
> It's a good question; I'm not sure what the impact would be, but probably
almost
> minimal. I will follow up on this in a separate bug if that's okay.

given that it adds some complexity to ContentClient, I think it's worth it to
look into this so that we don't add a possibly unnecessary parameter

[Dirk Pranke, 2011-09-21 22:38:31]

On Wed, Sep 21, 2011 at 3:26 PM,  <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc
> File webkit/glue/webkit_glue.cc (right):
>
>
http://codereview.chromium.org/7922023/diff/17023/webkit/glue/webkit_glue.cc#...
> webkit/glue/webkit_glue.cc:401: // Should be removed if the sniffing is
> removed: http://crbug.com/88211
> On 2011/09/21 22:19:59, Dirk Pranke wrote:
>>
>> On 2011/09/21 21:27:36, John Abd-El-Malek wrote:
>> > this whole overriding thing seems like a lot of complexity just to
>
> support
>>
>> > testing this one block of code, on mac, when a user specifies a user
>
> agent
>>
>> flag.
>> > why don't we just get rid of it?
>
>> It's a good question; I'm not sure what the impact would be, but
>
> probably almost
>>
>> minimal. I will follow up on this in a separate bug if that's okay.
>
> given that it adds some complexity to ContentClient, I think it's worth
> it to look into this so that we don't add a possibly unnecessary
> parameter
>

Actually, having thought about it further, I think we should leave it
the way it is (with the overriding). The rationale is that (a) we
clearly need to spoof in the normal/default case, and (b) I think as
long as we support overriding at all -- which is clearly useful from a
debugging point of view -- we should *always* override; not to do so
would at the very least make it impossible to test if the spoofing in
(a) was still needed, and more importantly I think would just be
confusing.

I agree that the interface is less than ideal, but I'm not sure I see
a better alternative. In patchset #9/#10 I flipped the code around so
that embedders would call content::SetUserAgent(user_agent,
overriding), which was slightly less crufty, but as we discussed, this
didn't really fit w/ the rest of the ContentClient design.

> http://codereview.chromium.org/7922023/
>

[Dirk Pranke, 2011-09-21 23:09:17]

Tommi, can you take a look at the changes I had to make to
chrome_frame/renderer_glue and chrome_frame/tests/html_util_unittests?

[jam, 2011-09-21 23:31:18]

lgtm

[tommi (sloooow) - chröme, 2011-09-22 08:58:46]

lgtm for html_util_unittests.cc with a couple of nits.

http://codereview.chromium.org/7922023/diff/20060/chrome_frame/test/html_util...
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/7922023/diff/20060/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:407: 
nit: remove empty line

http://codereview.chromium.org/7922023/diff/20060/chrome_frame/test/html_util...
chrome_frame/test/html_util_unittests.cc:413: std::string chrome_ua =
webkit_glue::BuildUserAgentFromProduct(product);
nit: constructor syntax for complex types as is done elsewhere.