Start moving signin code out of browser/sync.

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 20d6572c88a152dae4642585272d8ba38433488b..621a01708399bffb9d5c534f727368d3b7c61a31 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -48,6 +48,8 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error_service.h"
 #include "chrome/browser/ui/global_error_service_factory.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
@@ -157,6 +159,13 @@ bool ProfileSyncService::AreCredentialsAvailable(
     return false;
   }
 
+  // If we have start suppressed, then basically just act like we have no
+  // credentials (login is required to fix this, since we need the user's
+  // passphrase to encrypt/decrypt anyway).
+  // TODO(sync): Revisit this when we move to a server-based keystore.
+  if (sync_prefs_.IsStartSuppressed())
+    return false;
+
   // CrOS user is always logged in. Chrome uses signin_ to check logged in.
   if (signin_->GetAuthenticatedUsername().empty())
     return false;
@@ -718,21 +727,14 @@ void ProfileSyncService::OnDataTypesChanged(
 
 void ProfileSyncService::UpdateAuthErrorState(
     const GoogleServiceAuthError& error) {
+  is_auth_in_progress_ = false;
   last_auth_error_ = error;
-  // Protect against the in-your-face dialogs that pop out of nowhere.
-  // Require the user to click somewhere to run the setup wizard in the case
-  // of a steady-state auth failure.
-  if (WizardIsVisible()) {
-    wizard_.Step(last_auth_error_.state() == AuthError::NONE ?
-        SyncSetupWizard::GAIA_SUCCESS : SyncSetupWizard::GetLoginState());
-  } else {
-    auth_error_time_ = base::TimeTicks::Now();
-  }
 
-  if (!auth_start_time_.is_null()) {
-    UMA_HISTOGRAM_TIMES("Sync.AuthorizationTimeInNetwork",
-                    base::TimeTicks::Now() - auth_start_time_);
-    auth_start_time_ = base::TimeTicks();
+  if (WizardIsVisible() && last_auth_error_.state() != AuthError::NONE) {
+    // Got some kind of auth error while the wizard is visible. Exit out of

[tim (not reviewing), 2012/02/14 02:11:21]

err... why is this fatal?  what happens to any common GSAE like invalid gaia?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

Remember that this is dealing with a GAIA error *after* login, while the config
screen is visible (not during signin). But, yeah, I guess nothing bad happens if
we leave the config screen up, so I'll remove this (the case of
SERVICE_UNAVAILABLE due to StopSyncingPermanently is already handled below).

+    // the config flow (UI handler can notify the user or bring up the signin
+    // flow as appropriate).
+    wizard_.Step(SyncSetupWizard::FATAL_ERROR);
   }
 
   // Fan the notification out to interested UI-thread components.
@@ -744,8 +746,11 @@ void ProfileSyncService::OnAuthError() {
 }
 
 void ProfileSyncService::OnStopSyncingPermanently() {
+  UpdateAuthErrorState(
+      GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
+
   if (SetupInProgress()) {

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

BTW, I'm changing this to abort whenever the wizard is visible. If we've stopped
syncing, it doesn't matter if this is initial setup or not, if the wizard is up
we need to close it.

-    wizard_.Step(SyncSetupWizard::SETUP_ABORTED_BY_PENDING_CLEAR);
+    wizard_.Step(SyncSetupWizard::ABORT);
     expect_sync_configuration_aborted_ = true;
   }
   sync_prefs_.SetStartSuppressed(true);
@@ -945,23 +950,9 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
 }
 
 void ProfileSyncService::ShowLoginDialog() {
-  if (WizardIsVisible()) {
-    wizard_.Focus();
-    // Force the wizard to step to the login screen (which will only actually
-    // happen if the transition is valid).
-    wizard_.Step(SyncSetupWizard::GetLoginState());
-    return;
-  }
-
-  if (!auth_error_time_.is_null()) {
-    UMA_HISTOGRAM_LONG_TIMES("Sync.ReauthorizationTime",
-                             base::TimeTicks::Now() - auth_error_time_);
-    auth_error_time_ = base::TimeTicks();  // Reset auth_error_time_ to null.
-  }
-
-  ShowSyncSetupWithWizard(SyncSetupWizard::GetLoginState());
-
-  NotifyObservers();
+  // TODO(atwilson): Remove this API and have the callers directly call
+  // LoginUIService.
+  LoginUIServiceFactory::GetForProfile(profile_)->ShowLoginUI();
 }
 
 void ProfileSyncService::ShowErrorUI() {
@@ -970,12 +961,21 @@ void ProfileSyncService::ShowErrorUI() {
     return;
   }
 
-  if (last_auth_error_.state() != AuthError::NONE)
+  // Figure out what kind of error we've encountered. There are only 3 kinds:
+  // 1) auth error.
+  // 2) server-initiated error
+  // 3) passphrase error
+  // Any other errors (such as unrecoverable error) should be handled by the UI
+  // itself and should not result in a call to ShowErrorUI.

[tim (not reviewing), 2012/02/14 02:11:21]

hm. so what about a failure to initialize / load the database in sync_manager?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

This change just makes explicit what was previously hidden away in
SyncSetupWizard::GetStepForNonFatalError(). That code assumed that either you
have passphrase required, or else an auth error, anything else hit a NOTREACHED
- so this shouldn't be a change in functionality, it's just clearer now.

The code in sync_ui_util.cc is dense and i'm still wrapping my head around it,
but I believe the intent is that unrecoverable errors like the type you
described don't result in calls to ShowErrorUI().

+  if (last_auth_error_.state() != AuthError::NONE) {
     ShowLoginDialog();
-  else if (ShouldShowActionOnUI(last_actionable_error_))
+  } else if (ShouldShowActionOnUI(last_actionable_error_)) {
     ShowSyncSetup(chrome::kPersonalOptionsSubPage);
-  else
-    ShowSyncSetupWithWizard(SyncSetupWizard::NONFATAL_ERROR);
+  } else {
+    // We should only get here for passphrase error.
+    DCHECK(IsPassphraseRequired());
+    ShowSyncSetupWithWizard(SyncSetupWizard::ENTER_PASSPHRASE);
+  }
 }
 
 void ProfileSyncService::ShowConfigure(bool sync_everything) {
@@ -1061,18 +1061,7 @@ bool ProfileSyncService::unrecoverable_error_detected() const {
 }
 
 bool ProfileSyncService::UIShouldDepictAuthInProgress() const {
-  return is_auth_in_progress_;
-}
-
-void ProfileSyncService::SetUIShouldDepictAuthInProgress(
-    bool auth_in_progress) {
-  is_auth_in_progress_ = auth_in_progress;
-  // TODO(atwilson): Figure out if we still need to track this or if we should
-  // move this up to the UI (or break it out into two stats that track GAIA
-  // auth and sync auth separately).
-  if (is_auth_in_progress_)
-    auth_start_time_ = base::TimeTicks::Now();
-  NotifyObservers();
+  return signin()->AuthInProgress();
 }
 
 bool ProfileSyncService::IsPassphraseRequired() const {
@@ -1449,6 +1438,7 @@ void ProfileSyncService::Observe(int type,
               details).ptr();
       // The user has submitted credentials, which indicates they don't
       // want to suppress start up anymore.
+      // TODO(sync): Remove this when sync is no longer tied to browser signin.

[tim (not reviewing), 2012/02/14 02:11:21]

should tie this to a bug (and mention in the bug to grep for the bug # before
closing)

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

Done.

       sync_prefs_.SetStartSuppressed(false);
 
       // Because we specify IMPLICIT to SetPassphrase, we know it won't override
@@ -1457,6 +1447,12 @@ void ProfileSyncService::Observe(int type,
       // an explicit passphrase set so this becomes a no-op.
       if (!successful->password.empty())
         SetPassphrase(successful->password, IMPLICIT, INTERNAL);
+
+      if (!sync_initialized() ||

[tim (not reviewing), 2012/02/14 02:11:21]

Hm, so there's no way we can call UpdateAuthErrorState here instead, is there?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

We aren't actually changing the state - all we're doing is setting
is_auth_in_progress if we decide we need to wait for auth to complete.

+          GetAuthError().state() != GoogleServiceAuthError::NONE) {
+        // Track the fact that we're still waiting for auth to complete.
+        is_auth_in_progress_ = true;
+      }
       break;
     }
     case chrome::NOTIFICATION_TOKEN_REQUEST_FAILED: {