Setup from settings should allow configuration first.

BUG=81265
TEST=When setting up sync using the gaia-based web flow, the user should have
the option to configure sync before it starts.  The UX should be same as the
native ui flow.


Review URL: https://chromiumcodereview.appspot.com/11418200

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170999 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/sync/one_click_signin_helper.cc

@@ -108,10 +108,8 @@ void StartSync(Browser* browser,
 }
 
 bool UseWebBasedSigninFlow() {
-  static const bool use_web_based_singin_flow =
-      CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kUseWebBasedSigninFlow);
-  return use_web_based_singin_flow;
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kUseWebBasedSigninFlow);
 }
 
 // Determines the source of the sign in.  Its either one of the known sign in
@@ -439,20 +437,22 @@ bool OneClickSigninHelper::CanOffer(content::WebContents* web_contents,
       }
     }
 
-    // If we're about to show a one-click infobar but the user has started
-    // a concurrent signin flow (perhaps via the promo), we may not have yet
-    // established an authenticated username but we still shouldn't move
-    // forward with two simultaneous signin processes.  This is a bit
-    // contentious as the one-click flow is a much smoother flow from the user
-    // perspective, but it's much more difficult to hijack the other flow from
-    // here as it is to bail.
-    ProfileSyncService* service =
-        ProfileSyncServiceFactory::GetForProfile(profile);
-    if (!service)
-      return false;
-
-    if (service->FirstSetupInProgress())
-      return false;
+    if (!UseWebBasedSigninFlow()) {
+      // If we're about to show a one-click infobar but the user has started
+      // a concurrent signin flow (perhaps via the promo), we may not have yet
+      // established an authenticated username but we still shouldn't move
+      // forward with two simultaneous signin processes.  This is a bit
+      // contentious as the one-click flow is a much smoother flow from the user
+      // perspective, but it's much more difficult to hijack the other flow from
+      // here as it is to bail.
+      ProfileSyncService* service =
+          ProfileSyncServiceFactory::GetForProfile(profile);
+      if (!service)
+        return false;
+
+      if (service->FirstSetupInProgress())
+        return false;
+    }
   }
 
   return true;
