Cleanup: Remove syncer::prefs::kSyncHasAuthError

The only use was inside a single DCHECK in DiceHeaderHelper. That's not
worth keeping the pref and all the plumbing around.

Bug: 935956
Change-Id: Ic7d0f11751cb8ce6d09472296a87493b1848e57a
Reviewed-on: https://chromium-review.googlesource.com/c/1489191Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635532}

chrome/browser/prefs/browser_prefs.cc

@@ -971,4 +971,5 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
 
   // Added 2/2019.
   syncer::ClearObsoleteClearServerDataPrefs(profile_prefs);
+  syncer::ClearObsoleteAuthErrorPrefs(profile_prefs);
 }

chrome/browser/profiles/profile_io_data.cc

@@ -452,8 +452,6 @@ void ProfileIOData::InitializeOnUIThread(Profile* profile) {
     sync_first_setup_complete_.Init(syncer::prefs::kSyncFirstSetupComplete,
                                     pref_service);
     sync_first_setup_complete_.MoveToThread(io_task_runner);
-    sync_has_auth_error_.Init(syncer::prefs::kSyncHasAuthError, pref_service);
-    sync_has_auth_error_.MoveToThread(io_task_runner);
   }
 
 #if !defined(OS_CHROMEOS)
@@ -820,10 +818,6 @@ bool ProfileIOData::IsSyncEnabled() const {
          !sync_suppress_start_.GetValue();
 }
 
