Adding show profile switcher field trial.

Adding show profile switcher field trial. 

This sets up a finch server style server controlled field trial to experiment with having the profile switcher always visible. 

We want to see the effect this has on sign in. 


BUG=172872
TEST=start chrome with --force-fieldtrials=ShowProfileSwitcher/AlwaysShow/ either with a clean user data dir or only one profile. The avatar menu should be visible and there should only be one profile in the profile switcher. You should also be able to edit that profile.

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

[jwd, 2013-01-29 15:40:42]

Roger for general review
sail@ for profiles

I have some tests for ShouldShowAvatareMenu in the works.

[Roger Tawa OOO till Jul 10th, 2013-01-29 15:53:44]

lgtm with the tests

[sail, 2013-01-29 18:17:23]

LGTM
When is the field trial supposed to end? We should have a bug to track the work
to remove this code once it ends.

[jwd, 2013-01-30 16:34:45]

On 2013/01/29 18:17:23, sail wrote:
> LGTM
> When is the field trial supposed to end? We should have a bug to track the
work
> to remove this code once it ends.

+sky for ui owners
+stevet for field trials owners

@Sail, It's currently only planned for M26 dev and beta, and depending on the
results we may let it go to stable. I'll setup the server experiment configs to
prevent it from going to stable unless we want it to, and keep it on 26. 
Is the experiment meta bug crbug.com/172476 not enough to track it? (it stays
open until the trial is no longer affects live traffic) Or do you want something
specifically mentioning the code?

[SteveT, 2013-01-30 16:53:42]

LGTM for FieldTrial stuff. Please add a TEST= line with instructions on how to
test this using the --force-fieldtrial flag.

[sail, 2013-01-30 17:53:35]

On 2013/01/30 16:34:45, Jesse Doherty wrote:
> On 2013/01/29 18:17:23, sail wrote:
> > LGTM
> > When is the field trial supposed to end? We should have a bug to track the
> work
> > to remove this code once it ends.
> 
> +sky for ui owners
> +stevet for field trials owners
> 
> @Sail, It's currently only planned for M26 dev and beta, and depending on the
> results we may let it go to stable. I'll setup the server experiment configs
to
> prevent it from going to stable unless we want it to, and keep it on 26. 
> Is the experiment meta bug crbug.com/172476 not enough to track it? (it stays
> open until the trial is no longer affects live traffic) Or do you want
something
> specifically mentioning the code?

Using bug 172476 to track it sounds great. I was worried we were going to close
that bug once the trial started.

[sky, 2013-01-30 18:14:08]

LGTM

[jwd, 2013-01-31 16:48:00]

On 2013/01/30 18:14:08, sky wrote:
> LGTM

@Sailesh: I've changed the how the testing works and I've made
IsMultipleProfilesEnabled able to be forced to a particular state for testing.
Let me know what you think.

[sail, 2013-01-31 17:15:23]

Hi Jesse, I don't understand why you would want to enable multi-profile support.
The only time its disabled is when the platform doesn't support it. We would
never want to override that. If there's a good reason for it then it need a
comment.

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1040: if
(is_multiple_profiles_forced_)
I think this should move to the very bottom

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:361: static bool
is_multiple_profiles_forced_;
maybe use an enum instead of 2 bools, something like
  default
  force_on
  force_off

[jwd, 2013-01-31 19:25:49]

On 2013/01/31 17:15:23, sail wrote:
> Hi Jesse, I don't understand why you would want to enable multi-profile
support.
> The only time its disabled is when the platform doesn't support it. We would
> never want to override that. If there's a good reason for it then it need a
> comment.

I added a comment. Basically I want to be able to mock the behavior of
IsMultipleProfilesEnabled in the ShouldShowAvatarMenu unit tests. Those tests
don't actually care about the platform stuff in IsMultiplePRofilesEnabled. 

The simplest alternative to this would be to have these tests behave differently
depending on what IsMultipleProfilesEnabled returns. I don't mind doing that,
having all that code in profile_manager is a bit ugly.

If you have a better suggestion let me know.

> 
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_manager.cc (right):
> 
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_manager.cc:1040: if
> (is_multiple_profiles_forced_)
> I think this should move to the very bottom
> 
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_manager.h (right):
> 
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_manager.h:361: static bool
> is_multiple_profiles_forced_;
> maybe use an enum instead of 2 bools, something like
>   default
>   force_on
>   force_off

