Update behavior of one-click infobar to remove modal dialog, add "undo".

Update behavior of one-click infobar to remove modal dialog, add "undo".

There are many non-signin-related files in this CL because of a method signature change.  The significant changes are in the one_click_* files.

BUG=125253
TEST=See bug description for complete UX spec.


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

[akalin, 2012-05-16 23:00:12]

Taking a look at this.  I'll patch this in and get it working (compiling +
tests) on Mac/Linux, but I think any UI changes for those platforms should be
done after this lands.  I suspect it'll be a pain to coordinate my and your
patches, and landing UI changes for Mac is already an involved process.

[akalin, 2012-05-16 23:36:20]

On 2012/05/16 23:00:12, akalin wrote:
> Taking a look at this.  I'll patch this in and get it working (compiling +
> tests) on Mac/Linux, but I think any UI changes for those platforms should be
> done after this lands.  I suspect it'll be a pain to coordinate my and your
> patches, and landing UI changes for Mac is already an involved process.

Can you rebase?  I can't patch it in on ToT (says one_click_signin_helper.cc
doesn't apply)

[akalin, 2012-05-17 00:39:24]

On second thought, it might be easier to just do this as a code review, since
you'll have to revert most of your mac changes, anyway.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (left):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:21: IBOutlet
NSButton* learnMoreLink_;
leave this variable in, and add a TODO for me to remove it (it requires nib
changes)

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:33:
start_sync_callback:(const base::Callback<void(bool)>&)start_sync_callback;
obj-c style is to align the :s.  If you use 'const StartSyncCallback&' I think
it'll fit

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:35: // Starts sync
and closes teh bubble..
typo

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:38: // Just closes
the bubble.
this would require nib changes too.  Just leave this out and put a TODO for me
to add it in.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (left):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:84: totalYOffset
+=
why did you remove this?

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:32:
start_sync_callback:(const base::Callback<void(bool)>&)
line up : here too

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:63: //
TODO(rogerta): why not close bubble now?
Navigating to a page already closes the infobar on Mac.  But, yeah, you can
explicitly close here for completeness.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:54:
EXPECT_CALL(*this, OnStartSync()).Times(0);
you want _

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:72:
EXPECT_CALL(*this, OnStartSync(testing::_)).Times(0);
put 'using ::testing::_;' at top of file

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:85:
[controller_.release() close:nil];
you'll want to remove the close line and instead have:

[controller_.release() onClickAdvancedLink:nil];

if auto-closing

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:89: //
TODO(rogerta: what is the point of this test?
if you change the advanced handler to auto-close, then you can remove this test.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:71: // Undo Button.
add a TODO for me to figure out the spacing

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:47: bool
call_sync_callback_on_close_;
seems cleaner to me to remove the bool, make the callback non-const, have a
method like 

void MaybeStartSync(x) { if (!start_sync_callback_.is_null())
base::ResetAndRun(&start_sync_callback_).Run(x); }

Then you can replace the bool fiddling with MaybeStartSync() calls

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/sync/one_...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_helper.cc:144: profile, session_index,
email, password, use_default_settings));
Can you turn use_default_settings into an enum?  I was confused as to what the
bool meant until I looked for the definition, and it wasn't what I originally
thought.

I suggest putting it in one_click_signin_sync_starter.h, maybe:

enum StartSyncMode { SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST };

and also

typedef base::Callback<void(StartSyncMode)> StartSyncCallback;

[Roger Tawa OOO till Jul 10th, 2012-05-17 21:17:37]

Thanks Fred.  Still working on it, but please take another look.  Passing
through the try bots now.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (left):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:21: IBOutlet
NSButton* learnMoreLink_;
On 2012/05/17 00:39:24, akalin wrote:
> leave this variable in, and add a TODO for me to remove it (it requires nib
> changes)

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:35: // Starts sync
and closes teh bubble..
On 2012/05/17 00:39:24, akalin wrote:
> typo

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:38: // Just closes
the bubble.
On 2012/05/17 00:39:24, akalin wrote:
> this would require nib changes too.  Just leave this out and put a TODO for me
> to add it in.

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (left):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:84: totalYOffset
+=
On 2012/05/17 00:39:24, akalin wrote:
> why did you remove this?

I removed this because the learn more link is no longer supposed to be in the
bubble.  However, since I did not change the nib thing yet, I'll put the code
back in with a comment for you to take it out.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:32:
start_sync_callback:(const base::Callback<void(bool)>&)
On 2012/05/17 00:39:24, akalin wrote:
> line up : here too

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:63: //
TODO(rogerta): why not close bubble now?
On 2012/05/17 00:39:24, akalin wrote:
> Navigating to a page already closes the infobar on Mac.  But, yeah, you can
> explicitly close here for completeness.

