Cleanup: Get rid of ProfileSyncService::waiting_for_auth()

Its semantics were a bit weird: It got set on initial sign-in (i.e. not
during a Chrome restart), and cleared again once we updated the auth
error state: Either to an error, if we failed to get an access token or
got an error from the sync server, or to "no error", if we successfully
connected to the sync server.
It had only a single call site, which only used it to suppress auth
errors while waiting_for_auth was true. However, according to the
above, that's an impossible state: Sync can not be in an auth error
state *before* being signed in, and the waiting_for_auth bit is cleared
when actually entering an auth error state.

Bug: 839834
Change-Id: Iefbf54a93ede54aabb2a244b3e429b5591bb9934
Reviewed-on: https://chromium-review.googlesource.com/1099162Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567250}

chrome/browser/sync/sync_startup_tracker.cc

@@ -57,9 +57,8 @@ SyncStartupTracker::SyncServiceState SyncStartupTracker::GetSyncServiceState(
       ProfileSyncServiceFactory::GetForProfile(profile);
 
   // If no service exists or it can't be started, treat as a startup error.
-  if (!service || !service->CanSyncStart()) {
+  if (!service || !service->CanSyncStart())
     return SYNC_STARTUP_ERROR;
-  }
 
   // If the sync engine has started up, notify the callback.
   if (service->IsEngineInitialized())
@@ -69,12 +68,9 @@ SyncStartupTracker::SyncServiceState SyncStartupTracker::GetSyncServiceState(
   if (service->HasUnrecoverableError())
     return SYNC_STARTUP_ERROR;
 
-  // If we have an auth error and sync is not still waiting for new auth tokens
-  // to be fetched, exit.
-  if (!service->waiting_for_auth() &&
-      service->GetAuthError().state() != GoogleServiceAuthError::NONE) {
+  // If we have an auth error, exit.
+  if (service->GetAuthError().state() != GoogleServiceAuthError::NONE)
     return SYNC_STARTUP_ERROR;
-  }
 
   // No error detected yet, but the sync engine hasn't started up yet, so
   // we're in the pending state.

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc

@@ -37,11 +37,13 @@
 #include "components/signin/core/browser/signin_metrics.h"
 #include "components/signin/core/browser/signin_pref_names.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "google_apis/gaia/google_service_auth_error.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::AtLeast;
 using ::testing::Return;
+using ::testing::ReturnRef;
 
 class DiceTurnSyncOnHelperTest;
 
@@ -245,27 +247,20 @@ class DiceTurnSyncOnHelperTest : public testing::Test {
     browser_sync::ProfileSyncServiceMock* sync_service_mock =
         GetProfileSyncServiceMock();
     EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
-    EXPECT_CALL(*sync_service_mock, CanSyncStart())
-        .Times(AtLeast(0))
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*sync_service_mock, IsEngineInitialized())
-        .Times(AtLeast(0))
-        .WillRepeatedly(Return(true));
+    ON_CALL(*sync_service_mock, CanSyncStart()).WillByDefault(Return(true));
+    ON_CALL(*sync_service_mock, IsEngineInitialized())
+        .WillByDefault(Return(true));
   }
 
   void SetExpectationsForSyncStartupPending() {
     browser_sync::ProfileSyncServiceMock* sync_service_mock =
         GetProfileSyncServiceMock();
     EXPECT_CALL(*sync_service_mock, GetSetupInProgressHandle()).Times(1);
-    EXPECT_CALL(*sync_service_mock, CanSyncStart())
-        .Times(AtLeast(0))
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*sync_service_mock, IsEngineInitialized())
-        .Times(AtLeast(0))
-        .WillRepeatedly(Return(false));
-    EXPECT_CALL(*sync_service_mock, waiting_for_auth())
-        .Times(AtLeast(0))
-        .WillRepeatedly(Return(true));
+    ON_CALL(*sync_service_mock, CanSyncStart()).WillByDefault(Return(true));
+    ON_CALL(*sync_service_mock, IsEngineInitialized())
+        .WillByDefault(Return(false));
+    ON_CALL(*sync_service_mock, GetAuthError())
+        .WillByDefault(ReturnRef(kNoAuthError));
   }
 
   void CheckDelegateCalls() {
@@ -388,6 +383,10 @@ class DiceTurnSyncOnHelperTest : public testing::Test {
   std::string new_profile_username_;
   bool sync_confirmation_shown_ = false;
   bool sync_settings_shown_ = false;
+
+  // Note: This needs to be a member variable for testing::ReturnRef.
+  const GoogleServiceAuthError kNoAuthError =
+      GoogleServiceAuthError::AuthErrorNone();
 };
 
 // TestDiceTurnSyncOnHelperDelegate implementation.

components/browser_sync/profile_sync_service.cc

@@ -1307,11 +1307,6 @@ bool ProfileSyncService::ConfigurationDone() const {
          data_type_manager_->state() == DataTypeManager::CONFIGURED;
 }
 
-bool ProfileSyncService::waiting_for_auth() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return auth_manager_->IsAuthInProgress();
-}
-
 bool ProfileSyncService::HasUnrecoverableError() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return unrecoverable_error_reason_ != ERROR_REASON_UNSET;

