Allow the creation of partially spoofed user agents

Allow the creation of partially spoofed user agents

Refactors user agent generation so that we can create partially spoofed user
agents for other platforms.

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

[gone, 2012-08-22 18:22:58]

Hey James, is this what you were getting at in the other user agent review? 
I've added a function that takes in an OS string instead of having it built up
from actual system properties.

[gone, 2012-08-24 17:38:23]

Expanding the reviewer list.  Please have a look, if you can.

[gone, 2012-08-24 17:46:14]

Adding dpranke for the chrome/ changes based on review logs.

[tony, 2012-08-24 19:46:17]

Are there unittests for GetUserAgent on Android (or any other platform)?

https://chromiumcodereview.appspot.com/10854251/diff/9001/chrome/common/chrom...
File chrome/common/chrome_content_client.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/9001/chrome/common/chrom...
chrome/common/chrome_content_client.cc:433: std::string
ChromeContentClient::GetProduct(bool mobile_device) const {
I like the rename to GetProduct, but do we need to pass in the bool? Can't we
just += " Mobile" in the #if below?

[gone, 2012-08-24 20:35:10]

As far as I can tell, there aren't any unit tests that test GetUserAgent(). 
There's a stub in chrome::TestContentClient::GetUserAgent(), but I don't think
any test uses it explicitly.  I'm not sure what a unit test for this function
would look like, though.  Calling it just plumbs through to
webkit_glue::BuildUserAgentFromProduct() and builds up a very platform-specific
string.

https://chromiumcodereview.appspot.com/10854251/diff/9001/chrome/common/chrom...
File chrome/common/chrome_content_client.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/9001/chrome/common/chrom...
chrome/common/chrome_content_client.cc:433: std::string
ChromeContentClient::GetProduct(bool mobile_device) const {
On 2012/08/24 19:46:17, tony wrote:
> I like the rename to GetProduct, but do we need to pass in the bool? Can't we
> just += " Mobile" in the #if below?

Did it thinking that we might want to be able to create a mobile user agent in
the future, but I can't think of any cases where we actually need it at the
moment.  Moved that back down and simplified.

[tony, 2012-08-24 20:38:29]

LGTM.

I think it would be useful to have a unittest that verifies that on Android you
get "Mobile" in the user agent.  Maybe that's done somewhere else?  Anyway,
would be nice to have, not required.

[Yaron, 2012-08-24 20:43:59]

lgtm for android

[gone, 2012-08-24 21:00:34]

Adding OWNER/reviewer for chrome/ directory.

[James Hawkins, 2012-08-24 21:30:22]

chrome/ LGTM

[Charlie Reis, 2012-08-24 21:38:42]

I agree with Tony about adding a unit_test, since there's a fair amount of logic
involved with user agent strings now.

Just a few questions below.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
content/browser/android/content_view_core_impl.cc:103: // Automatically set it
for all WebContents.
I don't understand this comment.  Request desktop site isn't on by default, is
it?  Why would this be set for all WebContents?

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
File content/public/common/content_client.h (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
content/public/common/content_client.h:111: // Returns a string describing the
product.
Can you elaborate on what you mean by "product?"  It's not obvious outside the
context of user agents.

[gone, 2012-08-24 23:50:34]

> I agree with Tony about adding a unit_test, since there's a fair amount of
logic involved with user agent strings now.

I don't mind adding a unit test for GetUserAgent(), but I'm still not sure what
it should do.  The user agent function basically concatenates hard-coded
platform-specific strings and dynamically changing version numbers.  We'd
essentially be testing if each hard-coded string fragment in the code matches
the same hard-coded string fragment in the test, and I'm not sure that would be
all that useful.

If you're talking about more general unit tests for this CL, I'm open to
suggestions.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
content/browser/android/content_view_core_impl.cc:103: // Automatically set it
for all WebContents.
On 2012/08/24 21:38:43, creis wrote:
> I don't understand this comment.  Request desktop site isn't on by default, is
> it?  Why would this be set for all WebContents?

The main reason we set it here is that we don't save the user agent override out
in our version of the tab restoration pipeline.  The reasoning for doing so was
to ensure that our override string could report proper Chromium version numbers
while spoofing the rest of the string.  Since there isn't any way to set the
string manually, a user could potentially get stuck with an out-of-date string
until they opened a new tab and surfed with that.  It also saves some storage
space since we always override with the same string, but that's a minor thing.

As a consequence, the override has to be re-set for the WebContents when the
browser restarts; the constructor catches both this case and the case where the
user toggles the overrides on for a tab for the first time.  The alternative is
to set it in both of those locations; I don't have a strong opinion either way
and could switch over, if necessary.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
File content/public/common/content_client.h (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
content/public/common/content_client.h:111: // Returns a string describing the
product.
On 2012/08/24 21:38:43, creis wrote:
> Can you elaborate on what you mean by "product?"  It's not obvious outside the
> context of user agents.

I pulled the name from the existing GetUserAgent() code.  Would changing the
comment to
"Returns a string describing the embedder version.  Used as part of the user
agent string." make more sense?

[tony, 2012-08-24 23:55:58]

On 2012/08/24 23:50:34, dfalcantara wrote:
> > I agree with Tony about adding a unit_test, since there's a fair amount of
> logic involved with user agent strings now.
> 
> I don't mind adding a unit test for GetUserAgent(), but I'm still not sure
what
> it should do.  The user agent function basically concatenates hard-coded
> platform-specific strings and dynamically changing version numbers.  We'd
> essentially be testing if each hard-coded string fragment in the code matches
> the same hard-coded string fragment in the test, and I'm not sure that would
be
> all that useful.

Yes, I think it would be useful to verify that everything up until the version
number is correct.  That verifies that the hardcoded platform specific strings
are concatenated in the right order or don't get dropped by accident.

It's easy for me to imagine that someone makes a refactor unrelated to mobile
and the "Mobile" token gets moved or dropped.

[gone, 2012-08-25 00:00:38]

On 2012/08/24 23:55:58, tony wrote:
> Yes, I think it would be useful to verify that everything up until the version
> number is correct.  That verifies that the hardcoded platform specific strings
> are concatenated in the right order or don't get dropped by accident.
> 
> It's easy for me to imagine that someone makes a refactor unrelated to mobile
> and the "Mobile" token gets moved or dropped.

Ah, alright.  I'll go ahead and add one to that effect...

[Charlie Reis, 2012-08-25 00:11:10]

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
content/browser/android/content_view_core_impl.cc:103: // Automatically set it
for all WebContents.
On 2012/08/24 23:50:35, dfalcantara wrote:
> On 2012/08/24 21:38:43, creis wrote:
> > I don't understand this comment.  Request desktop site isn't on by default,
is
> > it?  Why would this be set for all WebContents?
> 
> The main reason we set it here is that we don't save the user agent override
out
> in our version of the tab restoration pipeline.  The reasoning for doing so
was
> to ensure that our override string could report proper Chromium version
numbers
> while spoofing the rest of the string.  Since there isn't any way to set the
> string manually, a user could potentially get stuck with an out-of-date string
> until they opened a new tab and surfed with that.  It also saves some storage
> space since we always override with the same string, but that's a minor thing.
> 
> As a consequence, the override has to be re-set for the WebContents when the
> browser restarts; the constructor catches both this case and the case where
the
> user toggles the overrides on for a tab for the first time.  The alternative
is
> to set it in both of those locations; I don't have a strong opinion either way
> and could switch over, if necessary.

Sorry if I'm just missing something basic, but is this just saying what the
overridden user agent should be if the user decides to use Request Desktop Site?
 (Rather than turning on the override by default?)  If so, please clarify that
in the comment, since I didn't get it.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
File content/public/common/content_client.h (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
content/public/common/content_client.h:111: // Returns a string describing the
product.
On 2012/08/24 23:50:35, dfalcantara wrote:
> On 2012/08/24 21:38:43, creis wrote:
> > Can you elaborate on what you mean by "product?"  It's not obvious outside
the
> > context of user agents.
> 
> I pulled the name from the existing GetUserAgent() code.  Would changing the
> comment to
> "Returns a string describing the embedder version.  Used as part of the user
> agent string." make more sense?

Yes, that would help a lot.

[gone, 2012-08-25 00:23:49]

Updated the comments; looking into a unit test now.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/browser/an...
content/browser/android/content_view_core_impl.cc:103: // Automatically set it
for all WebContents.
On 2012/08/25 00:11:10, creis wrote:
> Sorry if I'm just missing something basic, but is this just saying what the
> overridden user agent should be if the user decides to use Request Desktop
Site?
>  (Rather than turning on the override by default?)  If so, please clarify that
> in the comment, since I didn't get it.

D'oh, yeah, that's all that does.  It just sets the string for when it's needed
and doesn't turn it on by default.  I'll clarify in the comment.

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
File content/public/common/content_client.h (right):

https://chromiumcodereview.appspot.com/10854251/diff/14002/content/public/com...
content/public/common/content_client.h:111: // Returns a string describing the
product.
On 2012/08/25 00:11:10, creis wrote:
> On 2012/08/24 23:50:35, dfalcantara wrote:
> > On 2012/08/24 21:38:43, creis wrote:
> > > Can you elaborate on what you mean by "product?"  It's not obvious outside
> the
> > > context of user agents.
> > 
> > I pulled the name from the existing GetUserAgent() code.  Would changing the
> > comment to
> > "Returns a string describing the embedder version.  Used as part of the user
> > agent string." make more sense?
> 
> Yes, that would help a lot.

Will set it for the next patch.

[gone, 2012-08-28 17:32:45]

Added a unit test in chrome_content_client_unittest.cc; is this along the lines
of what you're looking for?

[tony, 2012-08-28 18:51:24]

Yes, that's what I had in mind.  The unittest lgtm.

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
File chrome/common/chrome_content_client_unittest.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
chrome/common/chrome_content_client_unittest.cc:12: namespace chrome {
Nit: This should be in the chrome_common namespace.

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
chrome/common/chrome_content_client_unittest.cc:14: void
CheckUserAgentStringOrdering(bool mobile_device) {
Nit: Maybe use an anonymous namespace for this helper function?

[Charlie Reis, 2012-08-28 19:14:14]

Thanks.  LGTM after Tony's suggestions.

[gone, 2012-08-28 20:25:53]

Thanks!  Sending it up to the CQ now.

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
File chrome/common/chrome_content_client_unittest.cc (right):

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
chrome/common/chrome_content_client_unittest.cc:12: namespace chrome {
On 2012/08/28 18:51:25, tony wrote:
> Nit: This should be in the chrome_common namespace.

I've moved the test into the chrome_common::, but oddly, ChromeContentClient
isn't in that namespace.  There doesn't seem to be any consistency in chrome/
though: chrome_common_net:: is used by some files in net/, and some files in the
mac/ directory lie in chrome::common::

https://chromiumcodereview.appspot.com/10854251/diff/22012/chrome/common/chro...
chrome/common/chrome_content_client_unittest.cc:14: void
CheckUserAgentStringOrdering(bool mobile_device) {
On 2012/08/28 18:51:25, tony wrote:
> Nit: Maybe use an anonymous namespace for this helper function?

Done.

[commit-bot: I haz the power, 2012-08-28 20:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10854251/31001

[commit-bot: I haz the power, 2012-08-28 23:25:59]

Change committed as 153783