Will close explicitly then.  The advanced page comes up in a new tab.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:54:
EXPECT_CALL(*this, OnStartSync()).Times(0);
On 2012/05/17 00:39:24, akalin wrote:
> you want _

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:72:
EXPECT_CALL(*this, OnStartSync(testing::_)).Times(0);
On 2012/05/17 00:39:24, akalin wrote:
> put 'using ::testing::_;' at top of file

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:85:
[controller_.release() close:nil];
On 2012/05/17 00:39:24, akalin wrote:
> you'll want to remove the close line and instead have:
> 
> [controller_.release() onClickAdvancedLink:nil];
> 
> if auto-closing

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/cocoa/one...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:89: //
TODO(rogerta: what is the point of this test?
On 2012/05/17 00:39:24, akalin wrote:
> if you change the advanced handler to auto-close, then you can remove this
test.

Added a TODO comment.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:71: // Undo Button.
On 2012/05/17 00:39:24, akalin wrote:
> add a TODO for me to figure out the spacing

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/gtk/one_c...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:47: bool
call_sync_callback_on_close_;
On 2012/05/17 00:39:24, akalin wrote:
> seems cleaner to me to remove the bool, make the callback non-const, have a
> method like 
> 
> void MaybeStartSync(x) { if (!start_sync_callback_.is_null())
> base::ResetAndRun(&start_sync_callback_).Run(x); }
> 
> Then you can replace the bool fiddling with MaybeStartSync() calls

Done.

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/sync/one_...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_helper.cc:144: profile, session_index,
email, password, use_default_settings));
On 2012/05/17 00:39:24, akalin wrote:
> Can you turn use_default_settings into an enum?  I was confused as to what the
> bool meant until I looked for the definition, and it wasn't what I originally
> thought.
> 
> I suggest putting it in one_click_signin_sync_starter.h, maybe:
> 
> enum StartSyncMode { SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST };
> 
> and also
> 
> typedef base::Callback<void(StartSyncMode)> StartSyncCallback;

I'll see how best to handle this.  I'm not sure I want to declare this type in
browser_window.h.

[akalin, 2012-05-18 17:50:28]

http://codereview.chromium.org/10332185/diff/8003/chrome/browser/ui/sync/one_...
> chrome/browser/ui/sync/one_click_signin_helper.cc:144: profile, session_index,
> email, password, use_default_settings));
> On 2012/05/17 00:39:24, akalin wrote:
> > Can you turn use_default_settings into an enum?  I was confused as to what
the
> > bool meant until I looked for the definition, and it wasn't what I
originally
> > thought.
> > 
> > I suggest putting it in one_click_signin_sync_starter.h, maybe:
> > 
> > enum StartSyncMode { SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST };
> > 
> > and also
> > 
> > typedef base::Callback<void(StartSyncMode)> StartSyncCallback;
> 
> I'll see how best to handle this.  I'm not sure I want to declare this type in
> browser_window.h.

Can't it also be in one_click_signin_sync_starter.h?

[akalin, 2012-05-18 17:57:03]

looks good, but let's figure out the callback typedef issue.

