Added a function to BrowserMainParts that is called after about_flags has con...

Added a function to BrowserMainParts that sets up field trials. It is called after about_flags has converted the flags to command-line switches. This fixes the case where a field trial would opt a user out of a feature even when they'd explicitly enabled it in their flags.

Contributed by: dominich@chromium.org

BUG=none
TEST=Enable page-prerender through about:flags and restart. You should skip the random part of the Field Test.

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

[dominich, 2011-01-25 20:17:46]

As discussed with cbentzel

[cbentzel, 2011-01-25 21:50:21]

Adding trungl - this looks OK for prerender/prefetch, but I'm not sure about how
it fits into BrowserMainParts.

http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1270: parts->PostAboutFlagsConvertedToSwitches();
This should be done directly after ConvertFlagsToSwitches

[cbentzel, 2011-01-25 21:51:20]

Adding a few others who have been doing about:flags changes as well.

On Tue, Jan 25, 2011 at 4:50 PM, <cbentzel@chromium.org> wrote:

> Adding trungl - this looks OK for prerender/prefetch, but I'm not sure
> about how
> it fits into BrowserMainParts.
>
>
>
> http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
>
>
>
http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc#...
> chrome/browser/browser_main.cc:1270:
> parts->PostAboutFlagsConvertedToSwitches();
> This should be done directly after ConvertFlagsToSwitches
>
>
> http://codereview.chromium.org/6354023/
>

[Nico, 2011-01-25 21:53:10]

Is there any way to do this without making about_flags even more complex?

[dominich, 2011-01-25 22:00:41]

On 2011/01/25 21:53:10, Nico wrote:
> Is there any way to do this without making about_flags even more complex?

I'm not sure why you think I'm making about_flags more complex. This change is
localized to browser main. It does make the logic there a little more complex,
but I'm trying to change as little as possible. Theoretically, all the field
tests could be moved to the PostFlagConvert... method.

[dominich, 2011-01-25 22:00:45]

http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6354023/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1270: parts->PostAboutFlagsConvertedToSwitches();
On 2011/01/25 21:50:21, cbentzel wrote:
> This should be done directly after ConvertFlagsToSwitches

Done.

[Nico, 2011-01-25 22:09:56]

On 2011/01/25 22:00:41, dominic wrote:
> On 2011/01/25 21:53:10, Nico wrote:
> > Is there any way to do this without making about_flags even more complex?
> 
> I'm not sure why you think I'm making about_flags more complex. This change is
> localized to browser main. It does make the logic there a little more complex,
> but I'm trying to change as little as possible. Theoretically, all the field
> tests could be moved to the PostFlagConvert... method.

Yes, but it adds more knowledge about the prefs -> commandline flags conversion
to browser_main. Why is this necessary? Do you check for your flag before the
conversion happens? Can you move your check to be later?

(the flags stuff recently got moved from the profile into local state, so the
conversion now happens earlier than it used to. maybe that help?)

[Nico, 2011-01-25 22:10:01]

s

[dominich, 2011-01-25 22:17:18]

On 2011/01/25 22:09:56, Nico wrote:
> On 2011/01/25 22:00:41, dominic wrote:
> > On 2011/01/25 21:53:10, Nico wrote:
> > > Is there any way to do this without making about_flags even more complex?
> > 
> > I'm not sure why you think I'm making about_flags more complex. This change
is
> > localized to browser main. It does make the logic there a little more
complex,
> > but I'm trying to change as little as possible. Theoretically, all the field
> > tests could be moved to the PostFlagConvert... method.
> 
> Yes, but it adds more knowledge about the prefs -> commandline flags
conversion
> to browser_main. Why is this necessary? Do you check for your flag before the
> conversion happens? Can you move your check to be later?

This is doing exactly that - it's moving the checks to just after the
command-line flags are updated from the local state.

> (the flags stuff recently got moved from the profile into local state, so the
> conversion now happens earlier than it used to. maybe that help?)

They didn't go early enough - the field trials are being run as part of
EarlyInitialization but the conversion is still after that.

[dominich, 2011-01-25 22:51:05]

After discussion with Nico I've moved the other Field trial setup functions to
the same place as the Prefetch trial and updated the method names accordingly.

[cbentzel, 2011-01-25 22:51:41]

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main...
chrome/browser/browser_main.cc:196: base::FieldTrial::EnableBenchmarking();
Why didn't this move to SetupFieldTrials? Did it need to be called early?

[Nico, 2011-01-25 22:58:07]

LG

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1259: // field trials
Nit "." at end of comment

[dominich, 2011-01-25 22:59:26]

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main...
chrome/browser/browser_main.cc:196: base::FieldTrial::EnableBenchmarking();
On 2011/01/25 22:51:41, cbentzel wrote:
> Why didn't this move to SetupFieldTrials? Did it need to be called early?

My first thought was that it isn't enabled through about:flags, and while you're
right that this should be in the SetupFieldTrials() method for completeness,
there may be some field trials that need to be set up earlier. This would then
need to be called before those. I thought it safer to leave it.

[cbentzel, 2011-01-25 23:30:54]

LGTM

On Tue, Jan 25, 2011 at 5:59 PM, <dominich@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
>
>
>
http://codereview.chromium.org/6354023/diff/11002/chrome/browser/browser_main...
> chrome/browser/browser_main.cc:196:
> base::FieldTrial::EnableBenchmarking();
>
> On 2011/01/25 22:51:41, cbentzel wrote:
>
>> Why didn't this move to SetupFieldTrials? Did it need to be called
>>
> early?
>
> My first thought was that it isn't enabled through about:flags, and
> while you're right that this should be in the SetupFieldTrials() method
> for completeness, there may be some field trials that need to be set up
> earlier. This would then need to be called before those. I thought it
> safer to leave it.
>
>
> http://codereview.chromium.org/6354023/
>