Setup field trial for one-click signin inforbar.

Setup field trial for one-click signin inforbar. 

BUG=147227
TEST=There are two variants: the normal infobar and the blue-on-white infobar. Start chrome with one of the following command line options to force one or the other: 

--force-fieldtrials="OneClickSignIn/Standard/" 
--force-fieldtrials="OneClickSignIn/BlueOnWhite/"


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

[Roger Tawa OOO till Jul 10th, 2012-09-09 13:14:42]

Hi Peter, Scott, Steve,

This a redo combining CL 10914146 and CL 10913121.

Steve: nothing has changed from CL 10914146 wrt experiments.  Please take a look
at one_click_signin_helper.cc.

Peter: reworked as new inforbar type as discussed.

Scott: please do owner review.

Thanks.

[Peter Kasting, 2012-09-11 01:39:08]

Sorry I didn't get to this today; I'll try tomorrow.

[SteveT, 2012-09-11 02:16:25]

FieldTrial code lgtm. Some nits for you to look at...

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:70: 
nit: Is this extra break necessary?

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:409: session_index_, email_,
password_));
nit: Line up second line here.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:36: // Border
overrides:
nit: Indent to line up with virtual methods below.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:98: }
Should the dtor be declared in the class decl above, and then definition moved
up to match the order?

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:129: if (fix_color
&& ok_button() != NULL) {
Optional nit: It's been suggested to me in the past that when there is a big fat
if block that leads to the end of the method, it may be better to do an early
return, ie:

if (!fix_color || ok_button() == NULL)
  return;

This way, you don't have to keep as much context in your head while reading the
bits below. What do you think?

[Roger Tawa OOO till Jul 10th, 2012-09-11 19:23:21]

Thanks Steve.  Comments addressed, changes uploaded.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:70: 
On 2012/09/11 02:16:25, SteveT wrote:
> nit: Is this extra break necessary?

Done.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:409: session_index_, email_,
password_));
On 2012/09/11 02:16:25, SteveT wrote:
> nit: Line up second line here.

Done.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:36: // Border
overrides:
On 2012/09/11 02:16:25, SteveT wrote:
> nit: Indent to line up with virtual methods below.

Done.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:98: }
On 2012/09/11 02:16:25, SteveT wrote:
> Should the dtor be declared in the class decl above, and then definition moved
> up to match the order?

I think its fine the way it is.

