[sync] Eliminate reload loop in sync settings page

During initial sync setup, if the user refreshes the settings page when
the advanced settings page is visible, and then hits the back button, we
enter a page reload loop.

A couple of salient features of the loop are:

1) When the back button is pressed, SyncSetupHandler::CloseSyncSetup()
   is called, which causes ProfileSyncService::NotifyObservers() to
   again indirectly call SyncSetupHandler::CloseSyncSetup(), due to
   SigninTracker::OnStateChanged() incorrectly detecting a signin
   failure.
2) When the sync setup page is loaded, SyncSetupHandler::OpenSyncSetup()
   calls OptionsSyncSetupHandler::ShowSetupUI(), resulting in an
   unnecessary navigation to chrome://settings/syncSetup, given that we
   are already at the sync setup page.

This patch does a few things to eliminate reload loops:

1) Removes SyncSetupHandler::ShowSetupUI(), and eliminates the derived
   class OptionsSyncSetupHandler. With web based sign in, there is no
   longer the need for SyncSetupHandler to re-navigate to
   chrome://settings/syncSetup.
2) In SyncSetupHandler::CloseSyncSetup(), we clear SigninTracker
   before potentially aborting sync setup. This eliminates the case
   where SigninTracker detects an abandoned sync setup as a signin
   failure.
3) In ProfileSyncService::SetSetupInProgress(), we only call
   NotifyObservers() if the value of |setup_in_progress_| changes.
   This eliminates additional recursive calls to
   SyncSetupHandler::CloseSyncSetup() in cases where it is called in
   response to ProfileSyncService::NotifyObservers() originating from
   ProfileSyncService::SetSetupInProgress().

BUG=245219
TEST=Repro steps in the above bug shouldn't result in a reload loop

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

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

11 files changed, 19 insertions, 84 deletions

diff --git a/chrome/browser/signin/signin_tracker.h b/chrome/browser/signin/signin_tracker.h
index 5fa4af4..ea7bdc7 100644
--- a/chrome/browser/signin/signin_tracker.h
+++ b/chrome/browser/signin/signin_tracker.h
@@ -27,9 +27,7 @@ class Profile;
 // the web UI for performing system login and sync configuration. Receives
 // callbacks from the UI when the user wishes to initiate a login, and
 // translates system state (login errors, etc) into the appropriate calls into
-// the UI to reflect this status to the user. Various subclasses
-// (OptionsSyncSetupHandler and SyncPromoHandler provide different UIs to the
-// user, but the core logic lies in the base SyncSetupHandler class).
+// the UI to reflect this status to the user.
 //
 // LoginUIService - Our desktop UI flows rely on having only a single login flow
 // visible to the user at once. This is achieved via LoginUIService
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 2042e8d..6d6e023 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -1404,13 +1404,13 @@ bool ProfileSyncService::FirstSetupInProgress() const {
 }
 
 void ProfileSyncService::SetSetupInProgress(bool setup_in_progress) {
-  bool was_in_progress = setup_in_progress_;
+  // This method is a no-op if |setup_in_progress_| remains unchanged.
+  if (setup_in_progress_ == setup_in_progress)
+    return;
+
   setup_in_progress_ = setup_in_progress;
-  if (!setup_in_progress && was_in_progress) {
-    if (sync_initialized()) {
-      ReconfigureDatatypeManager();
-    }
-  }
+  if (!setup_in_progress && sync_initialized())
+    ReconfigureDatatypeManager();
   NotifyObservers();
 }
 
