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 f9f4e7127406f00522b8bec9252ad61908ccd78d..e5d3e309a7066fd3411a7b04953fe4e5336e186f 100644
--- a/chrome/browser/ui/sync/one_click_signin_helper.cc
+++ b/chrome/browser/ui/sync/one_click_signin_helper.cc
@@ -84,14 +84,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 untrusted_confirmation_required,
+      SyncPromoUI::Source source);
 
   Profile* profile;
   Browser* browser;
@@ -100,25 +102,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 untrusted_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 (untrusted_confirmation_required) {
+    confirmation_required = OneClickSigninSyncStarter::CONFIRM_UNTRUSTED_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;
+  }
 }
 
 
@@ -477,7 +489,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) {
+      untrusted_confirmation_required_(false) {
 }
 
 OneClickSigninHelper::~OneClickSigninHelper() {
@@ -831,6 +843,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;
 
@@ -845,8 +859,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->untrusted_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
@@ -867,28 +881,29 @@ 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, untrusted_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(source_ == SyncPromoUI::SOURCE_APPS_PAGE_LINK ?
@@ -899,8 +914,6 @@ void OneClickSigninHelper::RedirectToNtpOrAppsPage(bool show_bubble) {
                                 content::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                 false);
   contents->OpenURL(params);
-
-  ShowSyncConfirmationBubble(show_bubble);
 }
 
 void OneClickSigninHelper::RedirectToSignin() {
@@ -931,7 +944,8 @@ void OneClickSigninHelper::CleanTransientState() {
   original_source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
   untrusted_navigations_since_signin_visit_ = 0;
-  confirmation_required_ = false;
+  untrusted_confirmation_required_ = false;
+  error_message_.clear();
 
   // Post to IO thread to clear pending email.
   Profile* profile =
@@ -1006,10 +1020,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;
   }
 
@@ -1044,7 +1062,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) &&
@@ -1123,6 +1141,9 @@ void OneClickSigninHelper::DidStopLoading(
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_ACCEPTED);
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_WITH_DEFAULTS);
       SigninManager::DisableOneClickSignIn(profile);
+      // No need to display a second confirmation since we're displaying one
+      // already.
+      untrusted_confirmation_required_ = false;
       browser->window()->ShowOneClickSigninBubble(
           BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG,
           UTF8ToUTF16(email_),
@@ -1131,7 +1152,7 @@ void OneClickSigninHelper::DidStopLoading(
                      StartSyncArgs(profile, browser, auto_accept_,
                                    session_index_, email_, password_,
                                    false /* force_same_tab_navigation */,
-                                   confirmation_required_)));
+                                   untrusted_confirmation_required_, source_)));
       break;
     case AUTO_ACCEPT_CONFIGURE:
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_ACCEPTED);
@@ -1140,7 +1161,7 @@ void OneClickSigninHelper::DidStopLoading(
       StartSync(
           StartSyncArgs(profile, browser, auto_accept_, session_index_, email_,
                         password_, false /* force_same_tab_navigation */,
-                        confirmation_required_),
+                        untrusted_confirmation_required_, source_),

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

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:13]

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]

OK.

           OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
       break;
     case AUTO_ACCEPT_EXPLICIT: {
@@ -1166,7 +1187,7 @@ void OneClickSigninHelper::DidStopLoading(
         // just signed in, show a confirmation dialog.
 
         // No need to display a second confirmation.
-        confirmation_required_ = false;
+        untrusted_confirmation_required_ = false;
         ConfirmEmailDialogDelegate::AskForConfirmation(
             contents,
             last_email,
@@ -1176,14 +1197,14 @@ void OneClickSigninHelper::DidStopLoading(
                 StartSyncArgs(profile, browser, auto_accept_,
                               session_index_, email_, password_,
                               force_same_tab_navigation,
-                              confirmation_required_),
+                              untrusted_confirmation_required_, source_),
                 contents,
                 start_mode));
       } else {
         StartExplicitSync(
             StartSyncArgs(profile, browser, auto_accept_, session_index_,
                           email_, password_, force_same_tab_navigation,
-                          confirmation_required_),
+                          untrusted_confirmation_required_, source_),
             contents,
             start_mode,
             IDS_ONE_CLICK_SIGNIN_CONFIRM_EMAIL_DIALOG_CANCEL_BUTTON);
@@ -1205,9 +1226,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;
     }
@@ -1223,9 +1243,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.
@@ -1253,36 +1270,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();
-}