http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:129: if (fix_color
&& ok_button() != NULL) {
On 2012/09/11 02:16:25, SteveT wrote:
> Optional nit: It's been suggested to me in the past that when there is a big
fat
> if block that leads to the end of the method, it may be better to do an early
> return, ie:
> 
> if (!fix_color || ok_button() == NULL)
>   return;
> 
> This way, you don't have to keep as much context in your head while reading
the
> bits below. What do you think?

Done.

[Peter Kasting, 2012-09-11 23:02:07]

LGTM

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
File chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:8:
SkColorSetARGB(0, 0, 0, 0);
Nit: Unless you need a fully-transparent value, just use SK_ColorBLACK.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:12: :
ConfirmInfoBarDelegate(infobar_service) {
Nit: Indent 4

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
File chrome/browser/api/infobars/one_click_signin_infobar_delegate.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.h:15: static const
SkColor kDefaultColor;
Nit: If your intent is always to either change all the colors or none, consider
eliminating this and replacing with a boolean member of AlternateColors called
"enabled" or some such.  This is a little clearer than specifying a default and
then checking to see if the colors differ from that, especially if for some
crazy reason we wanted to use an experimental color that happened to match the
"default" value.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:38: static void
InitializeFieldTrial();
Random drive-by nit: The Google style guide says static methods should be below
the constructor rather than above.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/confirm_infobar.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/confirm_infobar.h:28: protected:
Nit: I suggest adding a comment like:

// TODO(rogerta): These only need to be protected due to the
OneClickLoginInfoBar experiment and can be made private once that's removed.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/confirm_infobar.h:33: // InfoBarView:
Nit: Looks like only the destructor, ok_button(), and ViewHierarchyChanged()
need to be protected, the others can remain private.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/infobar_background.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/infobar_background.cc:14:
InfoBarBackground::InfoBarBackground(InfoBarDelegate::Type infobar_type)
Nit: I think if you nuke this constructor and make the few callers who use it
use your two-arg version instead, you can simplify this file (re-inline Init(),
remove dependency on infobar.h).

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:25: const int
kPreferredPaddingVertical = 5;
Nit: Not used

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:59: SkColor
InfoBarColoredButtonBorder::DarkenColor(SkColor color) {
Nit: Only called one place, maybe just inline this into the constructor body

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:108: 
Nit: One more blank line

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.h:14: class
OneClickLoginInfoBar : public ConfirmInfoBar {
Nit: The file says "signin" and the class says "login".  Be consistent (here and
everywhere else in the CL).

[Roger Tawa OOO till Jul 10th, 2012-09-12 15:07:13]

Thanks Peter.  Comments addressed, changes uploaded.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
File chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:8:
SkColorSetARGB(0, 0, 0, 0);
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: Unless you need a fully-transparent value, just use SK_ColorBLACK.

Removed default colour.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:12: :
ConfirmInfoBarDelegate(infobar_service) {
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: Indent 4

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
File chrome/browser/api/infobars/one_click_signin_infobar_delegate.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobar...
chrome/browser/api/infobars/one_click_signin_infobar_delegate.h:15: static const
SkColor kDefaultColor;
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: If your intent is always to either change all the colors or none,
consider
> eliminating this and replacing with a boolean member of AlternateColors called
> "enabled" or some such.  This is a little clearer than specifying a default
and
> then checking to see if the colors differ from that, especially if for some
> crazy reason we wanted to use an experimental color that happened to match the
> "default" value.

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.h:38: static void
InitializeFieldTrial();
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Random drive-by nit: The Google style guide says static methods should be
below
> the constructor rather than above.

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/confirm_infobar.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/confirm_infobar.h:28: protected:
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: I suggest adding a comment like:
> 
> // TODO(rogerta): These only need to be protected due to the
> OneClickLoginInfoBar experiment and can be made private once that's removed.

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/confirm_infobar.h:33: // InfoBarView:
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: Looks like only the destructor, ok_button(), and ViewHierarchyChanged()
> need to be protected, the others can remain private.

Right, but I didn't want to mess up the declaration order of the interface
implementation and break up the methods.  I think I'll leave it like this if no
objections.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/infobar_background.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/infobar_background.cc:14:
InfoBarBackground::InfoBarBackground(InfoBarDelegate::Type infobar_type)
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: I think if you nuke this constructor and make the few callers who use it
> use your two-arg version instead, you can simplify this file (re-inline
Init(),
> remove dependency on infobar.h).

I was trying to minimize changes since you didn't want changes to this file or
any of the other infobars.

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:25: const int
kPreferredPaddingVertical = 5;
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: Not used

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:59: SkColor
InfoBarColoredButtonBorder::DarkenColor(SkColor color) {
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: Only called one place, maybe just inline this into the constructor body

I'd rather leave the type of border_color_hot_ as "const SkColor", so it needs
to be initialized in the ctor initializer list.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:108: 
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: One more blank line

Done.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.h:14: class
OneClickLoginInfoBar : public ConfirmInfoBar {
On 2012/09/11 23:02:07, Peter Kasting wrote:
> Nit: The file says "signin" and the class says "login".  Be consistent (here
and
> everywhere else in the CL).

Done.

[Peter Kasting, 2012-09-12 17:21:23]

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/confirm_infobar.h (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/confirm_infobar.h:33: // InfoBarView:
On 2012/09/12 15:07:14, Roger Tawa wrote:
> On 2012/09/11 23:02:07, Peter Kasting wrote:
> > Nit: Looks like only the destructor, ok_button(), and ViewHierarchyChanged()
> > need to be protected, the others can remain private.
> 
> Right, but I didn't want to mess up the declaration order of the interface
> implementation and break up the methods.  I think I'll leave it like this if
no
> objections.

It doesn't matter much if we expect to rip this back out again shortly.  If we
wanted to leave things for any length of time I'd probably be more inclined to
reorder things.  Up to you.

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:59: SkColor
InfoBarColoredButtonBorder::DarkenColor(SkColor color) {
On 2012/09/12 15:07:14, Roger Tawa wrote:
> On 2012/09/11 23:02:07, Peter Kasting wrote:
> > Nit: Only called one place, maybe just inline this into the constructor body
> 
> I'd rather leave the type of border_color_hot_ as "const SkColor", so it needs
> to be initialized in the ctor initializer list.

Ah.  That's fine.  Maybe add a comment on the declaration of this function
noting that it's a helper so that we can easily perform its functionality from
the constructor initializer list.

[Roger Tawa OOO till Jul 10th, 2012-09-12 18:36:55]

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right):

http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:59: SkColor
InfoBarColoredButtonBorder::DarkenColor(SkColor color) {
On 2012/09/12 17:21:24, Peter Kasting wrote:
> On 2012/09/12 15:07:14, Roger Tawa wrote:
> > On 2012/09/11 23:02:07, Peter Kasting wrote:
> > > Nit: Only called one place, maybe just inline this into the constructor
body
> > 
> > I'd rather leave the type of border_color_hot_ as "const SkColor", so it
needs
> > to be initialized in the ctor initializer list.
> 
> Ah.  That's fine.  Maybe add a comment on the declaration of this function
> noting that it's a helper so that we can easily perform its functionality from
> the constructor initializer list.

Done.

[sky, 2012-09-13 23:32:13]

LGTM

[commit-bot: I haz the power, 2012-09-13 23:33:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/10911169/16001

[commit-bot: I haz the power, 2012-09-14 02:11:45]

Change committed as 156715