diff --git a/chrome/browser/ui/webui/options/options_sync_setup_handler.cc b/chrome/browser/ui/webui/options/options_sync_setup_handler.cc
deleted file mode 100644
index f1b43f2c..0000000
--- a/chrome/browser/ui/webui/options/options_sync_setup_handler.cc
+++ /dev/null
@@ -1,27 +0,0 @@
-// 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/webui/options/options_sync_setup_handler.h"
-
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sync/profile_sync_service.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
-#include "content/public/browser/web_ui.h"
-
-namespace options {
-
-OptionsSyncSetupHandler::OptionsSyncSetupHandler(
-    ProfileManager* profile_manager) : SyncSetupHandler(profile_manager) {
-}
-
-OptionsSyncSetupHandler::~OptionsSyncSetupHandler() {
-}
-
-void OptionsSyncSetupHandler::ShowSetupUI() {
-  // Show the Sync Setup page.
-  web_ui()->CallJavascriptFunction("OptionsPage.navigateToPage",
-                                   base::StringValue("syncSetup"));
-}
-
-}  // namespace options
diff --git a/chrome/browser/ui/webui/options/options_sync_setup_handler.h b/chrome/browser/ui/webui/options/options_sync_setup_handler.h
deleted file mode 100644
index 74855ef..0000000
--- a/chrome/browser/ui/webui/options/options_sync_setup_handler.h
+++ /dev/null
@@ -1,25 +0,0 @@
-// 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.
-
-#ifndef CHROME_BROWSER_UI_WEBUI_OPTIONS_OPTIONS_SYNC_SETUP_HANDLER_H_
-#define CHROME_BROWSER_UI_WEBUI_OPTIONS_OPTIONS_SYNC_SETUP_HANDLER_H_
-
-#include "chrome/browser/ui/webui/sync_setup_handler.h"
-
-namespace options {
-
-// The handler for Javascript messages related to sync setup UI in the options
-// page.
-class OptionsSyncSetupHandler : public SyncSetupHandler {
- public:
-  explicit OptionsSyncSetupHandler(ProfileManager* profile_manager);
-  virtual ~OptionsSyncSetupHandler();
-
- protected:
-  virtual void ShowSetupUI() OVERRIDE;
-};
-
-}  // namespace options
-
-#endif  // CHROME_BROWSER_UI_WEBUI_OPTIONS_OPTIONS_SYNC_SETUP_HANDLER_H_
diff --git a/chrome/browser/ui/webui/options/options_ui.cc b/chrome/browser/ui/webui/options/options_ui.cc
index 85bac3b..7add1ee 100644
--- a/chrome/browser/ui/webui/options/options_ui.cc
+++ b/chrome/browser/ui/webui/options/options_ui.cc
@@ -39,11 +39,11 @@
 #include "chrome/browser/ui/webui/options/managed_user_learn_more_handler.h"
 #include "chrome/browser/ui/webui/options/media_devices_selection_handler.h"
 #include "chrome/browser/ui/webui/options/media_galleries_handler.h"
-#include "chrome/browser/ui/webui/options/options_sync_setup_handler.h"
 #include "chrome/browser/ui/webui/options/password_manager_handler.h"
 #include "chrome/browser/ui/webui/options/reset_profile_settings_handler.h"
 #include "chrome/browser/ui/webui/options/search_engine_manager_handler.h"
 #include "chrome/browser/ui/webui/options/startup_pages_handler.h"
+#include "chrome/browser/ui/webui/sync_setup_handler.h"
 #include "chrome/browser/ui/webui/theme_source.h"
 #include "chrome/common/time_format.h"
 #include "chrome/common/url_constants.h"
@@ -277,7 +277,7 @@ OptionsUI::OptionsUI(content::WebUI* web_ui)
   AddOptionsPageUIHandler(localized_strings, new SearchEngineManagerHandler());
   AddOptionsPageUIHandler(localized_strings, new ImportDataHandler());
   AddOptionsPageUIHandler(localized_strings, new StartupPagesHandler());
