Move post-signin confirmation bubble to OneClickSigninSyncStarter.

chrome/browser/ui/sync/one_click_signin_helper.cc

Index: chrome/browser/ui/sync/one_click_signin_helper.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_helper.cc b/chrome/browser/ui/sync/one_click_signin_helper.cc
index 3891458ac6285e26cb4c26b6caf5f4ff9194c315..d53d54b098eba161ed78b648432493467261896d 100644
--- a/chrome/browser/ui/sync/one_click_signin_helper.cc
+++ b/chrome/browser/ui/sync/one_click_signin_helper.cc
@@ -82,14 +82,16 @@ namespace {
 // Arguments used with StartSync function.  base::Bind() cannot support too
 // many args for performance reasons, so they are packaged up into a struct.
 struct StartSyncArgs {
-  StartSyncArgs(Profile* profile,
-                Browser* browser,
-                OneClickSigninHelper::AutoAccept auto_accept,
-                const std::string& session_index,
-                const std::string& email,
-                const std::string& password,
-                bool force_same_tab_navigation,
-                bool confirmation_required);
+  StartSyncArgs(
+      Profile* profile,
+      Browser* browser,
+      OneClickSigninHelper::AutoAccept auto_accept,
+      const std::string& session_index,
+      const std::string& email,
+      const std::string& password,
+      bool force_same_tab_navigation,
+      bool saml_confirmation_required,

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

Is this really saml related?  I think my suggestion below it is no longer saml
specific.

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

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.

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

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

+      SyncPromoUI::Source source);
 
   Profile* profile;
   Browser* browser;
@@ -98,25 +100,35 @@ struct StartSyncArgs {
   std::string email;
   std::string password;
   bool force_same_tab_navigation;
-  bool confirmation_required;
+  OneClickSigninSyncStarter::ConfirmationRequired confirmation_required;
 };
 
-StartSyncArgs::StartSyncArgs(Profile* profile,
-                             Browser* browser,
-                             OneClickSigninHelper::AutoAccept auto_accept,
-                             const std::string& session_index,
-                             const std::string& email,
-                             const std::string& password,
-                             bool force_same_tab_navigation,
-                             bool confirmation_required)
+StartSyncArgs::StartSyncArgs(
+    Profile* profile,
+    Browser* browser,
+    OneClickSigninHelper::AutoAccept auto_accept,
+    const std::string& session_index,
+    const std::string& email,
+    const std::string& password,
+    bool force_same_tab_navigation,
+    bool saml_confirmation_required,
+    SyncPromoUI::Source source)
     : profile(profile),
       browser(browser),
       auto_accept(auto_accept),
       session_index(session_index),
       email(email),
       password(password),
-      force_same_tab_navigation(force_same_tab_navigation),
-      confirmation_required(confirmation_required) {
+      force_same_tab_navigation(force_same_tab_navigation) {
+  if (saml_confirmation_required) {
+    confirmation_required = OneClickSigninSyncStarter::CONFIRM_SAML_SIGNIN;
+  } else if (source == SyncPromoUI::SOURCE_SETTINGS ||
+             source == SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
+    // Do not display a status confirmation for webstore installs or re-auth.
+    confirmation_required = OneClickSigninSyncStarter::NO_CONFIRMATION;
+  } else {
+    confirmation_required = OneClickSigninSyncStarter::CONFIRM_AFTER_SIGNIN;
+  }
 }
 
 
@@ -471,7 +483,7 @@ OneClickSigninHelper::OneClickSigninHelper(content::WebContents* web_contents)
       switched_to_advanced_(false),
       original_source_(SyncPromoUI::SOURCE_UNKNOWN),
       untrusted_navigations_since_signin_visit_(0),
-      confirmation_required_(false) {
+      saml_confirmation_required_(false) {
 }
 
 OneClickSigninHelper::~OneClickSigninHelper() {
@@ -824,6 +836,8 @@ void OneClickSigninHelper::ShowInfoBarUIThread(
   if (!web_contents || !CanOffer(web_contents, can_offer_for, email,
                                  &error_message)) {
     VLOG(1) << "OneClickSigninHelper::ShowInfoBarUIThread: not offering";
+    // TODO(rogerta): Can we just display our error now instead of keeping it
+    // around and doing it later?
     if (helper && helper->error_message_.empty() && !error_message.empty())
       helper->error_message_ = error_message;
 
@@ -838,8 +852,8 @@ void OneClickSigninHelper::ShowInfoBarUIThread(
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   SigninManager* manager = profile ?
       SigninManagerFactory::GetForProfile(profile) : NULL;
-  helper->confirmation_required_ |= (manager &&
-                                     !manager->IsSigninProcess(child_id));
+  helper->saml_confirmation_required_ |= (manager &&
+                                          !manager->IsSigninProcess(child_id));
 
   // Save the email in the one-click signin manager.  The manager may
   // not exist if the contents is incognito or if the profile is already
@@ -860,28 +874,28 @@ void OneClickSigninHelper::RemoveCurrentHistoryItem(
   new CurrentHistoryCleaner(web_contents);  // will self-destruct when finished
 }
 
-void OneClickSigninHelper::ShowSyncConfirmationBubble(bool show_bubble) {
-  if (show_bubble) {
-    content::WebContents* contents = web_contents();
-    Profile* profile =
-        Profile::FromBrowserContext(contents->GetBrowserContext());
-    Browser* browser = chrome::FindBrowserWithWebContents(contents);
-
-    browser->window()->ShowOneClickSigninBubble(
-        BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
-        string16(), /* no SAML email */
-        UTF8ToUTF16(error_message_),
-        base::Bind(&StartSync,
-                   StartSyncArgs(profile, browser, AUTO_ACCEPT_ACCEPTED,
-                                 session_index_, email_, password_,
-                                 false, confirmation_required_)));
-  }
-  error_message_.clear();
+void OneClickSigninHelper::ShowSigninErrorBubble(const std::string& error) {
+  DCHECK(!error.empty());
+  content::WebContents* contents = web_contents();
+  Profile* profile =
+      Profile::FromBrowserContext(contents->GetBrowserContext());
+  Browser* browser = chrome::FindBrowserWithWebContents(contents);
+
+  browser->window()->ShowOneClickSigninBubble(
+      BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
+      string16(), /* no SAML email */
+      UTF8ToUTF16(error),
+      // This callback is never invoked.
+      // TODO(rogerta): Separate out the bubble API so we don't have to pass
+      // ignored |email| and |callback| params.
+      base::Bind(&StartSync,
+                 StartSyncArgs(profile, browser, AUTO_ACCEPT_ACCEPTED,
+                               session_index_, email_, password_,
+                               false, saml_confirmation_required_, source_)));
 }
 
-void OneClickSigninHelper::RedirectToNtpOrAppsPage(bool show_bubble) {
+void OneClickSigninHelper::RedirectToNtpOrAppsPage() {
   VLOG(1) << "OneClickSigninHelper::RedirectToNtpOrAppsPage";
-
   // Redirect to NTP/Apps page and display a confirmation bubble
   content::WebContents* contents = web_contents();
   GURL url(chrome::IsInstantExtendedAPIEnabled() ?
@@ -892,8 +906,6 @@ void OneClickSigninHelper::RedirectToNtpOrAppsPage(bool show_bubble) {
                                 content::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                 false);
   contents->OpenURL(params);
-
-  ShowSyncConfirmationBubble(show_bubble);
 }
 
 void OneClickSigninHelper::RedirectToSignin() {
@@ -924,7 +936,8 @@ void OneClickSigninHelper::CleanTransientState() {
   original_source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
   untrusted_navigations_since_signin_visit_ = 0;
-  confirmation_required_ = false;
+  saml_confirmation_required_ = false;
+  error_message_.clear();
 
   // Post to IO thread to clear pending email.
   Profile* profile =
@@ -999,10 +1012,14 @@ void OneClickSigninHelper::DidStopLoading(
   // If an error has already occured during the sign in flow, make sure to
   // display it to the user and abort the process.  Do this only for
   // explicit sign ins.
+  // TODO(rogerta): Could we move this code back up to ShowInfoBarUIThread()?
   if (!error_message_.empty() && auto_accept_ == AUTO_ACCEPT_EXPLICIT) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: error=" << error_message_;
     RemoveCurrentHistoryItem(contents);
-    RedirectToNtpOrAppsPage(true);
+    // Redirect to the landing page and display an error popup.
+    RedirectToNtpOrAppsPage();
+    ShowSigninErrorBubble(error_message_);
+    CleanTransientState();
     return;
   }
 
@@ -1037,7 +1054,7 @@ void OneClickSigninHelper::DidStopLoading(
     if (net::GetValueForKeyInQuery(url, "ntp", &unused_value)) {
       SyncPromoUI::SetUserSkippedSyncPromo(profile);
       RemoveCurrentHistoryItem(contents);
-      RedirectToNtpOrAppsPage(false);
+      RedirectToNtpOrAppsPage();
     }
 
     if (!continue_url_match && !IsValidGaiaSigninRedirectOrResponseURL(url) &&
@@ -1134,7 +1151,7 @@ void OneClickSigninHelper::DidStopLoading(
                      StartSyncArgs(profile, browser, auto_accept_,
                                    session_index_, email_, password_,
                                    false /* force_same_tab_navigation */,
-                                   confirmation_required_)));
+                                   saml_confirmation_required_, source_)));

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

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);

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

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).

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

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?

[Andrew T Wilson (Slow), 2013/05/06 17:00:13]

Done.

       break;
     case AUTO_ACCEPT_CONFIGURE:
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_ACCEPTED);
@@ -1143,7 +1160,7 @@ void OneClickSigninHelper::DidStopLoading(
       StartSync(
           StartSyncArgs(profile, browser, auto_accept_, session_index_, email_,
                         password_, false /* force_same_tab_navigation */,
-                        confirmation_required_),
+                        saml_confirmation_required_, source_),
           OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
       break;
     case AUTO_ACCEPT_EXPLICIT: {
@@ -1169,7 +1186,7 @@ void OneClickSigninHelper::DidStopLoading(
         // just signed in, show a confirmation dialog.
 
         // No need to display a second confirmation.
-        confirmation_required_ = false;
+        saml_confirmation_required_ = false;
         ConfirmEmailDialogDelegate::AskForConfirmation(
             contents,
             last_email,
@@ -1179,14 +1196,14 @@ void OneClickSigninHelper::DidStopLoading(
                 StartSyncArgs(profile, browser, auto_accept_,
                               session_index_, email_, password_,
                               force_same_tab_navigation,
-                              confirmation_required_),
+                              saml_confirmation_required_, source_),
                 contents,
                 start_mode));

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

Can we move this confirmation dialog into the starter as well?

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

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

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

Sure.

[Andrew T Wilson (Slow), 2013/05/06 17:00:13]

Done.

       } else {
         StartExplicitSync(
             StartSyncArgs(profile, browser, auto_accept_, session_index_,
                           email_, password_, force_same_tab_navigation,
-                          confirmation_required_),
+                          saml_confirmation_required_, source_),
             contents,
             start_mode,
             IDS_ONE_CLICK_SIGNIN_CONFIRM_EMAIL_DIALOG_CANCEL_BUTTON);
@@ -1208,9 +1225,8 @@ void OneClickSigninHelper::DidStopLoading(
       // it will redirect back to webstore.
       if (source_ != SyncPromoUI::SOURCE_SETTINGS &&
           source_ != SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
-        signin_tracker_.reset(new SigninTracker(profile, this));
         RemoveCurrentHistoryItem(contents);
-        RedirectToNtpOrAppsPage(false);
+        RedirectToNtpOrAppsPage();
       }
       break;
     }
@@ -1226,9 +1242,6 @@ void OneClickSigninHelper::DidStopLoading(
   CleanTransientState();
 }
 
-void OneClickSigninHelper::GaiaCredentialsValid() {
-}
-
 void OneClickSigninHelper::OnStateChanged() {
   // No redirect url after sync setup is set, thus no need to watch for sync
   // state changes.
@@ -1256,36 +1269,3 @@ void OneClickSigninHelper::OnStateChanged() {
   redirect_url_ = GURL();
   sync_service->RemoveObserver(this);
 }
-
-void OneClickSigninHelper::SigninFailed(const GoogleServiceAuthError& error) {
-  if (error_message_.empty() && !error.error_message().empty())
-      error_message_ = error.error_message();
-
-  bool display_bubble = true;
-  if (error_message_.empty()) {
-    switch (error.state()) {
-      case GoogleServiceAuthError::NONE:
-        error_message_.clear();
-        break;
-      case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
-        error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_UNRECOVERABLE_ERROR);
-        break;
-      case GoogleServiceAuthError::REQUEST_CANCELED:
-        // If the user cancelled signin, then no need to display any error
-        // messages or anything - just go back to the NTP.
-        error_message_.clear();
-        display_bubble = false;
-        break;
-      default:
-        error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_ERROR_SIGNING_IN);
-        break;
-    }
-  }
-  ShowSyncConfirmationBubble(display_bubble);
-  signin_tracker_.reset();
-}
-
-void OneClickSigninHelper::SigninSuccess() {
-  ShowSyncConfirmationBubble(true);
-  signin_tracker_.reset();
-}