Change the one-click sign in confirmation bubble into a modal dialog

Change the one-click sign in confirmation bubble into a modal dialog

BUG=171330
TEST=One-click sign in process should end with this modal dialog instead of
non-modal bubble.  See image attached to bug.


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

[Roger Tawa OOO till Jul 10th, 2013-01-22 19:18:35]

Hi Tim,

Can please take a look?  Thanks.

[tfarina, 2013-01-22 19:22:48]

https://codereview.chromium.org/11896021/diff/4001/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/11896021/diff/4001/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:36:
kColumnSetFillAlign,
why do you need this here, in the public section? This is pretty unusual.

[Roger Tawa OOO till Jul 10th, 2013-01-22 21:32:35]

Thanks Thiago.

https://codereview.chromium.org/11896021/diff/4001/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/11896021/diff/4001/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:36:
kColumnSetFillAlign,
On 2013/01/22 19:22:48, tfarina wrote:
> why do you need this here, in the public section? This is pretty unusual. 

Made it protected.  I want to expose it to derived class so that it does not
define classes with same id.

[tfarina, 2013-01-22 21:37:22]

Do you really want/need to make it part of the BubbleView class?

Moving to unnamed namespace in the source file doesn't work?

https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:49: // The column
set constants thaat can be used in the InitContent() function
s/thaat/that

[tim (not reviewing), 2013-01-23 18:36:16]

On 2013/01/22 21:37:22, tfarina wrote:
> Do you really want/need to make it part of the BubbleView class?
> 
> Moving to unnamed namespace in the source file doesn't work?
> 
>
https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
> File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):
> 
>
https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
> chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:49: // The column
> set constants thaat can be used in the InitContent() function
> s/thaat/that

Aside from tfarina's comments, this LGTM

[Roger Tawa OOO till Jul 10th, 2013-01-23 22:24:51]

Thanks Tim, Thiago.  Moved enums to cc file as suggested.

https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/11896021/diff/1015/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:49: // The column
set constants thaat can be used in the InitContent() function
On 2013/01/22 21:37:22, tfarina wrote:
> s/thaat/that

Done.

[Roger Tawa OOO till Jul 10th, 2013-01-24 01:21:10]

Hi Alexei, Scott, Elliot,

Can you please do owner reviews?  Note that the cocoa and gtk changes are only
to change a function signature.  The new implementation for those platforms is
coming follow up CLs.

Alexei: can you look at the cocoa changes?
Elliot: can you look at the gtk changes?
Scott: can you look at the rest?

Thanks.

[Alexei Svitkine (slow), 2013-01-24 03:30:01]

cocoa/ LGTM

[Elliot Glaysher, 2013-01-24 17:21:56]

On 2013/01/24 03:30:01, Alexei Svitkine wrote:
> cocoa/ LGTM

gtk lgtm

[sky, 2013-01-24 17:27:59]

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:38: enum {
Name your enum.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:39:
kColumnSetFillAlign,
chrome uses ALL_CAPS for enums.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:47: 
remove newline.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:58: virtual void
InitContent(views::GridLayout* layout) OVERRIDE;
I'm surprised this works. Shouldn't virtual methods you expect to override be in
the protected section in the parent class?

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:186: }
  // namespace

[Roger Tawa OOO till Jul 10th, 2013-01-24 23:30:10]

Thanks Alexei, Elliot, Scott.

It was decided that the image should not be part of the dialog.  The text of the
dialog should remain the same, however the "Advanced..." link should drop the
trailing "...".  I have made these changes as well as addressing Scott's
comments.  This only change the views implementation, no changes to cocoa or
gtk.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:38: enum {
On 2013/01/24 17:28:00, sky wrote:
> Name your enum.

Done.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:39:
kColumnSetFillAlign,
On 2013/01/24 17:28:00, sky wrote:
> chrome uses ALL_CAPS for enums.

Done.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:47: 
On 2013/01/24 17:28:00, sky wrote:
> remove newline.

Done.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:58: virtual void
InitContent(views::GridLayout* layout) OVERRIDE;
On 2013/01/24 17:28:00, sky wrote:
> I'm surprised this works. Shouldn't virtual methods you expect to override be
in
> the protected section in the parent class?

No.  This is common to allow behaviour to be overridden by derived classes while
not increasing the visible api of the class.

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:186: }
On 2013/01/24 17:28:00, sky wrote:
>   // namespace

Done.

[sky, 2013-01-25 00:51:31]

LGTM

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/11896021/diff/6005/chrome/browser/ui/views/sy...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:58: virtual void
InitContent(views::GridLayout* layout) OVERRIDE;
On 2013/01/24 23:30:10, Roger Tawa wrote:
> On 2013/01/24 17:28:00, sky wrote:
> > I'm surprised this works. Shouldn't virtual methods you expect to override
be
> in
> > the protected section in the parent class?
> 
> No.  This is common to allow behaviour to be overridden by derived classes
while
> not increasing the visible api of the class.

I've seen this in subclasses, but never in the parent.

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

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

[commit-bot: I haz the power, 2013-01-25 03:08:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-25 14:42:16]

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

[commit-bot: I haz the power, 2013-01-25 17:21:31]

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

[commit-bot: I haz the power, 2013-01-25 19:18:50]

Change committed as 178868