Make setting the user agent work with the zygote on linux.

Make setting the user agent work with the zygote on linux.

Previously the user agent was set when we set the content client,
which was set in the zygote, which didn't have a command line.

Now, we still set the user agent in the zygote, but if the
--user-agent command line switch is passed, we will reset the
user agent in ZygoteMain.

R=jam@chromium.org
BUG=99418
TEST=run chrome with --user-agent=foo on linux, visit
whatsmyuseragent.com, ensure 'foo' is displayed.

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

[jam, 2011-10-15 01:32:04]

http://codereview.chromium.org/8296011/diff/1/content/app/content_main.cc
File content/app/content_main.cc (right):

http://codereview.chromium.org/8296011/diff/1/content/app/content_main.cc#new...
content/app/content_main.cc:213: if (custom) {
nit: this file doesn't use brace brackets for single line statements

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h#newco...
webkit/glue/webkit_glue.h:100: // been called).
it seems a little redundant to provide two methods that do the same thing with
the only difference being that one has an extra check. why not just take out the
check altogether? if another caller runs into it, they'll just use the second
version

[Dirk Pranke, 2011-10-15 02:14:07]

http://codereview.chromium.org/8296011/diff/1/content/app/content_main.cc
File content/app/content_main.cc (right):

http://codereview.chromium.org/8296011/diff/1/content/app/content_main.cc#new...
content/app/content_main.cc:213: if (custom) {
On 2011/10/15 01:32:04, John Abd-El-Malek wrote:
> nit: this file doesn't use brace brackets for single line statements

Will fix.

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h#newco...
webkit/glue/webkit_glue.h:100: // been called).
On 2011/10/15 01:32:04, John Abd-El-Malek wrote:
> it seems a little redundant to provide two methods that do the same thing with
> the only difference being that one has an extra check. why not just take out
the
> check altogether? if another caller runs into it, they'll just use the second
> version

Well, Reset() should really only be called in extenuating circumstances (like
the zygote case). We could decide that we don't want to be this paranoid, but I
worry that we might introduce weird bugs by doing so. 

On the other hand, it would probably be better to get rid of this reset call and
the setuseragent call in content client, and just have the one set() call done
at the appropriate times and in the appropriate places.

WDYT?

As an aside, I would like to add some tests to make sure this doesn't break in
the future, but I'm not sure what the best way to test this would be. Would a
browser_test be the right way to test it (since we need to launch a renderer and
ensure it uses the right string)? Any idea how I could figure out what user
agent was actually used?

[jam, 2011-10-17 03:43:22]

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h#newco...
webkit/glue/webkit_glue.h:100: // been called).
On 2011/10/15 02:14:07, Dirk Pranke wrote:
> On 2011/10/15 01:32:04, John Abd-El-Malek wrote:
> > it seems a little redundant to provide two methods that do the same thing
with
> > the only difference being that one has an extra check. why not just take out
> the
> > check altogether? if another caller runs into it, they'll just use the
second
> > version
> 
> Well, Reset() should really only be called in extenuating circumstances (like
> the zygote case). We could decide that we don't want to be this paranoid, but
I
> worry that we might introduce weird bugs by doing so. 
> 
> On the other hand, it would probably be better to get rid of this reset call
and
> the setuseragent call in content client, and just have the one set() call done
> at the appropriate times and in the appropriate places.
> 
> WDYT?

sure, if you can find one place that calls this and which works in all processes
that would be better

> 
> As an aside, I would like to add some tests to make sure this doesn't break in
> the future, but I'm not sure what the best way to test this would be. Would a
> browser_test be the right way to test it (since we need to launch a renderer
and
> ensure it uses the right string)? Any idea how I could figure out what user
> agent was actually used?

browser_tests run in the context of the browser process, so I'm not sure how you
will verify what's happening in the renderer?

[Dirk Pranke, 2011-10-17 19:56:45]

On 2011/10/17 03:43:22, John Abd-El-Malek wrote:
> http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h
> File webkit/glue/webkit_glue.h (right):
> 
>
http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h#newco...
> webkit/glue/webkit_glue.h:100: // been called).
> On 2011/10/15 02:14:07, Dirk Pranke wrote:
> > On 2011/10/15 01:32:04, John Abd-El-Malek wrote:
> > > it seems a little redundant to provide two methods that do the same thing
> with
> > > the only difference being that one has an extra check. why not just take
out
> > the
> > > check altogether? if another caller runs into it, they'll just use the
> second
> > > version
> > 
> > Well, Reset() should really only be called in extenuating circumstances
(like
> > the zygote case). We could decide that we don't want to be this paranoid,
but
> I
> > worry that we might introduce weird bugs by doing so. 
> > 

Sorry, I meant, do you think this is being too paranoid?

> > As an aside, I would like to add some tests to make sure this doesn't break
in
> > the future, but I'm not sure what the best way to test this would be. Would
a
> > browser_test be the right way to test it (since we need to launch a renderer
> and
> > ensure it uses the right string)? Any idea how I could figure out what user
> > agent was actually used?
> 
> browser_tests run in the context of the browser process, so I'm not sure how
you
> will verify what's happening in the renderer?

I don't know, either, short of issuing a request to a web server and seeing what
user agent gets used somehow?

[jam, 2011-10-17 22:25:42]

On 2011/10/17 19:56:45, Dirk Pranke wrote:
> On 2011/10/17 03:43:22, John Abd-El-Malek wrote:
> > http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h
> > File webkit/glue/webkit_glue.h (right):
> > 
> >
>
http://codereview.chromium.org/8296011/diff/1/webkit/glue/webkit_glue.h#newco...
> > webkit/glue/webkit_glue.h:100: // been called).
> > On 2011/10/15 02:14:07, Dirk Pranke wrote:
> > > On 2011/10/15 01:32:04, John Abd-El-Malek wrote:
> > > > it seems a little redundant to provide two methods that do the same
thing
> > with
> > > > the only difference being that one has an extra check. why not just take
> out
> > > the
> > > > check altogether? if another caller runs into it, they'll just use the
> > second
> > > > version
> > > 
> > > Well, Reset() should really only be called in extenuating circumstances
> (like
> > > the zygote case). We could decide that we don't want to be this paranoid,
> but
> > I
> > > worry that we might introduce weird bugs by doing so. 
> > > 
> 
> Sorry, I meant, do you think this is being too paranoid?

I think it's not paranoid as much as unnecessary functions. Paranoid, IMO, would
be having one function that checks. But having two, with one not checking,
strikes me as not letting a paranoid person sleep at night, because there's an
escape hatch that's still available for use by anyone :)

> 
> > > As an aside, I would like to add some tests to make sure this doesn't
break
> in
> > > the future, but I'm not sure what the best way to test this would be.
Would
> a
> > > browser_test be the right way to test it (since we need to launch a
renderer
> > and
> > > ensure it uses the right string)? Any idea how I could figure out what
user
> > > agent was actually used?
> > 
> > browser_tests run in the context of the browser process, so I'm not sure how
> you
> > will verify what's happening in the renderer?
> 
> I don't know, either, short of issuing a request to a web server and seeing
what
> user agent gets used somehow?

[Dirk Pranke, 2011-10-24 21:15:13]

John, can you take another look please?

[jam, 2011-10-24 22:18:07]

lgtm