[jwd, 2013-01-31 19:26:11]

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1040: if
(is_multiple_profiles_forced_)
On 2013/01/31 17:15:23, sail wrote:
> I think this should move to the very bottom

See new comment and response.

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:361: static bool
is_multiple_profiles_forced_;
On 2013/01/31 17:15:23, sail wrote:
> maybe use an enum instead of 2 bools, something like
>   default
>   force_on
>   force_off

Done. Kept the api the same though.

[sail, 2013-01-31 20:31:10]

On 2013/01/31 19:25:49, Jesse Doherty wrote:
> On 2013/01/31 17:15:23, sail wrote:
> > Hi Jesse, I don't understand why you would want to enable multi-profile
> support.
> > The only time its disabled is when the platform doesn't support it. We would
> > never want to override that. If there's a good reason for it then it need a
> > comment.
> 
> I added a comment. Basically I want to be able to mock the behavior of
> IsMultipleProfilesEnabled in the ShouldShowAvatarMenu unit tests. Those tests
> don't actually care about the platform stuff in IsMultiplePRofilesEnabled. 

I'd rather not do this in tests.
If you still think this is important to change in tests then rename the API to
something like:
  SetMultipleProfilesEnabledForTesting(bool enabled);
I don't think you need a "use default" setting.

> 
> The simplest alternative to this would be to have these tests behave
differently
> depending on what IsMultipleProfilesEnabled returns. I don't mind doing that,
> having all that code in profile_manager is a bit ugly.

Yea, have the test behave different might be better.

> 
> If you have a better suggestion let me know.
> 
> > 
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_manager.cc:1040: if
> > (is_multiple_profiles_forced_)
> > I think this should move to the very bottom
> > 
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_manager.h:361: static bool
> > is_multiple_profiles_forced_;
> > maybe use an enum instead of 2 bools, something like
> >   default
> >   force_on
> >   force_off

[jwd, 2013-01-31 21:06:20]

Ok, I changed it to have the tests have different behavior depending on
IsMultipleProfilesEnabled. Have a look.

On 2013/01/31 20:31:10, sail wrote:
> On 2013/01/31 19:25:49, Jesse Doherty wrote:
> > On 2013/01/31 17:15:23, sail wrote:
> > > Hi Jesse, I don't understand why you would want to enable multi-profile
> > support.
> > > The only time its disabled is when the platform doesn't support it. We
would
> > > never want to override that. If there's a good reason for it then it need
a
> > > comment.
> > 
> > I added a comment. Basically I want to be able to mock the behavior of
> > IsMultipleProfilesEnabled in the ShouldShowAvatarMenu unit tests. Those
tests
> > don't actually care about the platform stuff in IsMultiplePRofilesEnabled. 
> 
> I'd rather not do this in tests.
> If you still think this is important to change in tests then rename the API to
> something like:
>   SetMultipleProfilesEnabledForTesting(bool enabled);
> I don't think you need a "use default" setting.
> 
> > 
> > The simplest alternative to this would be to have these tests behave
> differently
> > depending on what IsMultipleProfilesEnabled returns. I don't mind doing
that,
> > having all that code in profile_manager is a bit ugly.
> 
> Yea, have the test behave different might be better.
> 
> > 
> > If you have a better suggestion let me know.
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > > File chrome/browser/profiles/profile_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > > chrome/browser/profiles/profile_manager.cc:1040: if
> > > (is_multiple_profiles_forced_)
> > > I think this should move to the very bottom
> > > 
> > >
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > > File chrome/browser/profiles/profile_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12040085/diff/13016/chrome/browser/profiles/p...
> > > chrome/browser/profiles/profile_manager.h:361: static bool
> > > is_multiple_profiles_forced_;
> > > maybe use an enum instead of 2 bools, something like
> > >   default
> > >   force_on
> > >   force_off

[sail, 2013-01-31 21:09:00]

LGTM

[commit-bot: I haz the power, 2013-01-31 21:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/12040085/23001

[commit-bot: I haz the power, 2013-02-01 01:12:13]

Change committed as 180027