components/browser_sync/profile_sync_service.h

@@ -476,13 +476,6 @@ class ProfileSyncService : public syncer::SyncService,
   // TODO(sync): This is only used in tests.  Can we remove it?
   const syncer::DataTypeStatusTable& data_type_status_table() const;
 
-  // If true, the ProfileSyncService has detected that a new GAIA signin has
-  // succeeded, and is waiting for initialization to complete. This is used by
-  // the UI to differentiate between a new auth error (encountered as part of
-  // the initialization process) and a pre-existing auth error that just hasn't
-  // been cleared yet. Virtual for testing purposes.
-  virtual bool waiting_for_auth() const;
-
   // KeyedService implementation.  This must be called exactly
   // once (before this object is destroyed).
   void Shutdown() override;

components/browser_sync/profile_sync_service_mock.h

@@ -81,7 +81,6 @@ class ProfileSyncServiceMock : public ProfileSyncService {
   MOCK_CONST_METHOD0(IsEngineInitialized, bool());
   MOCK_CONST_METHOD0(IsSyncRequested, bool());
   MOCK_CONST_METHOD0(IsSyncConfirmationNeeded, bool());
-  MOCK_CONST_METHOD0(waiting_for_auth, bool());
   MOCK_METHOD1(OnActionableError, void(const syncer::SyncProtocolError&));
   MOCK_CONST_METHOD1(IsDataTypeControllerRunning, bool(syncer::ModelType));
 

components/browser_sync/sync_auth_manager.cc

@@ -60,7 +60,6 @@ SyncAuthManager::SyncAuthManager(
       account_state_changed_callback_(account_state_changed),
       credentials_changed_callback_(credentials_changed),
       registered_for_auth_notifications_(false),
-      is_auth_in_progress_(false),
       request_access_token_backoff_(&kRequestAccessTokenBackoffPolicy),
       weak_ptr_factory_(this) {
   DCHECK(sync_prefs_);
@@ -169,7 +168,6 @@ void SyncAuthManager::ConnectionStatusChanged(syncer::ConnectionStatus status) {
 
 void SyncAuthManager::UpdateAuthErrorState(
     const GoogleServiceAuthError& error) {
-  is_auth_in_progress_ = false;
   last_auth_error_ = error;
 }
 
@@ -192,10 +190,6 @@ void SyncAuthManager::Clear() {
 
 void SyncAuthManager::OnPrimaryAccountSet(
     const AccountInfo& primary_account_info) {
-  // Track the fact that we're still waiting for auth to complete.
-  DCHECK(!is_auth_in_progress_);
-  is_auth_in_progress_ = true;
-
   account_state_changed_callback_.Run();
 }
 
@@ -203,7 +197,6 @@ void SyncAuthManager::OnPrimaryAccountCleared(
     const AccountInfo& previous_primary_account_info) {
   UMA_HISTOGRAM_ENUMERATION("Sync.StopSource", syncer::SIGN_OUT,
                             syncer::STOP_SOURCE_LIMIT);
-  is_auth_in_progress_ = false;
   account_state_changed_callback_.Run();
 }
 

components/browser_sync/sync_auth_manager.h

@@ -62,7 +62,6 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
   const GoogleServiceAuthError& GetLastAuthError() const {
     return last_auth_error_;
   }
-  bool IsAuthInProgress() const { return is_auth_in_progress_; }
 
   // Returns the credentials to be passed to the SyncEngine.
   syncer::SyncCredentials GetCredentials() const;
@@ -117,10 +116,6 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
   // from the sync server or from Chrome's identity/token management system.
   GoogleServiceAuthError last_auth_error_;
 
-  // Set to true if a signin has completed but we're still waiting for the
-  // engine to refresh its credentials.
-  bool is_auth_in_progress_;
-
   // The current access token. This is mutually exclusive with
   // |ongoing_access_token_fetch_| and |request_access_token_retry_timer_|:
   // We either have an access token OR a pending request OR a pending retry.