Make sure the new confirmation bubble is always shown upon signin

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/sync/one_click_signin_sync_starter.h"
 
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 169 matching lines @@
   SetUserChoiceHistogram(SIGNIN_CHOICE_CANCEL);
   if (sync_starter_ != NULL)
     sync_starter_->CancelSigninAndDelete();
 }
 
 void OneClickSigninSyncStarter::SigninDialogDelegate::OnContinueSignin() {
   SetUserChoiceHistogram(SIGNIN_CHOICE_CONTINUE);
 
   if (sync_starter_ != NULL) {
     // If the user signs in from the new avatar bubble, the enterprise
-    // confirmation dialog would dismiss the avatar bubble, thus it won't show
-    // any confirmation upon sign in completes. This cofirmation dialog already
-    // mentions that user data would be synced, thus we just start sync
-    // immediately.
-
-    // TODO(guohui): add a sync settings link to allow user to configure sync
-    // settings before sync starts.
+    // confirmation dialog would dismiss the avatar bubble, thus we need to
+    // manually re-show the avatar bubble to confirm sync settings.
     if (sync_starter_->GetStartSyncMode() == CONFIRM_SYNC_SETTINGS_FIRST)
-      sync_starter_->SetStartSyncMode(SYNC_WITH_DEFAULT_SETTINGS);
+      sync_starter_->SetConfirmationRequired(CONFIRM_AFTER_SIGNIN);
     sync_starter_->LoadPolicyWithCachedCredentials();
   }
 }
 
 void OneClickSigninSyncStarter::SigninDialogDelegate::OnSigninWithNewProfile() {
   SetUserChoiceHistogram(SIGNIN_CHOICE_NEW_PROFILE);
 
   if (sync_starter_ != NULL) {
-    // TODO(guohui): add a sync settings link to allow user to configure sync
-    // settings before sync starts.
     if (sync_starter_->GetStartSyncMode() == CONFIRM_SYNC_SETTINGS_FIRST)
-      sync_starter_->SetStartSyncMode(SYNC_WITH_DEFAULT_SETTINGS);
+      sync_starter_->SetConfirmationRequired(CONFIRM_AFTER_SIGNIN);
     sync_starter_->CreateNewSignedInProfile();
   }
 }
 
-void OneClickSigninSyncStarter::SetStartSyncMode(StartSyncMode start_mode) {
-  start_mode_ = start_mode;
+void OneClickSigninSyncStarter::SetConfirmationRequired(
+    ConfirmationRequired confirmation_required) {
+  confirmation_required_ = confirmation_required;
 }
 
 OneClickSigninSyncStarter::StartSyncMode
     OneClickSigninSyncStarter::GetStartSyncMode() {
   return start_mode_;
 }
 
 void OneClickSigninSyncStarter::OnRegisteredForPolicy(
     const std::string& dm_token, const std::string& client_id) {
   SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
@@ skipping 248 matching lines @@
           // Sync is disabled by policy.
           message = l10n_util::GetStringUTF16(
               IDS_ONE_CLICK_SIGNIN_BUBBLE_SYNC_DISABLED_MESSAGE);
         }
         DisplayFinalConfirmationBubble(message);
       }
       break;
     }
     case CONFIRM_SYNC_SETTINGS_FIRST:
       // Blocks sync until the sync settings confirmation UI is closed.
+      if (confirmation_required_ == CONFIRM_AFTER_SIGNIN)
+        DisplayFinalConfirmationBubble(base::string16());

[Roger Tawa OOO till Jul 10th, 2014/08/15 15:49:13]

What if you always call DisplayFinalConfirmationBubble() here, and remove the
code in ProfileChooserView::OnRefreshTokenAvailable() that switches to
TUTORIAL_MODE_CONFIRM_SIGNIN?

Maybe ProfileChooserView::OnRefreshTokenAvailable() should completely ignore
profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN?

[guohui, 2014/08/15 15:57:52]

as discussed yesterday, that would be a cleaner solution, however it requires
non-trivial changes. I am working on three other CLs for M38 blockers, thus I am
de-prioritizing any refactoring work for now. If time allows i ll upload another
patch with the refactoring, otherwise i will just land this CL as it is and
refactor in the next CL.

I am more concerned about the no-confirmation bug you mentioned in the email, do
you still see it?

       return;
     case CONFIGURE_SYNC_FIRST:
       ShowSettingsPage(true);  // Show sync config UI.
       break;
     case SHOW_SETTINGS_WITHOUT_CONFIGURE:
       ShowSettingsPage(false);  // Don't show sync config UI.
       break;
     case UNDO_SYNC:
       NOTREACHED();
   }
 
   // Navigate to the |continue_url_| if one is set, unless the user first needs
   // to configure Sync.
   if (web_contents() && !continue_url_.is_empty() &&
       start_mode_ != CONFIGURE_SYNC_FIRST) {
     LoadContinueUrl();
   }
 
   delete this;
 }
 
 void OneClickSigninSyncStarter::DisplayFinalConfirmationBubble(
     const base::string16& custom_message) {
   browser_ = EnsureBrowser(browser_, profile_, desktop_type_);
+  // Show the success confirmation message in the new avatar menu if it is
+  // enabled.
+  // TODO(guohui): needs to handle custom messages.
+  if (custom_message.empty() && switches::IsNewAvatarMenu()) {
+    browser_->window()->ShowAvatarBubbleFromAvatarButton(
+        BrowserWindow::AVATAR_BUBBLE_MODE_CONFIRM_SIGNIN,
+        signin::ManageAccountsParams());
+    return;
+  }
+
   browser_->window()->ShowOneClickSigninBubble(
       BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
       base::string16(),  // No email required - this is not a SAML confirmation.
       custom_message,
       // Callback is ignored.
       BrowserWindow::StartSyncCallback());
 }
 
 // static
 Browser* OneClickSigninSyncStarter::EnsureBrowser(
@@ skipping 113 matching lines @@
                                             false /* user_gesture */);
 }
 
 void OneClickSigninSyncStarter::LoadContinueUrl() {
   web_contents()->GetController().LoadURL(
       continue_url_,
       content::Referrer(),
       content::PAGE_TRANSITION_AUTO_TOPLEVEL,
       std::string());
 }