summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorrsimha@chromium.org <rsimha@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2013-06-05 09:23:27 +0000
committerrsimha@chromium.org <rsimha@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2013-06-05 09:23:27 +0000
commit439dce0e361ea60656fa91eeb84d50b19a405027 (patch)
treeb1ae0bfe3ed3b609495cad030e3b720ea4adb4ef
parent5cb80b934436dfb0bf9b810a7a9f45c9c9155f75 (diff)
downloadchromium_src-439dce0e361ea60656fa91eeb84d50b19a405027.zip
chromium_src-439dce0e361ea60656fa91eeb84d50b19a405027.tar.gz
chromium_src-439dce0e361ea60656fa91eeb84d50b19a405027.tar.bz2
[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
-rw-r--r--chrome/browser/signin/signin_tracker.h4
-rw-r--r--chrome/browser/sync/profile_sync_service.cc12
-rw-r--r--chrome/browser/ui/webui/options/options_sync_setup_handler.cc27
-rw-r--r--chrome/browser/ui/webui/options/options_sync_setup_handler.h25
-rw-r--r--chrome/browser/ui/webui/options/options_ui.cc4
-rw-r--r--chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc3
-rw-r--r--chrome/browser/ui/webui/sync_promo/sync_promo_handler.h2
-rw-r--r--chrome/browser/ui/webui/sync_setup_handler.cc19
-rw-r--r--chrome/browser/ui/webui/sync_setup_handler.h4
-rw-r--r--chrome/browser/ui/webui/sync_setup_handler_unittest.cc1
-rw-r--r--chrome/chrome_browser_ui.gypi2
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',