Implement the inline signin confirmation bubble and mirror upgrade tutorial

Implement the inline signin confirmation bubble and mirror upgrade tutorial

Mock at https://docs.google.com/a/google.com/presentation/d/1GUagduYmawbUHiWwhQdtyxCUFugv2BPPCH0eHhIWGO8/edit#slide=id.g354ad708b_00

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

[guohui, 2014-08-05 14:50:10]

Hey,

could you please review the CL?

rogerta@ for everything,
sky@ for owner review of chrome\browser\ui\views\profiles\.. and  
chrome\browser\ui\profile_chooser_constants.h

Thanks,

Hui

[Roger Tawa OOO till Jul 10th, 2014-08-05 16:02:13]

lgtm with some questions below.

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:391: }
Are we guaranteed to get this callback?

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:571: }
Add comment explaining why we ignore these modes?

When is this called?

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:767:
SyncConfirmationUIClosed(true /* configure_sync_first */);
Do you need to reset the tutorial mode to none here?

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:133:
signin_error_controller();
indent four more.

[sky, 2014-08-05 17:45:35]

LGTM

[guohui, 2014-08-05 19:27:42]

Addressed Roger's comments and also added the missing implementation for the
"not claire' view (slide 12 in the attached mock above).

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:391: }
i think yes as long as Chrome shuts down properly. The callback is called in
three cases:

1. user clicks 'ok i got it'
2. user clicks 'settings' link
3. the avatar bubble is closed 

i think the case 3 is guaranteed to happen unless something really bad happens,
thus the callback will be eventually called.

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:571: }
On 2014/08/05 16:02:13, Roger Tawa wrote:
> Add comment explaining why we ignore these modes?
> 
> When is this called?

Done.

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:767:
SyncConfirmationUIClosed(true /* configure_sync_first */);
On 2014/08/05 16:02:13, Roger Tawa wrote:
> Do you need to reset the tutorial mode to none here?

Done.

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:133:
signin_error_controller();
On 2014/08/05 16:02:13, Roger Tawa wrote:
> indent four more.

Done.

[guohui, 2014-08-06 13:22:59]

Added mac implementation.

@alex, could you please review the mac implementation in
chrome/browser/ui/cocoa/profiles/...?

Thanks!

[guohui, 2014-08-06 13:42:52]

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:391: }
eh actually i missed one case =(

The callback will never be called if the bubble is closed before SyncStarter
starts. The bubble is auto closed if it loses focus, and this may happen
accidentally (e.g. user clicks somewhere other than the bubble too quickly), or
if Chrome decides to show a modal confirmation dialog (enterprise and
cross-account) before completing signin.

A possible solution is to ensure the avatar bubble opens the confirmation view
at line 449 below. This is related to the error/warning flows, so i ll handle it
in the next CL.

[Roger Tawa OOO till Jul 10th, 2014-08-06 14:09:41]

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/435423005/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:391: }
On 2014/08/06 13:42:52, guohui wrote:
> eh actually i missed one case =(
> 
> The callback will never be called if the bubble is closed before SyncStarter
> starts. The bubble is auto closed if it loses focus, and this may happen
> accidentally (e.g. user clicks somewhere other than the bubble too quickly),
or
> if Chrome decides to show a modal confirmation dialog (enterprise and
> cross-account) before completing signin.
> 
> A possible solution is to ensure the avatar bubble opens the confirmation view
> at line 449 below. This is related to the error/warning flows, so i ll handle
it
> in the next CL.
> 
> 

You have another bug to make the bubble not auto-close during sign in.  Would
that help as well?  If so, no need to hack some more here.

[guohui, 2014-08-06 15:12:42]

wee have a few options here:

1. not auto-close during signin. However i am not sure if it is good UI to show
both the avatar bubble and a modal confirmation dialog at the same time.
2. when the modal dialog is closed, re-show the avatar bubble with confirmation
for sync settings.
3. when the modal dialog is closed, just start sync immediately.
4. when the modal dialog is closed, show sync settings page.
5. show a settings link on the modal dialogs, and do not re-show the avatar
bubble since the user already confirmed sync settings on the modal dialog. 

option 3 is the simplest to implement, but i am not sure if we can start sync
without giving user option to configure the sync settings. I ll implement it in
this CL to avoid dangling callback.

Personally i prefer option 5, and eventually we should move all modal warnings
(AFAIK enterprise and cross account) to the avatar bubble.

[Alexei Svitkine (slow), 2014-08-06 15:37:25]

cocoa LGTM % nits

Please associate this CL with a crbug, since this is quite large.

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1786:
IDS_PROFILES_DISCONNECT_BUTTON)
Nit: Indent 4 more.

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1802:
IDS_PROFILES_ADD_PERSON_BUTTON)
Nit: Indent 4 more.

[guohui, 2014-08-06 17:23:39]

The CQ bit was checked by guohui@chromium.org

[guohui, 2014-08-06 17:23:47]

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1786:
IDS_PROFILES_DISCONNECT_BUTTON)
On 2014/08/06 15:37:24, Alexei Svitkine wrote:
> Nit: Indent 4 more.

Done.

https://codereview.chromium.org/435423005/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1802:
IDS_PROFILES_ADD_PERSON_BUTTON)
On 2014/08/06 15:37:24, Alexei Svitkine wrote:
> Nit: Indent 4 more.

Done.

[commit-bot: I haz the power, 2014-08-06 17:25:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/guohui@chromium.org/435423005/100001

[commit-bot: I haz the power, 2014-08-06 20:27:27]

Change committed as 287843