-  AddOptionsPageUIHandler(localized_strings, new OptionsSyncSetupHandler(
+  AddOptionsPageUIHandler(localized_strings, new SyncSetupHandler(
       g_browser_process->profile_manager()));
 #if defined(OS_CHROMEOS)
   AddOptionsPageUIHandler(localized_strings,
diff --git a/chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc b/chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc
index 9d11bb4b..6a6a27e 100644
--- a/chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc
+++ b/chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc
@@ -189,9 +189,6 @@ void SyncPromoHandler::Observe(int type,
   }
 }
 
-void SyncPromoHandler::ShowSetupUI() {
-}
-
 void SyncPromoHandler::HandleCloseSyncPromo(const base::ListValue* args) {
   CloseSyncSetup();
 
diff --git a/chrome/browser/ui/webui/sync_promo/sync_promo_handler.h b/chrome/browser/ui/webui/sync_promo/sync_promo_handler.h
index 8f25ab4..6676930 100644
--- a/chrome/browser/ui/webui/sync_promo/sync_promo_handler.h
+++ b/chrome/browser/ui/webui/sync_promo/sync_promo_handler.h
@@ -41,8 +41,6 @@ class SyncPromoHandler : public SyncSetupHandler {
   virtual void CloseUI() OVERRIDE;
 
  protected:
-  virtual void ShowSetupUI() OVERRIDE;
-
   virtual void RecordSignin() OVERRIDE;
 
  private:
diff --git a/chrome/browser/ui/webui/sync_setup_handler.cc b/chrome/browser/ui/webui/sync_setup_handler.cc
index 6c67046..228cf8d 100644
--- a/chrome/browser/ui/webui/sync_setup_handler.cc
+++ b/chrome/browser/ui/webui/sync_setup_handler.cc
@@ -1150,6 +1150,14 @@ void SyncSetupHandler::HandleCloseTimeout(const ListValue* args) {
 }
 
 void SyncSetupHandler::CloseSyncSetup() {
+  // Stop a timer to handle timeout in waiting for checking network connection.
+  backend_start_timer_.reset();
+
+  // Clear the signin tracker before canceling sync setup, as it may incorrectly
+  // flag a signin failure.
+  bool was_signing_in = (signin_tracker_.get() != NULL);
+  signin_tracker_.reset();
+
   // TODO(atwilson): Move UMA tracking of signin events out of sync module.
   ProfileSyncService* sync_service = GetSyncService();
   if (IsActiveLogin()) {
@@ -1157,7 +1165,8 @@ void SyncSetupHandler::CloseSyncSetup() {
     // automatically closed due to an auth error.
     if (!sync_service || (!sync_service->HasSyncSetupCompleted() &&
         sync_service->GetAuthError().state() == GoogleServiceAuthError::NONE)) {
-      if (signin_tracker_.get()) {
+      if (was_signing_in) {
+        // TODO(rsimha): Remove this. Sync should not be logging sign in events.
         ProfileSyncService::SyncEvent(
             ProfileSyncService::CANCEL_DURING_SIGNON);
       } else if (configuring_sync_) {
@@ -1203,10 +1212,6 @@ void SyncSetupHandler::CloseSyncSetup() {
 #endif
 
   configuring_sync_ = false;
-  signin_tracker_.reset();
-
-  // Stop a timer to handle timeout in waiting for checking network connection.
-  backend_start_timer_.reset();
 }
 
 void SyncSetupHandler::OpenSyncSetup() {
@@ -1233,8 +1238,6 @@ void SyncSetupHandler::OpenSyncSetup() {
     // User is not logged in, or login has been specially requested - need to
     // display login UI (cases 1-3).
     DisplayGaiaLogin(false);
-    if (!SyncPromoUI::UseWebBasedSigninFlow())
-      ShowSetupUI();
     return;
   }
 #endif
@@ -1249,7 +1252,6 @@ void SyncSetupHandler::OpenSyncSetup() {
   // via the "Advanced..." button or through One-Click signin (cases 4-6), or
   // they are re-enabling sync after having disabled it (case 7).
   DisplayConfigureSync(true, false);
-  ShowSetupUI();
 }
 
 void SyncSetupHandler::OpenConfigureSync() {
@@ -1257,7 +1259,6 @@ void SyncSetupHandler::OpenConfigureSync() {
     return;
 
   DisplayConfigureSync(true, false);
-  ShowSetupUI();
 }
 
 void SyncSetupHandler::FocusUI() {
diff --git a/chrome/browser/ui/webui/sync_setup_handler.h b/chrome/browser/ui/webui/sync_setup_handler.h
index 80ea45a..cbd3eef 100644
--- a/chrome/browser/ui/webui/sync_setup_handler.h
+++ b/chrome/browser/ui/webui/sync_setup_handler.h
@@ -97,10 +97,6 @@ class SyncSetupHandler : public options::OptionsPageUIHandler,
   bool is_configuring_sync() const { return configuring_sync_; }
   bool have_signin_tracker() const { return signin_tracker_; }
 
-  // Subclasses must implement this to show the setup UI that's appropriate
-  // for the page this is contained in.
-  virtual void ShowSetupUI() = 0;
-
   // Overridden by subclasses (like SyncPromoHandler) to log stats about the
   // user's signin activity.
   virtual void RecordSignin();
diff --git a/chrome/browser/ui/webui/sync_setup_handler_unittest.cc b/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
index 1099024..b375e50 100644
--- a/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
+++ b/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
@@ -309,7 +309,6 @@ class TestingSyncSetupHandler : public SyncSetupHandler {
     set_web_ui(NULL);
   }
 
-  virtual void ShowSetupUI() OVERRIDE {}
   virtual void FocusUI() OVERRIDE {}
 
   virtual Profile* GetProfile() const OVERRIDE { return profile_; }
diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi
index 899ea9b..e4277d3 100644
--- a/chrome/chrome_browser_ui.gypi
+++ b/chrome/chrome_browser_ui.gypi
@@ -2221,8 +2221,6 @@
         'browser/ui/webui/options/media_devices_selection_handler.h',
         'browser/ui/webui/options/media_galleries_handler.cc',
         'browser/ui/webui/options/media_galleries_handler.h',
-        'browser/ui/webui/options/options_sync_setup_handler.cc',
-        'browser/ui/webui/options/options_sync_setup_handler.h',
         'browser/ui/webui/options/options_ui.cc',
         'browser/ui/webui/options/options_ui.h',
         'browser/ui/webui/options/password_manager_handler.cc',