http://codereview.chromium.org/10332185/diff/18002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9362: +      <message
name="IDS_ONE_CLICK_BUBBLE_UNDO" desc="The text for the undo button in the one
click signin bubble..">
.. -> .

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:42: //
TODO(akalin): The following line should be uncommented once the nib is
I think you should just remove this completely, and have the TODO be to hook up
the Undo button to close the dialog. (It might be possible to do that completely
in the .nib)

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:57: -
(IBAction)undo:(id)sender {
remove this, too, since nothing is calling it yet

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:73: /*
TODO(akalin): enable this test once the undo button is implemented.
move the TODO to the above comment and remove the commented-out code completely.
(I can always check this code review if I need to resurrect it.)

[Roger Tawa OOO till Jul 10th, 2012-05-23 17:13:02]

Hi Fred,

All mac and linux builds are now compiling.  Addressed your comments, please
take another look.  Thanks.

http://codereview.chromium.org/10332185/diff/18002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9362: +      <message
name="IDS_ONE_CLICK_BUBBLE_UNDO" desc="The text for the undo button in the one
click signin bubble..">
On 2012/05/18 17:57:03, akalin wrote:
> .. -> .

Done.

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:42: //
TODO(akalin): The following line should be uncommented once the nib is
On 2012/05/18 17:57:03, akalin wrote:
> I think you should just remove this completely, and have the TODO be to hook
up
> the Undo button to close the dialog. (It might be possible to do that
completely
> in the .nib)

Done.

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:57: -
(IBAction)undo:(id)sender {
On 2012/05/18 17:57:03, akalin wrote:
> remove this, too, since nothing is calling it yet

Done.

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/18002/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:73: /*
TODO(akalin): enable this test once the undo button is implemented.
On 2012/05/18 17:57:03, akalin wrote:
> move the TODO to the above comment and remove the commented-out code
completely.
> (I can always check this code review if I need to resurrect it.)

Done.

[akalin, 2012-05-23 21:15:47]

http://codereview.chromium.org/10332185/diff/35/chrome/browser/ui/browser_win...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10332185/diff/35/chrome/browser/ui/browser_win...
chrome/browser/ui/browser_window.h:57: typedef base::Callback<void(bool)>
OneClickSigninBubbleCallback;
I'm sorry to be a stickler about this, but:

- What happened to the enum? (instead of the bool) ( enum StartSyncMode {
SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST }; )

- OneClickSigninBubbleCallback isn't very descriptive -- it doesn't tell me what
it actually does, which is to start sync.  Why not my suggested name,
StartSyncCallback?

- The natural place for the enum (and thus the callback) is to me
one_click_signin_sync_starter.h, which this file can include.  Having to pull in
browser_window.h (relatively large) just for StartSyncCallback seems suboptimal
to me.

[Roger Tawa OOO till Jul 10th, 2012-05-24 14:38:59]

http://codereview.chromium.org/10332185/diff/35/chrome/browser/ui/browser_win...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10332185/diff/35/chrome/browser/ui/browser_win...
chrome/browser/ui/browser_window.h:57: typedef base::Callback<void(bool)>
OneClickSigninBubbleCallback;
On 2012/05/23 21:15:48, akalin wrote:
> I'm sorry to be a stickler about this, but:
> 
> - What happened to the enum? (instead of the bool) ( enum StartSyncMode {
> SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST }; )
> 
> - OneClickSigninBubbleCallback isn't very descriptive -- it doesn't tell me
what
> it actually does, which is to start sync.  Why not my suggested name,
> StartSyncCallback?
> 
> - The natural place for the enum (and thus the callback) is to me
> one_click_signin_sync_starter.h, which this file can include.  Having to pull
in
> browser_window.h (relatively large) just for StartSyncCallback seems
suboptimal
> to me.

Sorry Fred, my bad.  I was going to do the enum but forgot before sending the
review.  As for name of class I'm OK with that.

However, I don't think one_click_signin_sync_starter.h is the right place.  I do
think browser_window.h is the closest header file since the callback is needed
by any class that implements BrowserWindow or calls on BrowserWindow (which are
the various one click bubble implementations for the various platforms).  Its
not used at all by OneClickSigninSyncStarter.  Note that I could also declare
the type inside the class BrowserWindow so as to not pollute the namespace.

If you don't think browser_window.h is the right place, then I think the best
other alternative is to put it in its own header file, much like
base\callback_forward.h, and then include this header where needed.

[akalin, 2012-05-24 17:14:13]

On Thu, May 24, 2012 at 7:38 AM, <rogerta@chromium.org> wrote:

> However, I don't think one_click_signin_sync_starter.**h is the right
> place.  I do think browser_window.h is the closest header file since the
> callback is needed by any class that implements BrowserWindow or calls
> on BrowserWindow (which are the various one click bubble implementations
> for the various platforms).  Its not used at all by
> OneClickSigninSyncStarter.  Note that I could also declare the type
> inside the class BrowserWindow so as to not pollute the namespace.
>
> If you don't think browser_window.h is the right place, then I think the
> best other alternative is to put it in its own header file, much like
> base\callback_forward.h, and then include this header where needed.
>
>
http://codereview.chromium.**org/10332185/<http://codereview.chromium.org/103...
>

Okay why don't we just put it in its own header file?  But note that
one_click_signin_sync_starter.h should use the enum also (instead of the
bool).

I'll ping you on chat in an hour or so -- have to meet with someone.

[akalin, 2012-05-24 18:42:04]

LGTM after nits

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:460:
start_sync_callback:start_sync_callback];
indent to line up :s

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:27:
BrowserWindow::StartSyncCallback start_sync_callback_;
may need this callback to behave like the other platform implementations (null
if already called, etc.).  Add a TODO for me to investigate

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (left):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:58: -
(IBAction)onClickLearnMoreLink:(id)sender {
need to keep this method since the button is still hooked up to it, but make it
not do anything

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:53:
start_sync_callback_.Run(true);
gotta change this bool

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:58:
start_sync_callback_.Run(false);
this, too

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:63: // the
callback with argument set to true.
update comment (true -> whatever the enum value is)

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:46:
MOCK_METHOD1(OnStartSync, void(bool));
bool -> enum

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:65:
EXPECT_CALL(*this, OnStartSync(true)).Times(1);
true -> enum value

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:77:
EXPECT_CALL(*this, OnStartSync(false)).Times(1);
here too

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:11: #include
"base/i18n/rtl.h"
sort (i18n goes before logging)

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:48:
BrowserWindow::StartSyncCallback start_sync_callback_;
Add a comment saying that this callback is nulled out when it's called or if the
user decides to undo, and it's called on close if it's not nulled out

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:89:
BrowserWindow::StartSyncCallback start_sync_callback_;
comment explaining that the callback is nulled out if the user decides to undo
or when it is called once

[Roger Tawa OOO till Jul 10th, 2012-05-24 19:08:33]

Thanks Fred, please take another look.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:460:
start_sync_callback:start_sync_callback];
On 2012/05/24 18:42:04, akalin wrote:
> indent to line up :s

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:27:
BrowserWindow::StartSyncCallback start_sync_callback_;
On 2012/05/24 18:42:04, akalin wrote:
> may need this callback to behave like the other platform implementations (null
> if already called, etc.).  Add a TODO for me to investigate

Done.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (left):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:58: -
(IBAction)onClickLearnMoreLink:(id)sender {
On 2012/05/24 18:42:04, akalin wrote:
> need to keep this method since the button is still hooked up to it, but make
it
> not do anything

Done.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:53:
start_sync_callback_.Run(true);
On 2012/05/24 18:42:04, akalin wrote:
> gotta change this bool

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:58:
start_sync_callback_.Run(false);
On 2012/05/24 18:42:04, akalin wrote:
> this, too

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:63: // the
callback with argument set to true.
On 2012/05/24 18:42:04, akalin wrote:
> update comment (true -> whatever the enum value is)

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:46:
MOCK_METHOD1(OnStartSync, void(bool));
On 2012/05/24 18:42:04, akalin wrote:
> bool -> enum

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:65:
EXPECT_CALL(*this, OnStartSync(true)).Times(1);
On 2012/05/24 18:42:04, akalin wrote:
> true -> enum value

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:77:
EXPECT_CALL(*this, OnStartSync(false)).Times(1);
On 2012/05/24 18:42:04, akalin wrote:
> here too

Done in lastest patchset.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:11: #include
"base/i18n/rtl.h"
On 2012/05/24 18:42:04, akalin wrote:
> sort (i18n goes before logging)

this is the correct sorting, since we should keep files in the same directory
together.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:48:
BrowserWindow::StartSyncCallback start_sync_callback_;
On 2012/05/24 18:42:04, akalin wrote:
> Add a comment saying that this callback is nulled out when it's called or if
the
> user decides to undo, and it's called on close if it's not nulled out

Done.

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/32001/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:89:
BrowserWindow::StartSyncCallback start_sync_callback_;
On 2012/05/24 18:42:04, akalin wrote:
> comment explaining that the callback is nulled out if the user decides to undo
> or when it is called once

Done.

[Roger Tawa OOO till Jul 10th, 2012-05-24 20:06:27]

Hi Elliot, Peter,

Can you please do owner reviews for code under browser\ui?

Elliot: please take a look at the code in browser\ui\gtk.

Peter: please take a look at the rest of the ui code.  My mac code has a lot of
TODOs in it for Fred since I don't have mac development experience, and Fred has
agreed to clean up the mac code once this change is committed.

Thanks.

[Elliot Glaysher, 2012-05-24 20:10:56]

gtk lgtm

http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:73: // size and look kind
of ugly.  Figure out how to make it nicer.
If you are talking about having them be the same horizontal size, you want a
GtkSizeGroup. A good place to look for an example is browser_toolbar_gtk.cc.

[akalin, 2012-05-24 20:37:53]

still lgtm

On 2012/05/24 20:10:56, Elliot Glaysher wrote:
> gtk lgtm
> 
>
http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
> File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):
> 
>
http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
> chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:73: // size and look kind
> of ugly.  Figure out how to make it nicer.
> If you are talking about having them be the same horizontal size, you want a
> GtkSizeGroup. A good place to look for an example is browser_toolbar_gtk.cc.

[Roger Tawa OOO till Jul 10th, 2012-05-24 20:50:26]

Thanks Elliot, that's exactly what I was looking for.  Please take another look
if you like.

http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/35001/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:73: // size and look kind
of ugly.  Figure out how to make it nicer.
On 2012/05/24 20:10:56, Elliot Glaysher wrote:
> If you are talking about having them be the same horizontal size, you want a
> GtkSizeGroup. A good place to look for an example is browser_toolbar_gtk.cc.

Done.

[Peter Kasting, 2012-05-24 22:25:17]

Mostly nits but there are a few real issues mixed in.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:22: //
TODO(akalin): learnMoreLink_ needs to be removed, but it can't until
Nit: can't -> can't be

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:59: //
TODO(akalin): this method should be removed once the UI elements is
Nit: is -> are

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:20: using
testing::_;
Nit: Just explicitly-qualify the one usage

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:67:
OnStartSync(
Nit: Weird indenting; how about:

  EXPECT_CALL(*this, OnStartSync(
      OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);

(2 places)

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:75: //
TODO(akalin): Test that the dialog doesn't calls the callback if the Undo
Nit: calls -> call

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1094: ignore_result(new
OneClickSigninBubbleGtk(this, start_sync_callback));
Nit: Do we need ignore_result() here?  new() isn't annotated WARN_UNUSED_RESULT
is it?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:10: #include
"base/logging.h"
Nit: Alphabetize

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:77: // size and look kind
of ugly.  Figure out how to make it nicer.
This comment seems out of date?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:35: void ClickOKForTest();
Nit: The implementations of these are so simple that it would be nice to either
use FRIEND_TEST_ALL_PREFIXES() instead and inline these directly in the tests,
or else implement them in a base class of all the browsertests and then friend
that class.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc:49:
OnStartSync(
Nit: Unusual indenting (see earlier comment) (2 places)

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc:56: // Test
that the dialog doesn't calls the callback if the Undo button is
Nit: calls -> call

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

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:142: ignore_result(
Nit: ignore_result() not needed here?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:144: profile, session_index,
email, password, start_mode));
Nit: Could put half of args on previous line and indent even

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:153: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
Nit: Just inline this into the next line

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:179: // the future.
What?  You don't want this.  This looks like bad copy+paste from the alternate
nav infobar.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:185: // WebContents.
This seems wrong too.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:37: void
MarkCurrentThreadAsUiThread();
Nit: Ui -> UI

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:39: // Creates a mock
web contents for tests.  If |use_incognito| is true then
Nit: Clarity: web contents -> WebContents (2 places)

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:43: // Enables or
disables one-click signin.
Nit: These next two comments add nothing to the function declaration.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:51: void
ConnectProfileToAccount(const char* username);
Nit: Please take a const std::string&

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:81:
EXPECT_TRUE(web_contents_.get() == NULL);
Nit: "!web_contents_" would also work, I think

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:107:
signin_manager_->StartSignIn(username, "", "", "");
Nit: "" -> std::string()

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:29: ?
one_click_signin::HISTOGRAM_WITH_DEFAULTS
Nit: Operators go before linebreaks (2 places)

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:33: const int
kButtonWidth = 64;
Hardcoding this makes me really nervous for both l10n and accessibility/theming
reasons (what if someone has large fonts?).

I'm confused why we need this anyway.  You can just call
set_ignore_minimum_size(false) on the button after you create it to allow
narrower buttons.  Then you can make both buttons the same size by calling
SetText() with both strings on each (just in opposite orders).

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:121:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
This doesn't seem like the right value.  This is for when you're putting a
control below a label that describes it.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:152: if
(!start_sync_callback_.is_null())
Nit: Need {}

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:32: // will be
placed visually beneath |anchor_view|.  The closures are called
What closures?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:34: // The bool
argument indicates if default sync settings should be used.
What argument?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:46: // The following
accessor message should only be used for testing.
Nit: As before, I'd feel better about just making the relevant tests, or a test
base class, a friend.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc:32:
EXPECT_TRUE(view != NULL);
Tiny nit: EXPECT_FALSE(view == NULL) always felt like less of a double-negative
to me...

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constant...
chrome/common/url_constants.h:272: // "Learn more" URL for the one click signin
infobar.
Wow, the naming and ordering in this file has absolutely no consistency :(

[Roger Tawa OOO till Jul 10th, 2012-05-25 16:04:08]

Thanks Peter, please take another look.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:22: //
TODO(akalin): learnMoreLink_ needs to be removed, but it can't until
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: can't -> can't be

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm:59: //
TODO(akalin): this method should be removed once the UI elements is
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: is -> are

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:20: using
testing::_;
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Just explicitly-qualify the one usage

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:67:
OnStartSync(
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Weird indenting; how about:
> 
>   EXPECT_CALL(*this, OnStartSync(
>       OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);
> 
> (2 places)

That leaves the second line beyond 80 chars.  The logic I used was the
following:

- arguments to function are lined up, so OnStartSync is right below *this

- lines that are longer than 80 are indented by 4, so OneClickSigninSyncStarter
is indented by 4 from OnStartSync and Times(1) is indented by 4 from EXPECT_CALL

If you have another suggestion though please let me know.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:75: //
TODO(akalin): Test that the dialog doesn't calls the callback if the Undo
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: calls -> call

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1094: ignore_result(new
OneClickSigninBubbleGtk(this, start_sync_callback));
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Do we need ignore_result() here?  new() isn't annotated
WARN_UNUSED_RESULT
> is it?

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:10: #include
"base/logging.h"
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Alphabetize

I thought that the order of #includes is also based on files in the same
directory not being separated by files in subdirectories, which would be the
case if I do as you suggest.  But OK, done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:77: // size and look kind
of ugly.  Figure out how to make it nicer.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> This comment seems out of date?

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:35: void ClickOKForTest();
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: The implementations of these are so simple that it would be nice to
either
> use FRIEND_TEST_ALL_PREFIXES() instead and inline these directly in the tests,
> or else implement them in a base class of all the browsertests and then friend
> that class.

I like it better as is if you don't have huge objections.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc:49:
OnStartSync(
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Unusual indenting (see earlier comment) (2 places)

Same as before.  Please suggest an alternative indentation if you like.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk_browsertest.cc:56: // Test
that the dialog doesn't calls the callback if the Undo button is
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: calls -> call

Done.

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

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:142: ignore_result(
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: ignore_result() not needed here?

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:144: profile, session_index,
email, password, start_mode));
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Could put half of args on previous line and indent even

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:153: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Just inline this into the next line

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:179: // the future.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> What?  You don't want this.  This looks like bad copy+paste from the alternate
> nav infobar.

Done.  Yes my bad, a pasted and then forgot to back and fix it.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:185: // WebContents.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> This seems wrong too.

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:37: void
MarkCurrentThreadAsUiThread();
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Ui -> UI

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:39: // Creates a mock
web contents for tests.  If |use_incognito| is true then
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Clarity: web contents -> WebContents (2 places)

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:43: // Enables or
disables one-click signin.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: These next two comments add nothing to the function declaration.

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:51: void
ConnectProfileToAccount(const char* username);
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Please take a const std::string&

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:81:
EXPECT_TRUE(web_contents_.get() == NULL);
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: "!web_contents_" would also work, I think

That generates error:

8>.\browser\ui\sync\one_click_signin_helper_unittest.cc(79) : error C2675: unary
'!' : 'scoped_ptr<C>' does not define this operator or a conversion to a type
acceptable to the predefined operator

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:107:
signin_manager_->StartSignIn(username, "", "", "");
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: "" -> std::string()

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:29: ?
one_click_signin::HISTOGRAM_WITH_DEFAULTS
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Operators go before linebreaks (2 places)

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:33: const int
kButtonWidth = 64;
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Hardcoding this makes me really nervous for both l10n and
accessibility/theming
> reasons (what if someone has large fonts?).
> 
> I'm confused why we need this anyway.  You can just call
> set_ignore_minimum_size(false) on the button after you create it to allow
> narrower buttons.  Then you can make both buttons the same size by calling
> SetText() with both strings on each (just in opposite orders).

set_ignore_minimum_size() seems to be ignored, but in the interest of better
support for l10n and such I think this is better.  I also used spacing constants
instead of hard coding values.  You can see the before and after screenshots of
the bubble here:

www/~rogerta/no_crawl/signin/bubble-before.gif
www/~rogerta/no_crawl/signin/bubble-after.gif

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:121:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
On 2012/05/24 22:25:18, Peter Kasting wrote:
> This doesn't seem like the right value.  This is for when you're putting a
> control below a label that describes it.

Seems like the right one to me.  Which value do you think is better?

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:152: if
(!start_sync_callback_.is_null())
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: Need {}

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:32: // will be
placed visually beneath |anchor_view|.  The closures are called
On 2012/05/24 22:25:18, Peter Kasting wrote:
> What closures?

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:34: // The bool
argument indicates if default sync settings should be used.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> What argument?

This comment was stale because of change to the callback signature.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:46: // The following
accessor message should only be used for testing.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Nit: As before, I'd feel better about just making the relevant tests, or a
test
> base class, a friend.

I prefer this style.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc:32:
EXPECT_TRUE(view != NULL);
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Tiny nit: EXPECT_FALSE(view == NULL) always felt like less of a
double-negative
> to me...

6 of one, half a dozen of another...

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constant...
chrome/common/url_constants.h:272: // "Learn more" URL for the one click signin
infobar.
On 2012/05/24 22:25:18, Peter Kasting wrote:
> Wow, the naming and ordering in this file has absolutely no consistency :(

Yes, I tried to put it where it made most sense to me, but if you think there is
a better place in the file let me know.  Is it worth opening a bug to clean this
up?

[Peter Kasting, 2012-05-25 16:58:05]

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:67:
OnStartSync(
On 2012/05/25 16:04:08, Roger Tawa wrote:
> On 2012/05/24 22:25:18, Peter Kasting wrote:
> > Nit: Weird indenting; how about:
> > 
> >   EXPECT_CALL(*this, OnStartSync(
> >       OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);
> > 
> > (2 places)
> 
> That leaves the second line beyond 80 chars.

No it doesn't:

12345678901234567890123456789012345678901234567890123456789012345678901234567890
  EXPECT_CALL(*this, OnStartSync(
      OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);

71 chars.

> - arguments to function are lined up, so OnStartSync is right below *this

Arguments to functions do not need to be lined up at the callsite.

> - lines that are longer than 80 are indented by 4, so
OneClickSigninSyncStarter
> is indented by 4 from OnStartSync and Times(1) is indented by 4 from
EXPECT_CALL

We don't generally reduce indenting in the middle of a statement except when
ending one function call argument and moving to the next.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.cc:10: #include
"base/logging.h"
On 2012/05/25 16:04:08, Roger Tawa wrote:
> On 2012/05/24 22:25:18, Peter Kasting wrote:
> > Nit: Alphabetize
> 
> I thought that the order of #includes is also based on files in the same
> directory not being separated by files in subdirectories,

Nope, unix sort.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:35: void ClickOKForTest();
On 2012/05/25 16:04:08, Roger Tawa wrote:
> On 2012/05/24 22:25:18, Peter Kasting wrote:
> > Nit: The implementations of these are so simple that it would be nice to
> either
> > use FRIEND_TEST_ALL_PREFIXES() instead and inline these directly in the
tests,
> > or else implement them in a base class of all the browsertests and then
friend
> > that class.
> 
> I like it better as is if you don't have huge objections.

My problem with XXXForTest() is that it still leaks encapsulation, it just asks
nicely that people not misuse it.  The compiler can't catch this.

I think these can be useful when they're big, complex implementations that would
be unwieldy to move elsewhere, but in all other cases, it's much better to use
the FRIEND_TEST mechanism (or a friend test base class) as this allows the
compiler to still ensure no one else uses the object's internals.  This also
discourages people from later renaming XXXForTest() to just XXX() when they
decide they want to use it more broadly -- something we've had happen in the
past.

This also leaves the least code in the class itself, which optimizes for
class-level readability and compile size -- though the differences in this case
are tiny.

Finally, this is more consistent with typical Chrome patterns -- XXXForTest() is
used orders of magnitude less frequently.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:121:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
On 2012/05/25 16:04:08, Roger Tawa wrote:
> On 2012/05/24 22:25:18, Peter Kasting wrote:
> > This doesn't seem like the right value.  This is for when you're putting a
> > control below a label that describes it.
> 
> Seems like the right one to me.

This is for a case like:

  BLAH_FOO option:
    [ ] Yes
    [ ] No

It's the spacing between the descriptive header and the objects it describes.

> Which value do you think is better?

Based on other bubbles, the existing kRelatedControlSmallVerticalSpacing was
correct.

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/common/url_constant...
chrome/common/url_constants.h:272: // "Learn more" URL for the one click signin
infobar.
On 2012/05/25 16:04:08, Roger Tawa wrote:
> On 2012/05/24 22:25:18, Peter Kasting wrote:
> > Wow, the naming and ordering in this file has absolutely no consistency :(
> 
> Yes, I tried to put it where it made most sense to me, but if you think there
is
> a better place in the file let me know.  Is it worth opening a bug to clean
this
> up?

Probably.

http://codereview.chromium.org/10332185/diff/32006/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/32006/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:131:
ok_button_->set_ignore_minimum_size(false);
I think these are being "ignored" because you're calling them with (false)
instead of the (true) you want.

[Roger Tawa OOO till Jul 10th, 2012-05-25 18:49:51]

Hi Peter, please take another look.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/cocoa/on...
chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm:67:
OnStartSync(
On 2012/05/25 16:58:06, Peter Kasting wrote:
> On 2012/05/25 16:04:08, Roger Tawa wrote:
> > On 2012/05/24 22:25:18, Peter Kasting wrote:
> > > Nit: Weird indenting; how about:
> > > 
> > >   EXPECT_CALL(*this, OnStartSync(
> > >       OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);
> > > 
> > > (2 places)
> > 
> > That leaves the second line beyond 80 chars.
> 
> No it doesn't:
> 
>
12345678901234567890123456789012345678901234567890123456789012345678901234567890
>   EXPECT_CALL(*this, OnStartSync(
>       OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS)).Times(1);
> 
> 71 chars.
> 
> > - arguments to function are lined up, so OnStartSync is right below *this
> 
> Arguments to functions do not need to be lined up at the callsite.
> 
> > - lines that are longer than 80 are indented by 4, so
> OneClickSigninSyncStarter
> > is indented by 4 from OnStartSync and Times(1) is indented by 4 from
> EXPECT_CALL
> 
> We don't generally reduce indenting in the middle of a statement except when
> ending one function call argument and moving to the next.

Done.

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/39004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:121:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
On 2012/05/25 16:58:06, Peter Kasting wrote:
> On 2012/05/25 16:04:08, Roger Tawa wrote:
> > On 2012/05/24 22:25:18, Peter Kasting wrote:
> > > This doesn't seem like the right value.  This is for when you're putting a
> > > control below a label that describes it.
> > 
> > Seems like the right one to me.
> 
> This is for a case like:
> 
>   BLAH_FOO option:
>     [ ] Yes
>     [ ] No
> 
> It's the spacing between the descriptive header and the objects it describes.
> 
> > Which value do you think is better?
> 
> Based on other bubbles, the existing kRelatedControlSmallVerticalSpacing was
> correct.

Done.

http://codereview.chromium.org/10332185/diff/32006/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

http://codereview.chromium.org/10332185/diff/32006/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:131:
ok_button_->set_ignore_minimum_size(false);
On 2012/05/25 16:58:06, Peter Kasting wrote:
> I think these are being "ignored" because you're calling them with (false)
> instead of the (true) you want.

duh.  I misinterpreted the meaning.

[Peter Kasting, 2012-05-25 21:09:32]

LGTM

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:50:
FRIEND_TEST_ALL_PREFIXES(OneClickSigninBubbleGtkTest, ShowAndOK);
Nit: These go atop private section

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

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:176: false);
Nit: Since you're combining args on multiple lines anyway, go ahead and combine
this with the previous line; also OK to put as many args as will fit on the same
line as the called function name

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:106:
signin_manager_->StartSignIn(username, std::string(), std::string(),
Nit: Indent 2

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:45: // The following
accessor message should only be used for testing.
Did you decide these should still stay as for_testing() accessors?

[Roger Tawa OOO till Jul 10th, 2012-05-26 01:56:01]

Thanks Peter.  Comment addressed, changes uploaded.

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/gtk/one_...
File chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/gtk/one_...
chrome/browser/ui/gtk/one_click_signin_bubble_gtk.h:50:
FRIEND_TEST_ALL_PREFIXES(OneClickSigninBubbleGtkTest, ShowAndOK);
On 2012/05/25 21:09:33, Peter Kasting wrote:
> Nit: These go atop private section

Done.

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

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:176: false);
On 2012/05/25 21:09:33, Peter Kasting wrote:
> Nit: Since you're combining args on multiple lines anyway, go ahead and
combine
> this with the previous line; also OK to put as many args as will fit on the
same
> line as the called function name

Done.

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:106:
signin_manager_->StartSignIn(username, std::string(), std::string(),
On 2012/05/25 21:09:33, Peter Kasting wrote:
> Nit: Indent 2

Done.

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/36038/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:45: // The following
accessor message should only be used for testing.
On 2012/05/25 21:09:33, Peter Kasting wrote:
> Did you decide these should still stay as for_testing() accessors?

Done.

[tfarina, 2012-05-26 14:54:50]

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:53: // Creates a
BookmarkBubbleView.
nit: Creates a OneClickSigninBubbleView.

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:83: // Controls at
buttom of bubble.
nit: buttom -> bottom

[tfarina, 2012-05-28 16:06:10]

http://codereview.chromium.org/10332185/diff/44001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/44001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:28: public:
nit: indent one space.

[Roger Tawa OOO till Jul 10th, 2012-05-28 17:10:00]

Thanks Tiago.

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:53: // Creates a
BookmarkBubbleView.
On 2012/05/26 14:54:50, tfarina wrote:
> nit: Creates a OneClickSigninBubbleView.

Done.

http://codereview.chromium.org/10332185/diff/43004/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:83: // Controls at
buttom of bubble.
On 2012/05/26 14:54:50, tfarina wrote:
> nit: buttom -> bottom

Done.

http://codereview.chromium.org/10332185/diff/44001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

http://codereview.chromium.org/10332185/diff/44001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:28: public:
On 2012/05/28 16:06:10, tfarina wrote:
> nit: indent one space.

Done.

[commit-bot: I haz the power, 2012-05-29 17:37:49]

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

[commit-bot: I haz the power, 2012-05-29 20:40:14]

Change committed as 139365