-bool ProfileIOData::SyncHasAuthError() const {
-  return sync_has_auth_error_.GetValue();
-}
-
 #if !defined(OS_CHROMEOS)
 std::string ProfileIOData::GetSigninScopedDeviceId() const {
   return signin_scoped_device_id_.GetValue();
@@ -1163,7 +1157,6 @@ void ProfileIOData::ShutdownOnUIThread(
   google_services_user_account_id_.Destroy();
   sync_suppress_start_.Destroy();
   sync_first_setup_complete_.Destroy();
-  sync_has_auth_error_.Destroy();
 #if !defined(OS_CHROMEOS)
   signin_scoped_device_id_.Destroy();
 #endif

chrome/browser/profiles/profile_io_data.h

@@ -151,7 +151,6 @@ class ProfileIOData {
 
   // Gets Sync state, for Dice account consistency.
   bool IsSyncEnabled() const;
-  bool SyncHasAuthError() const;
 
   BooleanPrefMember* safe_browsing_enabled() const {
     return &safe_browsing_enabled_;
@@ -496,7 +495,6 @@ class ProfileIOData {
       client_cert_store_factory_;
 
   mutable StringPrefMember google_services_user_account_id_;
-  mutable BooleanPrefMember sync_has_auth_error_;
   mutable BooleanPrefMember sync_suppress_start_;
   mutable BooleanPrefMember sync_first_setup_complete_;
   mutable signin::AccountConsistencyMethod account_consistency_;

chrome/browser/signin/chrome_signin_helper.cc

@@ -523,8 +523,8 @@ void FixAccountConsistencyRequestHeader(ChromeRequestAdapter* request,
   // Dice header:
   bool dice_header_added = AppendOrRemoveDiceRequestHeader(
       request, redirect_url, account_id, io_data->IsSyncEnabled(),
-      io_data->SyncHasAuthError(), account_consistency,
-      io_data->GetCookieSettings(), io_data->GetSigninScopedDeviceId());
+      account_consistency, io_data->GetCookieSettings(),
+      io_data->GetSigninScopedDeviceId());
 
   // Block the AccountReconcilor while the Dice requests are in flight. This
   // allows the DiceReponseHandler to process the response before the reconcilor

components/browser_sync/profile_sync_service.cc

@@ -158,7 +158,6 @@ ProfileSyncService::ProfileSyncService(InitParams init_params)
       sync_prefs_(sync_client_->GetPrefService()),
       identity_manager_(init_params.identity_manager),
       auth_manager_(std::make_unique<SyncAuthManager>(
-          &sync_prefs_,
           identity_manager_,
           base::BindRepeating(&ProfileSyncService::AccountStateChanged,
                               base::Unretained(this)),

components/browser_sync/sync_auth_manager.cc

@@ -53,18 +53,15 @@ constexpr net::BackoffEntry::Policy kRequestAccessTokenBackoffPolicy = {
 }  // namespace
 
 SyncAuthManager::SyncAuthManager(
-    syncer::SyncPrefs* sync_prefs,
     identity::IdentityManager* identity_manager,
     const AccountStateChangedCallback& account_state_changed,
     const CredentialsChangedCallback& credentials_changed)
-    : sync_prefs_(sync_prefs),
-      identity_manager_(identity_manager),
+    : identity_manager_(identity_manager),
       account_state_changed_callback_(account_state_changed),
       credentials_changed_callback_(credentials_changed),
       registered_for_auth_notifications_(false),
       request_access_token_backoff_(&kRequestAccessTokenBackoffPolicy),
       weak_ptr_factory_(this) {
-  DCHECK(sync_prefs_);
   // |identity_manager_| can be null if local Sync is enabled.
 }
 
@@ -426,7 +423,6 @@ void SyncAuthManager::AccessTokenFetched(
   switch (error.state()) {
     case GoogleServiceAuthError::NONE:
       partial_token_status_.token_receive_time = base::Time::Now();
-      sync_prefs_->SetSyncAuthError(false);
       last_auth_error_ = GoogleServiceAuthError::AuthErrorNone();
       break;
     case GoogleServiceAuthError::CONNECTION_FAILED:
@@ -441,7 +437,6 @@ void SyncAuthManager::AccessTokenFetched(
       ScheduleAccessTokenRequest();
       break;
     case GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS:
-      sync_prefs_->SetSyncAuthError(true);
       last_auth_error_ = error;
       break;
     default:

components/browser_sync/sync_auth_manager.h

@@ -27,7 +27,6 @@ class AccessTokenFetcher;
 
 namespace syncer {
 struct SyncCredentials;
-class SyncPrefs;
 }  // namespace syncer
 
 namespace browser_sync {
@@ -47,11 +46,9 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
   // added/changed/removed. Call GetCredentials to get the new state.
   using CredentialsChangedCallback = base::RepeatingClosure;
 
-  // |sync_prefs| must not be null and must outlive this.
   // |identity_manager| may be null (this is the case if local Sync is enabled),
   // but if non-null, must outlive this object.
-  SyncAuthManager(syncer::SyncPrefs* sync_prefs,
-                  identity::IdentityManager* identity_manager,
+  SyncAuthManager(identity::IdentityManager* identity_manager,
                   const AccountStateChangedCallback& account_state_changed,
                   const CredentialsChangedCallback& credentials_changed);
   ~SyncAuthManager() override;
@@ -135,7 +132,6 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
   void AccessTokenFetched(GoogleServiceAuthError error,
                           identity::AccessTokenInfo access_token_info);
 
-  syncer::SyncPrefs* const sync_prefs_;
   identity::IdentityManager* const identity_manager_;
 
   const AccountStateChangedCallback account_state_changed_callback_;

components/browser_sync/sync_auth_manager_unittest.cc

@@ -10,11 +10,9 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "build/build_config.h"
-#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/engine/connection_status.h"
 #include "components/sync/engine/sync_credentials.h"
-#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "net/base/net_errors.h"
 #include "services/identity/public/cpp/identity_test_environment.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -32,10 +30,7 @@ class SyncAuthManagerTest : public testing::Test {
   using CredentialsChangedCallback =
       SyncAuthManager::CredentialsChangedCallback;
 
-  SyncAuthManagerTest() : identity_env_(&test_url_loader_factory_) {
-    syncer::SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
-    sync_prefs_ = std::make_unique<syncer::SyncPrefs>(&pref_service_);
-  }
+  SyncAuthManagerTest() : identity_env_(&test_url_loader_factory_) {}
 
   ~SyncAuthManagerTest() override {}
 
@@ -46,14 +41,14 @@ class SyncAuthManagerTest : public testing::Test {
   std::unique_ptr<SyncAuthManager> CreateAuthManager(
       const AccountStateChangedCallback& account_state_changed,
       const CredentialsChangedCallback& credentials_changed) {
-    return std::make_unique<SyncAuthManager>(
-        sync_prefs_.get(), identity_env_.identity_manager(),
-        account_state_changed, credentials_changed);
+    return std::make_unique<SyncAuthManager>(identity_env_.identity_manager(),
+                                             account_state_changed,
+                                             credentials_changed);
   }
 
   std::unique_ptr<SyncAuthManager> CreateAuthManagerForLocalSync() {
-    return std::make_unique<SyncAuthManager>(
-        sync_prefs_.get(), nullptr, base::DoNothing(), base::DoNothing());
+    return std::make_unique<SyncAuthManager>(nullptr, base::DoNothing(),
+                                             base::DoNothing());
   }
 
   identity::IdentityTestEnvironment* identity_env() { return &identity_env_; }
@@ -62,8 +57,6 @@ class SyncAuthManagerTest : public testing::Test {
   base::test::ScopedTaskEnvironment task_environment_;
   network::TestURLLoaderFactory test_url_loader_factory_;
   identity::IdentityTestEnvironment identity_env_;
-  sync_preferences::TestingPrefServiceSyncable pref_service_;
-  std::unique_ptr<syncer::SyncPrefs> sync_prefs_;
 };
 
 TEST_F(SyncAuthManagerTest, ProvidesNothingInLocalSyncMode) {

components/signin/core/browser/dice_header_helper.cc

@@ -49,11 +49,8 @@ DiceAction GetDiceActionFromHeader(const std::string& value) {
 
 }  // namespace
 
-DiceHeaderHelper::DiceHeaderHelper(bool signed_in_with_auth_error,
-                                   AccountConsistencyMethod account_consistency)
-    : SigninHeaderHelper("Dice"),
-      signed_in_with_auth_error_(signed_in_with_auth_error),
-      account_consistency_(account_consistency) {}
+DiceHeaderHelper::DiceHeaderHelper(AccountConsistencyMethod account_consistency)
+    : SigninHeaderHelper("Dice"), account_consistency_(account_consistency) {}
 
 // static
 DiceResponseParams DiceHeaderHelper::BuildDiceSigninResponseParams(
@@ -199,8 +196,6 @@ bool DiceHeaderHelper::IsUrlEligibleForRequestHeader(const GURL& url) {
 std::string DiceHeaderHelper::BuildRequestHeader(
     const std::string& sync_account_id,
     const std::string& device_id) {
-  DCHECK(!(sync_account_id.empty() && signed_in_with_auth_error_));
-
   std::vector<std::string> parts;
   parts.push_back(base::StringPrintf("version=%s", kDiceProtocolVersion));
   parts.push_back("client_id=" +

components/signin/core/browser/dice_header_helper.h

@@ -21,8 +21,7 @@ extern const char kDiceProtocolVersion[];
 // SigninHeaderHelper implementation managing the Dice header.
 class DiceHeaderHelper : public SigninHeaderHelper {
  public:
-  explicit DiceHeaderHelper(bool signed_in_with_auth_error,
-                            AccountConsistencyMethod account_consistency);
+  explicit DiceHeaderHelper(AccountConsistencyMethod account_consistency);
   ~DiceHeaderHelper() override {}
 
   // Returns the parameters contained in the X-Chrome-ID-Consistency-Response
@@ -53,7 +52,6 @@ class DiceHeaderHelper : public SigninHeaderHelper {
   // SigninHeaderHelper implementation:
   bool IsUrlEligibleForRequestHeader(const GURL& url) override;
 
-  bool signed_in_with_auth_error_;
   AccountConsistencyMethod account_consistency_;
 
   DISALLOW_COPY_AND_ASSIGN(DiceHeaderHelper);

components/signin/core/browser/signin_header_helper.cc

@@ -188,15 +188,12 @@ bool AppendOrRemoveDiceRequestHeader(
     const GURL& redirect_url,
     const std::string& account_id,
     bool sync_enabled,
-    bool sync_has_auth_error,
     AccountConsistencyMethod account_consistency,
     const content_settings::CookieSettings* cookie_settings,
     const std::string& device_id) {
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
   const GURL& url = redirect_url.is_empty() ? request->GetUrl() : redirect_url;
-  DiceHeaderHelper dice_helper(
-      !account_id.empty() && sync_has_auth_error && sync_enabled,
-      account_consistency);
+  DiceHeaderHelper dice_helper(account_consistency);
   std::string dice_header_value;
   if (dice_helper.ShouldBuildRequestHeader(url, cookie_settings)) {
     dice_header_value = dice_helper.BuildRequestHeader(

components/signin/core/browser/signin_header_helper.h

@@ -235,7 +235,6 @@ bool AppendOrRemoveDiceRequestHeader(
     const GURL& redirect_url,
     const std::string& account_id,
     bool sync_enabled,
-    bool sync_has_auth_error,
     AccountConsistencyMethod account_consistency,
     const content_settings::CookieSettings* cookie_settings,
     const std::string& device_id);

components/signin/core/browser/signin_header_helper_unittest.cc

@@ -68,8 +68,7 @@ class SigninHeaderHelperTest : public testing::Test {
         &request_adapter, GURL(), account_id, account_consistency_,
         cookie_settings_.get(), PROFILE_MODE_DEFAULT);
     AppendOrRemoveDiceRequestHeader(&request_adapter, GURL(), account_id,
-                                    sync_enabled_, sync_has_auth_error_,
-                                    account_consistency_,
+                                    sync_enabled_, account_consistency_,
                                     cookie_settings_.get(), device_id_);
     return url_request;
   }
@@ -115,7 +114,6 @@ class SigninHeaderHelperTest : public testing::Test {
   base::test::ScopedTaskEnvironment task_environment_;
 
   bool sync_enabled_ = false;
-  bool sync_has_auth_error_ = false;
   std::string device_id_ = kTestDeviceId;
   AccountConsistencyMethod account_consistency_ =
       AccountConsistencyMethod::kDisabled;

components/sync/base/pref_names.cc

@@ -22,9 +22,6 @@ const char kSyncLongPollIntervalSeconds[] = "sync.long_poll_interval";
 // Boolean specifying whether the user finished setting up sync at least once.
 const char kSyncFirstSetupComplete[] = "sync.has_setup_completed";
 
-// Boolean specifying whether sync has an auth error.
-const char kSyncHasAuthError[] = "sync.has_auth_error";
-
 // Boolean specifying whether to automatically sync all data types (including
 // future ones, as they're added).  If this is true, the following preferences
 // (kSyncBookmarks, kSyncPasswords, etc.) can all be ignored.

components/sync/base/pref_names.h

@@ -15,7 +15,6 @@ extern const char kSyncLastSyncedTime[];
 extern const char kSyncLastPollTime[];
 extern const char kSyncShortPollIntervalSeconds[];
 extern const char kSyncLongPollIntervalSeconds[];
-extern const char kSyncHasAuthError[];
 extern const char kSyncFirstSetupComplete[];
 extern const char kSyncKeepEverythingSynced[];
 

components/sync/base/sync_prefs.cc

@@ -27,6 +27,9 @@ const char kSyncPassphraseEncryptionTransitionInProgress[] =
 const char kSyncNigoriStateForPassphraseTransition[] =
     "sync.nigori_state_for_passphrase_transition";
 
+// Obsolete pref that used to store a bool on whether Sync has an auth error.
+const char kSyncHasAuthError[] = "sync.has_auth_error";
+
 // Groups of prefs that always have the same value as a "master" pref.
 // For example, the APPS group has {APP_LIST, APP_SETTINGS}
 // (as well as APPS, but that is implied), so
@@ -172,7 +175,7 @@ void SyncPrefs::RegisterProfilePrefs(
   registry->RegisterStringPref(prefs::kSyncSpareBootstrapToken, "");
 #endif
 
-  registry->RegisterBooleanPref(prefs::kSyncHasAuthError, false);
+  registry->RegisterBooleanPref(kSyncHasAuthError, false);
   registry->RegisterBooleanPref(prefs::kSyncPassphrasePrompted, false);
   registry->RegisterIntegerPref(prefs::kSyncMemoryPressureWarningCount, -1);
   registry->RegisterBooleanPref(prefs::kSyncShutdownCleanly, false);
@@ -228,16 +231,6 @@ void SyncPrefs::SetFirstSetupComplete() {
   pref_service_->SetBoolean(prefs::kSyncFirstSetupComplete, true);
 }
 
-bool SyncPrefs::SyncHasAuthError() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return pref_service_->GetBoolean(prefs::kSyncHasAuthError);
-}
-
-void SyncPrefs::SetSyncAuthError(bool error) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  pref_service_->SetBoolean(prefs::kSyncHasAuthError, error);
-}
-
 bool SyncPrefs::IsSyncRequested() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // IsSyncRequested is the inverse of the old SuppressStart pref.
@@ -643,4 +636,8 @@ void ClearObsoleteClearServerDataPrefs(PrefService* pref_service) {
   pref_service->ClearPref(kSyncNigoriStateForPassphraseTransition);
 }
 
+void ClearObsoleteAuthErrorPrefs(PrefService* pref_service) {
+  pref_service->ClearPref(kSyncHasAuthError);
+}
+
 }  // namespace syncer

components/sync/base/sync_prefs.h

@@ -82,9 +82,6 @@ class SyncPrefs : public CryptoSyncPrefs,
   bool IsFirstSetupComplete() const;
   void SetFirstSetupComplete();
 
-  bool SyncHasAuthError() const;
-  void SetSyncAuthError(bool error);
-
   bool IsSyncRequested() const;
   void SetSyncRequested(bool is_requested);
 
@@ -239,6 +236,7 @@ class SyncPrefs : public CryptoSyncPrefs,
 void MigrateSessionsToProxyTabsPrefs(PrefService* pref_service);
 void ClearObsoleteUserTypePrefs(PrefService* pref_service);
 void ClearObsoleteClearServerDataPrefs(PrefService* pref_service);
+void ClearObsoleteAuthErrorPrefs(PrefService* pref_service);
 
 }  // namespace syncer