@@ -724,7 +724,9 @@ void OneClickSigninHelper::DidStopLoading(
       break;
     case AUTO_ACCEPT_EXPLICIT:
       StartSync(browser, auto_accept_, session_index_, email_, password_,
-                OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
+                source_ == SyncPromoUI::SOURCE_SETTINGS ?
+                    OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST :
+                    OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
       break;
     case REJECTED_FOR_PROFILE:
       UMA_HISTOGRAM_ENUMERATION("AutoLogin.Reverse",
@@ -736,30 +738,20 @@ void OneClickSigninHelper::DidStopLoading(
       break;
   }
 
-  AutoAccept local_auto_accept = auto_accept_;
-  SyncPromoUI::Source local_source = source_;
+  // If this explicit sign in is not from settings page, show the NTP after
+  // sign in completes.  In the case of the settings page, it will get closed
+  // by SyncSetupHandler.
+  if (auto_accept_ == AUTO_ACCEPT_EXPLICIT &&
+      source_ != SyncPromoUI::SOURCE_SETTINGS) {
+    Profile* profile =
+        Profile::FromBrowserContext(contents->GetBrowserContext());
+    signin_tracker_.reset(new SigninTracker(profile, this));
+  }
 
   email_.clear();
   password_.clear();
   auto_accept_ = NO_AUTO_ACCEPT;
   source_ = SyncPromoUI::SOURCE_UNKNOWN;
-
-  // If this is an explicit sign in by the user, then redirect them to the
-  // NTP.
-  if (local_auto_accept == AUTO_ACCEPT_EXPLICIT) {
-    // If this is an explicit sign in from settings page, then close the
-    // tab.  Otherwise show the NTP after sign in completes.
-    if (local_source == SyncPromoUI::SOURCE_SETTINGS) {
-      contents->Close();
-    } else {
-      Profile* profile =
-          Profile::FromBrowserContext(contents->GetBrowserContext());
-      signin_tracker_.reset(new SigninTracker(profile, this));
-    }
-  }
-
-  // |this| may have been deleted due to the contents->Close() call above.
-  // No members should be accessed at this point.
 }
 
 void OneClickSigninHelper::GaiaCredentialsValid() {

chrome/browser/ui/sync/one_click_signin_helper_unittest.cc

@@ -48,6 +48,11 @@ const char kImplicitURLString[] =
     "https://accounts.google.com/ServiceLogin"
     "?service=foo&continue=http://foo.google.com";
 
+bool UseWebBasedSigninFlow() {
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kUseWebBasedSigninFlow);
+}
+
 class SigninManagerMock : public FakeSigninManager {
  public:
   explicit SigninManagerMock(Profile* profile)
@@ -352,21 +357,22 @@ TEST_F(OneClickSigninHelperTest, CanOfferFirstSetup) {
   EXPECT_CALL(*signin_manager_, IsAllowedUsername(_)).
         WillRepeatedly(Return(true));
 
-   // Invoke OneClickTestProfileSyncService factory function and grab result.
-   OneClickTestProfileSyncService* sync =
-       static_cast<OneClickTestProfileSyncService*>(
-           ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-               static_cast<Profile*>(browser_context()),
-               OneClickTestProfileSyncService::Build));
+  // Invoke OneClickTestProfileSyncService factory function and grab result.
+  OneClickTestProfileSyncService* sync =
+      static_cast<OneClickTestProfileSyncService*>(
+          ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+              static_cast<Profile*>(browser_context()),
+              OneClickTestProfileSyncService::Build));
 
-   sync->set_first_setup_in_progress(true);
+  sync->set_first_setup_in_progress(true);
 
-   EXPECT_FALSE(OneClickSigninHelper::CanOffer(web_contents(),
-                                               "foo@gmail.com",
-                                               true));
-   EXPECT_TRUE(OneClickSigninHelper::CanOffer(web_contents(),
-                                              "foo@gmail.com",
-                                              false));
+  EXPECT_EQ(UseWebBasedSigninFlow(),
+            OneClickSigninHelper::CanOffer(web_contents(),
+                                           "foo@gmail.com",
+                                           true));
+  EXPECT_TRUE(OneClickSigninHelper::CanOffer(web_contents(),
+                                             "foo@gmail.com",
+                                             false));
 }
 
 TEST_F(OneClickSigninHelperTest, CanOfferProfileConnected) {

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

@@ -59,18 +59,23 @@ void OneClickSigninSyncStarter::SigninSuccess() {
   ProfileSyncService* profile_sync_service =
       ProfileSyncServiceFactory::GetForProfile(browser_->profile());
 
-  if (start_mode_ == SYNC_WITH_DEFAULT_SETTINGS) {
-    // Just kick off the sync machine, no need to configure it first.
-    profile_sync_service->OnUserChoseDatatypes(true, syncer::ModelTypeSet());
-    profile_sync_service->SetSyncSetupCompleted();
-    profile_sync_service->SetSetupInProgress(false);
-  } else {
-    // Give the user a chance to configure things. We don't clear the
-    // ProfileSyncService::setup_in_progress flag because we don't want sync
-    // to start up until after the configure UI is displayed (the configure UI
-    // will clear the flag when the user is done setting up sync).
-    LoginUIServiceFactory::GetForProfile(browser_->profile())->ShowLoginUI(
-        browser_);
+  switch (start_mode_) {
+    case SYNC_WITH_DEFAULT_SETTINGS:
+      // Just kick off the sync machine, no need to configure it first.
+      profile_sync_service->OnUserChoseDatatypes(true, syncer::ModelTypeSet());
+      profile_sync_service->SetSyncSetupCompleted();
+      profile_sync_service->SetSetupInProgress(false);
+      break;
+    case CONFIGURE_SYNC_FIRST:
+      // Give the user a chance to configure things. We don't clear the
+      // ProfileSyncService::setup_in_progress flag because we don't want sync
+      // to start up until after the configure UI is displayed (the configure UI
+      // will clear the flag when the user is done setting up sync).
+      LoginUIServiceFactory::GetForProfile(browser_->profile())->ShowLoginUI(
+          browser_);
+      break;
+    default:
+      NOTREACHED() << "Invalid start_mode=" << start_mode_;
   }
 
   delete this;

chrome/browser/ui/sync/one_click_signin_sync_starter.h

@@ -16,7 +16,16 @@ class Browser;
 // the job is done.
 class OneClickSigninSyncStarter : public SigninTracker::Observer {
  public:
-  enum StartSyncMode {SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST };
+  enum StartSyncMode {
+    // Starts the process of signing the user in with the SigninManager, and
+    // once completed automatically starts sync with all data types enabled.
+    SYNC_WITH_DEFAULT_SETTINGS,
+
+    // Starts the process of signing the user in with the SigninManager, and
+    // once completed redirects the user to the settings page to allow them
+    // to configure which data types to sync before sync is enabled.
+    CONFIGURE_SYNC_FIRST
+  };
 
   OneClickSigninSyncStarter(Browser* browser,
                             const std::string& session_index,

chrome/browser/ui/webui/sync_setup_handler.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/browser/ui/webui/sync_promo/sync_promo_ui.h"
@@ -191,13 +192,26 @@ bool UseWebBasedSigninFlow() {
       switches::kUseWebBasedSigninFlow);
 }
 
+void BringTabToFront(WebContents* web_contents) {
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (browser) {
+    TabStripModel* tab_strip_model = browser->tab_strip_model();
+    if (tab_strip_model) {
+      int index = tab_strip_model->GetIndexOfWebContents(web_contents);
+      if (index != TabStripModel::kNoTab)
+        tab_strip_model->ActivateTabAt(index, false);
+    }
+  }
+}
+
 }  // namespace
 
 SyncSetupHandler::SyncSetupHandler(ProfileManager* profile_manager)
     : configuring_sync_(false),
       profile_manager_(profile_manager),
       last_signin_error_(GoogleServiceAuthError::NONE),
-      retry_on_signin_failure_(true) {
+      retry_on_signin_failure_(true),
+      active_gaia_signin_tab_(NULL) {
 }
 
 SyncSetupHandler::~SyncSetupHandler() {
@@ -489,6 +503,12 @@ void SyncSetupHandler::DisplayConfigureSync(bool show_advanced,
   StringValue page("configure");
   web_ui()->CallJavascriptFunction(
       "SyncSetupOverlay.showSyncSetupPage", page, args);
+
+  if (UseWebBasedSigninFlow()) {
+    // Make sure the tab used for the Gaia sign in does not cover the settings
+    // tab.
+    BringTabToFront(web_ui()->GetWebContents());
+  }
 }
 
 void SyncSetupHandler::ConfigureSyncDone() {
@@ -566,13 +586,25 @@ SigninManager* SyncSetupHandler::GetSignin() const {
 
 void SyncSetupHandler::DisplayGaiaLogin(bool fatal_error) {
   if (UseWebBasedSigninFlow()) {
+    DCHECK(!active_gaia_signin_tab_);
+
+    // Advanced options are no longer being configured if the login screen is
+    // visible. If the user exits the signin wizard after this without
+    // configuring sync, CloseSyncSetup() will ensure they are logged out.
+    configuring_sync_ = false;
+
     GURL url(SyncPromoUI::GetSyncPromoURL(GURL(),
         SyncPromoUI::SOURCE_SETTINGS, false));
     Browser* browser = chrome::FindBrowserWithWebContents(
         web_ui()->GetWebContents());
-    browser->OpenURL(
+    active_gaia_signin_tab_ = browser->OpenURL(
         content::OpenURLParams(url, content::Referrer(), SINGLETON_TAB,
-                               content::PAGE_TRANSITION_AUTO_BOOKMARK, false));
+                                content::PAGE_TRANSITION_AUTO_BOOKMARK,
+                                false));
+    content::WebContentsObserver::Observe(active_gaia_signin_tab_);
+    signin_tracker_.reset(
+        new SigninTracker(GetProfile(), this,
+                          SigninTracker::WAITING_FOR_GAIA_VALIDATION));
   } else {
     retry_on_signin_failure_ = true;
     DisplayGaiaLoginWithErrorMessage(string16(), fatal_error);
@@ -581,9 +613,9 @@ void SyncSetupHandler::DisplayGaiaLogin(bool fatal_error) {
 
 void SyncSetupHandler::DisplayGaiaLoginWithErrorMessage(
     const string16& error_message, bool fatal_error) {
-  // We are no longer configuring sync if the login screen is visible.
-  // If the user exits the signin wizard after this without configuring sync,
-  // CloseSyncSetup() will ensure they are logged out.
+  // Advanced options are no longer being configured if the login screen is
+  // visible. If the user exits the signin wizard after this without
+  // configuring sync, CloseSyncSetup() will ensure they are logged out.
   configuring_sync_ = false;
 
   string16 local_error_message(error_message);
@@ -662,11 +694,9 @@ bool SyncSetupHandler::PrepareSyncSetup() {
     return false;
   }
 
-  if (!UseWebBasedSigninFlow()) {
-    // Notify services that login UI is now active.
-    GetLoginUIService()->SetLoginUI(this);
-    service->SetSetupInProgress(true);
-  }
+  // Notify services that login UI is now active.
+  GetLoginUIService()->SetLoginUI(this);
+  service->SetSetupInProgress(true);
 
   return true;
 }
@@ -713,6 +743,9 @@ void SyncSetupHandler::DisplayGaiaSuccessAndClose() {
 
 void SyncSetupHandler::DisplayGaiaSuccessAndSettingUp() {
   RecordSignin();
+  if (UseWebBasedSigninFlow())
+    CloseGaiaSigninPage();
+
   web_ui()->CallJavascriptFunction("SyncSetupOverlay.showSuccessAndSettingUp");
 }
 
@@ -1013,6 +1046,7 @@ void SyncSetupHandler::CloseSyncSetup() {
             ProfileSyncService::CANCEL_FROM_SIGNON_WITHOUT_AUTH);
       }
     }
+
     // Let the various services know that we're no longer active.
     GetLoginUIService()->LoginUIClosed(this);
   }
@@ -1090,8 +1124,12 @@ void SyncSetupHandler::OpenConfigureSync() {
 
 void SyncSetupHandler::FocusUI() {
   DCHECK(IsActiveLogin());
-  WebContents* web_contents = web_ui()->GetWebContents();
-  web_contents->GetDelegate()->ActivateContents(web_contents);
+  if (UseWebBasedSigninFlow() && signin_tracker_) {
+      BringTabToFront(active_gaia_signin_tab_);
+  } else {
+    WebContents* web_contents = web_ui()->GetWebContents();
+    web_contents->GetDelegate()->ActivateContents(web_contents);
+  }
 }
 
 void SyncSetupHandler::CloseUI() {
@@ -1099,6 +1137,12 @@ void SyncSetupHandler::CloseUI() {
   CloseOverlay();
 }
 
+void SyncSetupHandler::WebContentsDestroyed(
+    content::WebContents* web_contents) {
+  DCHECK(active_gaia_signin_tab_);
+  CloseSyncSetup();
+}
+
 // Private member functions.
 
 bool SyncSetupHandler::FocusExistingWizardIfPresent() {
@@ -1121,6 +1165,28 @@ void SyncSetupHandler::CloseOverlay() {
   web_ui()->CallJavascriptFunction("OptionsPage.closeOverlay");
 }
 
+void SyncSetupHandler::CloseGaiaSigninPage() {
+  if (active_gaia_signin_tab_) {
+    content::WebContentsObserver::Observe(NULL);
+
+    Browser* browser = chrome::FindBrowserWithWebContents(
+        active_gaia_signin_tab_);
+    if (browser) {
+      TabStripModel* tab_strip_model = browser->tab_strip_model();
+      if (tab_strip_model) {
+        int index = tab_strip_model->GetIndexOfWebContents(
+            active_gaia_signin_tab_);
+        if (index != TabStripModel::kNoTab) {
+          tab_strip_model->ExecuteContextMenuCommand(
+              index, TabStripModel::CommandCloseTab);
+        }
+      }
+    }
+  }
+
+  active_gaia_signin_tab_ = NULL;
+}
+
 bool SyncSetupHandler::IsLoginAuthDataValid(const std::string& username,
                                             string16* error_message) {
   if (username.empty())

chrome/browser/ui/webui/sync_setup_handler.h

@@ -11,15 +11,21 @@
 #include "chrome/browser/signin/signin_tracker.h"
 #include "chrome/browser/ui/webui/options/options_ui.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
+#include "content/public/browser/web_contents_observer.h"
 
 class LoginUIService;
 class ProfileManager;
 class ProfileSyncService;
 class SigninManager;
 
+namespace content {
+class WebContents;
+}
+
 class SyncSetupHandler : public options::OptionsPageUIHandler,
                          public SigninTracker::Observer,
-                         public LoginUIService::LoginUI {
+                         public LoginUIService::LoginUI,
+                         public content::WebContentsObserver {
  public:
   // Constructs a new SyncSetupHandler. |profile_manager| may be NULL.
   explicit SyncSetupHandler(ProfileManager* profile_manager);
@@ -39,6 +45,10 @@ class SyncSetupHandler : public options::OptionsPageUIHandler,
   virtual void FocusUI() OVERRIDE;
   virtual void CloseUI() OVERRIDE;
 
+  // content::WebContentsObserver implementation.
+  virtual void WebContentsDestroyed(
+      content::WebContents* web_contents) OVERRIDE;
+
   static void GetStaticLocalizedValues(
       base::DictionaryValue* localized_strings,
       content::WebUI* web_ui);
@@ -164,6 +174,10 @@ class SyncSetupHandler : public options::OptionsPageUIHandler,
   // Invokes the javascript call to close the setup overlay.
   void CloseOverlay();
 
+  // When using web-flow, closes the Gaia page used to collection user
+  // credentials.
+  void CloseGaiaSigninPage();
+
   // Returns true if the given login data is valid, false otherwise. If the
   // login data is not valid then on return |error_message| will be set to  a
   // localized error message. Note, |error_message| must not be NULL.
@@ -201,6 +215,10 @@ class SyncSetupHandler : public options::OptionsPageUIHandler,
   // service.
   scoped_ptr<base::OneShotTimer<SyncSetupHandler> > backend_start_timer_;
 
+  // When using web-flow, weak pointer to the tab that holds the Gaia sign in
+  // page.
+  content::WebContents* active_gaia_signin_tab_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSetupHandler);
 };