Move post-signin confirmation bubble to OneClickSigninSyncStarter.

Move post-signin confirmation bubble to OneClickSigninSyncStarter.

OneClickSigninSyncStarter now displays the post-signin confirmation bubble so
we can better integrate this with the rest of the signin UI (creating a new
profile, displaying the SAML dialog, etc).

BUG=174655, 232339, 236828, 238173
R=rogerta@chromium.org

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

[Andrew T Wilson (Slow), 2013-05-03 13:41:07]

PTAL - I'd like to land this today (before M28 branch point) since it has a
string change.

[Roger Tawa OOO till Jul 10th, 2013-05-03 15:16:47]

Look good Drew, a few comments below.  Please let me patch in your CL and play
with it for a bit.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:93: bool
saml_confirmation_required,
Is this really saml related?  I think my suggestion below it is no longer saml
specific.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1154:
saml_confirmation_required_, source_)));
Hmm, this is actually broken.  Not your CL Drew, but I think of the recent
refactorings.  In this case, we will show two dialogs!  The one here, and since
|saml_confirmation_required_| will be true, the saml dialog is shown.

In discussions with the security team, it was decided that the non-saml
untrusted path should show the same dialog as the saml untrusted path.  That is,
the one with the email address.  So I think the right fix would be to replace
lines 1146-1154 with something like:

      StartSync(StartSyncArgs(profile, browser, auto_accept_,
                              session_index_, email_, password_,
                              false, true, source_),
                OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1201: start_mode));
Can we move this confirmation dialog into the starter as well?

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:286: // No error
notification neeeded if the user manually cancelled signin.
tooo many e's :-)

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:48: CONFIRM_SAML_SIGNIN,
The comment is correct, but saml is not necessarily the only reason.  I wonder
if the enum name should be changed.

[Andrew T Wilson (Slow), 2013-05-03 19:16:11]

PTAL

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:93: bool
saml_confirmation_required,
On 2013/05/03 15:16:47, Roger Tawa wrote:
> Is this really saml related?  I think my suggestion below it is no longer saml
> specific.
I only set this if confirmation_required_ is set, which is what is_trusted used
to be called. So I presume this is SAML - I guess I don't understand what the
"untrusted" path would be if not SAML - can you clarify? I've renamed this to
"untrusted", but I'd still like to understand that case.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1154:
saml_confirmation_required_, source_)));
On 2013/05/03 15:16:47, Roger Tawa wrote:
> Hmm, this is actually broken.  Not your CL Drew, but I think of the recent
> refactorings.  In this case, we will show two dialogs!  The one here, and
since
> |saml_confirmation_required_| will be true, the saml dialog is shown.
> 
> In discussions with the security team, it was decided that the non-saml
> untrusted path should show the same dialog as the saml untrusted path.  That
is,
> the one with the email address.  So I think the right fix would be to replace
> lines 1146-1154 with something like:
> 
>       StartSync(StartSyncArgs(profile, browser, auto_accept_,
>                               session_index_, email_, password_,
>                               false, true, source_),
>                 OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);

Do you mean that I should pass |false| as the confirmation_required flag instead
of |true| (I've made that change assuming that's what you meant).

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1201: start_mode));
On 2013/05/03 15:16:47, Roger Tawa wrote:
> Can we move this confirmation dialog into the starter as well?

Maybe as a separate CL? I wasn't really doing this CL just for cleanliness but
rather to fix specific bugs.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:286: // No error
notification neeeded if the user manually cancelled signin.
On 2013/05/03 15:16:47, Roger Tawa wrote:
> tooo many e's :-)

Done.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:48: CONFIRM_SAML_SIGNIN,
On 2013/05/03 15:16:47, Roger Tawa wrote:
> The comment is correct, but saml is not necessarily the only reason.  I wonder
> if the enum name should be changed.
OK, will do.

[Roger Tawa OOO till Jul 10th, 2013-05-06 15:46:39]

Thanks Drew.  A couple of comments below.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:93: bool
saml_confirmation_required,
On 2013/05/03 19:16:11, Andrew T Wilson wrote:
> On 2013/05/03 15:16:47, Roger Tawa wrote:
> > Is this really saml related?  I think my suggestion below it is no longer
saml
> > specific.
> I only set this if confirmation_required_ is set, which is what is_trusted
used
> to be called. So I presume this is SAML - I guess I don't understand what the
> "untrusted" path would be if not SAML - can you clarify? I've renamed this to
> "untrusted", but I'd still like to understand that case.

Thanks Drew.  The non-saml "untrusted" flow is the interstitial flow when
signing in to some other google service like gmail.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1154:
saml_confirmation_required_, source_)));
On 2013/05/03 19:16:11, Andrew T Wilson wrote:
> On 2013/05/03 15:16:47, Roger Tawa wrote:
> > Hmm, this is actually broken.  Not your CL Drew, but I think of the recent
> > refactorings.  In this case, we will show two dialogs!  The one here, and
> since
> > |saml_confirmation_required_| will be true, the saml dialog is shown.
> > 
> > In discussions with the security team, it was decided that the non-saml
> > untrusted path should show the same dialog as the saml untrusted path.  That
> is,
> > the one with the email address.  So I think the right fix would be to
replace
> > lines 1146-1154 with something like:
> > 
> >       StartSync(StartSyncArgs(profile, browser, auto_accept_,
> >                               session_index_, email_, password_,
> >                               false, true, source_),
> >                 OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
> 
> Do you mean that I should pass |false| as the confirmation_required flag
instead
> of |true| (I've made that change assuming that's what you meant).

I was actually suggesting passing |true| instead of |false|, and not calling
ShowOneClickSigninBubble() at all here.  Two reasons:

1/ We consolidate confirmations in the starter
2/ The security folks said we should display the email address in the dialog
even for the non-saml "untrusted" case

This allows us to get rid of ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG and only
have one dialog ONE_CLICK_SIGNIN_BUBBLE_TYPE_SAML_MODAL_DIALOG.  (I can remove
ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG in a follow up CL).

Does that sounds reasonable?

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1201: start_mode));
On 2013/05/03 19:16:11, Andrew T Wilson wrote:
> On 2013/05/03 15:16:47, Roger Tawa wrote:
> > Can we move this confirmation dialog into the starter as well?
> 
> Maybe as a separate CL? I wasn't really doing this CL just for cleanliness but
> rather to fix specific bugs.

Sure.

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1164:
untrusted_confirmation_required_, source_),
I think |untrusted_confirmation_required| should be set to false here, much like
in the case above.  Previous, we did not display the confirmation because the
user will see the advanced config dialog, and has a chance to abort the connect
before it actually happens.

[Andrew T Wilson (Slow), 2013-05-06 17:00:12]

PTAL - still needs some testing before I land it, but is this what you were
looking for?

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1154:
saml_confirmation_required_, source_)));
On 2013/05/06 15:46:39, Roger Tawa wrote:
> On 2013/05/03 19:16:11, Andrew T Wilson wrote:
> > On 2013/05/03 15:16:47, Roger Tawa wrote:
> > > Hmm, this is actually broken.  Not your CL Drew, but I think of the recent
> > > refactorings.  In this case, we will show two dialogs!  The one here, and
> > since
> > > |saml_confirmation_required_| will be true, the saml dialog is shown.
> > > 
> > > In discussions with the security team, it was decided that the non-saml
> > > untrusted path should show the same dialog as the saml untrusted path. 
That
> > is,
> > > the one with the email address.  So I think the right fix would be to
> replace
> > > lines 1146-1154 with something like:
> > > 
> > >       StartSync(StartSyncArgs(profile, browser, auto_accept_,
> > >                               session_index_, email_, password_,
> > >                               false, true, source_),
> > >                 OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
> > 
> > Do you mean that I should pass |false| as the confirmation_required flag
> instead
> > of |true| (I've made that change assuming that's what you meant).
> 
> I was actually suggesting passing |true| instead of |false|, and not calling
> ShowOneClickSigninBubble() at all here.  Two reasons:
> 
> 1/ We consolidate confirmations in the starter
> 2/ The security folks said we should display the email address in the dialog
> even for the non-saml "untrusted" case
> 
> This allows us to get rid of ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG and
only
> have one dialog ONE_CLICK_SIGNIN_BUBBLE_TYPE_SAML_MODAL_DIALOG.  (I can remove
> ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG in a follow up CL).
> 
> Does that sounds reasonable?

Done.

https://codereview.chromium.org/14914003/diff/10/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1201: start_mode));
On 2013/05/06 15:46:39, Roger Tawa wrote:
> On 2013/05/03 19:16:11, Andrew T Wilson wrote:
> > On 2013/05/03 15:16:47, Roger Tawa wrote:
> > > Can we move this confirmation dialog into the starter as well?
> > 
> > Maybe as a separate CL? I wasn't really doing this CL just for cleanliness
but
> > rather to fix specific bugs.
> 
> Sure.

Done.

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1164:
untrusted_confirmation_required_, source_),
On 2013/05/06 15:46:39, Roger Tawa wrote:
> I think |untrusted_confirmation_required| should be set to false here, much
like
> in the case above.  Previous, we did not display the confirmation because the
> user will see the advanced config dialog, and has a chance to abort the
connect
> before it actually happens.
I'm slightly nervous about this since the user isn't actually shown the email
address he's signed in with if he just goes to the advanced config dialog -
he'll be tempted to just click OK to dismiss the dialog, in which case the
signin will complete. In the untrusted/SAML case, I'd rather prompt him.

In any case, if this isn't OK/is too cumbersome, let's drive a separate CL.

[Roger Tawa OOO till Jul 10th, 2013-05-06 20:14:01]

Yup that's what I had in mind Drew :-)

I'll ask Travis about the confirmation dialog.

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/14914003/diff/33001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1164:
untrusted_confirmation_required_, source_),
On 2013/05/06 17:00:13, Andrew T Wilson wrote:
> On 2013/05/06 15:46:39, Roger Tawa wrote:
> > I think |untrusted_confirmation_required| should be set to false here, much
> like
> > in the case above.  Previous, we did not display the confirmation because
the
> > user will see the advanced config dialog, and has a chance to abort the
> connect
> > before it actually happens.
> I'm slightly nervous about this since the user isn't actually shown the email
> address he's signed in with if he just goes to the advanced config dialog -
> he'll be tempted to just click OK to dismiss the dialog, in which case the
> signin will complete. In the untrusted/SAML case, I'd rather prompt him.
> 
> In any case, if this isn't OK/is too cumbersome, let's drive a separate CL.

OK.

[Roger Tawa OOO till Jul 10th, 2013-05-06 20:41:11]

lgtm

Drew: The issue with the confirmation dialog is actually there already in ToT,
so I am lgtm'ing this change to not delay you any further.

[commit-bot: I haz the power, 2013-05-07 15:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/14914003/84001

[commit-bot: I haz the power, 2013-05-07 15:43:26]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Andrew T Wilson (Slow), 2013-05-08 09:30:22]

Committed patchset #8 manually as r198866 (presubmit successful).