[sync] Clean-up IsWebSignout() calls post StopSyncInPausedState launch

1) Before this CL, some ModelTypeControllers called IsWebSignout() in
their GetPreconditionState() implementation to make sure the data type
was stopped upon web sign-out. This is no longer needed following the
StopSyncInPausedState launch (crbug.com/906995), so this CL removes such
checks.
If GetPreconditionState() makes no other checks, the controller also no
longer needs to observe the SyncService. Note this isn't true for
controllers that check for persistent auth errors, since that concept is
more general than web sign-out. For those, only TODOs/includes are
removed.

2) The CL also addresses one IsWebSignout() TODO in PersonalDataManager.
Contrary to what the TODO implies, just removing the call isn't enough.
IsSyncFeatureEnabled() returns true in the sync-paused state, so the
check for TransportState::PAUSED must be moved up.

3) Finally, the CL updates the last TODO pointing to crbug.com/906995
to point to a newly filed bug. This concludes the post-launch cleanup
for StopSyncInPausedState.

Bug: 1140447
Change-Id: I996a8871050f8f18e1509a5560ca0ccfc849f217
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505772Reviewed-by: sebsg <sebsg@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#836062}

chrome/browser/sharing/sharing_message_model_type_controller.cc

@@ -6,8 +6,6 @@
 
 #include <utility>
 
-#include "base/feature_list.h"
-#include "components/sync/driver/sync_auth_util.h"
 #include "components/sync/driver/sync_service.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 
@@ -21,8 +19,6 @@ SharingMessageModelTypeController::SharingMessageModelTypeController(
                                   std::move(delegate_for_full_sync_mode),
                                   std::move(delegate_for_transport_mode)),
       sync_service_(sync_service) {
-  // TODO(crbug.com/906995): Remove this observing mechanism once all sync
-  // datatypes are stopped by ProfileSyncService, when sync is paused.
   sync_service_->AddObserver(this);
 }
 
@@ -33,10 +29,9 @@ SharingMessageModelTypeController::~SharingMessageModelTypeController() {
 syncer::DataTypeController::PreconditionState
 SharingMessageModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
-  if (sync_service_->GetAuthError().IsPersistentError()) {
-    return PreconditionState::kMustStopAndClearData;
-  }
-  return PreconditionState::kPreconditionsMet;
+  return sync_service_->GetAuthError().IsPersistentError()
+             ? PreconditionState::kMustStopAndClearData
+             : PreconditionState::kPreconditionsMet;
 }
 
 void SharingMessageModelTypeController::OnStateChanged(

components/autofill/core/browser/payments/autofill_wallet_model_type_controller.cc

@@ -13,7 +13,6 @@
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/autofill/core/common/autofill_prefs.h"
 #include "components/prefs/pref_service.h"
-#include "components/sync/driver/sync_auth_util.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
@@ -34,8 +33,6 @@ AutofillWalletModelTypeController::AutofillWalletModelTypeController(
          type == syncer::AUTOFILL_WALLET_METADATA ||
          type == syncer::AUTOFILL_WALLET_OFFER);
   SubscribeToPrefChanges();
-  // TODO(crbug.com/906995): remove this observing mechanism once all sync
-  // datatypes are stopped by ProfileSyncService, when sync is paused.
   sync_service_->AddObserver(this);
 }
 
@@ -56,8 +53,6 @@ AutofillWalletModelTypeController::AutofillWalletModelTypeController(
          type == syncer::AUTOFILL_WALLET_METADATA ||
          type == syncer::AUTOFILL_WALLET_OFFER);
   SubscribeToPrefChanges();
-  // TODO(crbug.com/906995): remove this observing mechanism once all sync
-  // datatypes are stopped by ProfileSyncService, when sync is paused.
   sync_service_->AddObserver(this);
 }
 
@@ -85,8 +80,6 @@ void AutofillWalletModelTypeController::Stop(
 syncer::DataTypeController::PreconditionState
 AutofillWalletModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
-  // Not being in a persistent error state implies not being in a web signout
-  // state.
   // TODO(https://crbug.com/819729): Add integration tests for web signout and
   // other persistent auth errors.
   bool preconditions_met =

components/autofill/core/browser/personal_data_manager.cc

@@ -594,21 +594,16 @@ AutofillSyncSigninState PersonalDataManager::GetSyncSigninState() const {
     return AutofillSyncSigninState::kSignedOut;
   }
 
-  // Check if the user has turned on sync.
-  if (sync_service_->IsSyncFeatureEnabled()) {
-    // TODO(crbug.com/906995): Remove this once the kStopSyncInPausedState
-    // feature is launched.
-    if (syncer::IsWebSignout(sync_service_->GetAuthError())) {
-      return AutofillSyncSigninState::kSyncPaused;
-    }
-    return AutofillSyncSigninState::kSignedInAndSyncFeatureEnabled;
-  }
-
   if (sync_service_->GetTransportState() ==
       syncer::SyncService::TransportState::PAUSED) {
     return AutofillSyncSigninState::kSyncPaused;
   }
 
+  // Check if the user has turned on sync.
+  if (sync_service_->IsSyncFeatureEnabled()) {
+    return AutofillSyncSigninState::kSignedInAndSyncFeatureEnabled;
+  }
+
   // Check if the feature is enabled and if Wallet data types are supported.
   if (base::FeatureList::IsEnabled(
           features::kAutofillEnableAccountWalletStorage) &&

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -371,7 +371,6 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
             .get();
     controllers.push_back(
         std::make_unique<send_tab_to_self::SendTabToSelfModelTypeController>(
-            sync_service,
             /*delegate_for_full_sync_mode=*/
             std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
                 delegate),

components/send_tab_to_self/send_tab_to_self_model_type_controller.cc

@@ -8,33 +8,23 @@
 
 #include "base/feature_list.h"
 #include "components/send_tab_to_self/features.h"
-#include "components/sync/driver/sync_auth_util.h"
-#include "components/sync/driver/sync_service.h"
-#include "google_apis/gaia/google_service_auth_error.h"
 
 namespace send_tab_to_self {
 
 SendTabToSelfModelTypeController::SendTabToSelfModelTypeController(
-    syncer::SyncService* sync_service,
     std::unique_ptr<syncer::ModelTypeControllerDelegate>
         delegate_for_full_sync_mode,
     std::unique_ptr<syncer::ModelTypeControllerDelegate>
         delegate_for_transport_mode)
     : ModelTypeController(syncer::SEND_TAB_TO_SELF,
                           std::move(delegate_for_full_sync_mode),
-                          std::move(delegate_for_transport_mode)),
-      sync_service_(sync_service) {
+                          std::move(delegate_for_transport_mode)) {
   DCHECK_EQ(base::FeatureList::IsEnabled(
                 send_tab_to_self::kSendTabToSelfWhenSignedIn),
             ShouldRunInTransportOnlyMode());
-  // TODO(crbug.com/906995): Remove this observing mechanism once all sync
-  // datatypes are stopped by ProfileSyncService, when sync is paused.
-  sync_service_->AddObserver(this);
 }
 
-SendTabToSelfModelTypeController::~SendTabToSelfModelTypeController() {
-  sync_service_->RemoveObserver(this);
-}
+SendTabToSelfModelTypeController::~SendTabToSelfModelTypeController() = default;
 
 void SendTabToSelfModelTypeController::Stop(
     syncer::ShutdownReason shutdown_reason,
@@ -55,19 +45,4 @@ void SendTabToSelfModelTypeController::Stop(
   ModelTypeController::Stop(shutdown_reason, std::move(callback));
 }
 
-syncer::DataTypeController::PreconditionState
-SendTabToSelfModelTypeController::GetPreconditionState() const {
-  DCHECK(CalledOnValidThread());
-  return syncer::IsWebSignout(sync_service_->GetAuthError())
-             ? PreconditionState::kMustStopAndClearData
-             : PreconditionState::kPreconditionsMet;
-}
-
-void SendTabToSelfModelTypeController::OnStateChanged(
-    syncer::SyncService* sync) {
-  DCHECK(CalledOnValidThread());
-  // Most of these calls will be no-ops but SyncService handles that just fine.
-  sync_service_->DataTypePreconditionChanged(type());
-}
-
 }  // namespace send_tab_to_self

components/send_tab_to_self/send_tab_to_self_model_type_controller.h

@@ -7,23 +7,15 @@
 
 #include "base/macros.h"
 #include "components/sync/driver/model_type_controller.h"
-#include "components/sync/driver/sync_service_observer.h"
-
-namespace syncer {
-class SyncService;
-}  // namespace syncer
 
 namespace send_tab_to_self {
 
 // Controls syncing of SEND_TAB_TO_SELF.
-class SendTabToSelfModelTypeController : public syncer::ModelTypeController,
-                                         public syncer::SyncServiceObserver {
+class SendTabToSelfModelTypeController : public syncer::ModelTypeController {
  public:
-  // The |delegate_for_full_sync_mode| and |sync_service| must not be null.
-  // |delegate_for_transport_mode| can be null. |sync_service| must outlive this
-  // object.
+  // |delegate_for_full_sync_mode| must not be null.
+  // |delegate_for_transport_mode| can be null.
   SendTabToSelfModelTypeController(
-      syncer::SyncService* sync_service,
       std::unique_ptr<syncer::ModelTypeControllerDelegate>
           delegate_for_full_sync_mode,
       std::unique_ptr<syncer::ModelTypeControllerDelegate>
@@ -34,15 +26,7 @@ class SendTabToSelfModelTypeController : public syncer::ModelTypeController,
   void Stop(syncer::ShutdownReason shutdown_reason,
             StopCallback callback) override;
 
-  // DataTypeController overrides.
-  PreconditionState GetPreconditionState() const override;
-
-  // syncer::SyncServiceObserver implementation.
-  void OnStateChanged(syncer::SyncService* sync) override;
-
  private:
-  syncer::SyncService* const sync_service_;
-
   DISALLOW_COPY_AND_ASSIGN(SendTabToSelfModelTypeController);
 };
 

components/sync/driver/sync_auth_manager.cc

@@ -370,7 +370,7 @@ void SyncAuthManager::OnRefreshTokenRemovedForAccount(
 
   // If we're still here, then that means Chrome is still signed in to this
   // account. Keep Sync alive but set an auth error.
-  // TODO(crbug.com/906995): Should we stop Sync in this case?
+  // TODO(crbug.com/1156584): Should we stop Sync in this case?
   DCHECK_EQ(sync_account_.account_info.account_id,
             identity_manager_->GetPrimaryAccountId());
 

components/sync_sessions/session_model_type_controller.cc

@@ -7,19 +7,10 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/feature_list.h"
 #include "components/prefs/pref_service.h"
-#include "components/sync/driver/sync_auth_util.h"
 #include "components/sync/driver/sync_service.h"
-#include "google_apis/gaia/google_service_auth_error.h"
 
 namespace sync_sessions {
-namespace {
-
-const base::Feature kStopSessionsIfSyncPaused{"StopSessionsIfSyncPaused",
-                                              base::FEATURE_ENABLED_BY_DEFAULT};
-
-}  // namespace
 
 SessionModelTypeController::SessionModelTypeController(
     syncer::SyncService* sync_service,
@@ -30,9 +21,6 @@ SessionModelTypeController::SessionModelTypeController(
       sync_service_(sync_service),
       pref_service_(pref_service),
       history_disabled_pref_name_(history_disabled_pref_name) {
-  // TODO(crbug.com/906995): remove this observing mechanism once all sync
-  // datatypes are stopped by ProfileSyncService, when sync is paused.
-  sync_service_->AddObserver(this);
   pref_registrar_.Init(pref_service);
   pref_registrar_.Add(
       history_disabled_pref_name_,
@@ -41,25 +29,14 @@ SessionModelTypeController::SessionModelTypeController(
           base::AsWeakPtr(this)));
 }
 
-SessionModelTypeController::~SessionModelTypeController() {
-  sync_service_->RemoveObserver(this);
-}
+SessionModelTypeController::~SessionModelTypeController() = default;
 
 syncer::DataTypeController::PreconditionState
 SessionModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
-  bool preconditions_met =
-      !pref_service_->GetBoolean(history_disabled_pref_name_) &&
-      !(syncer::IsWebSignout(sync_service_->GetAuthError()) &&
-        base::FeatureList::IsEnabled(kStopSessionsIfSyncPaused));
-  return preconditions_met ? PreconditionState::kPreconditionsMet
-                           : PreconditionState::kMustStopAndKeepData;
-}
-
-void SessionModelTypeController::OnStateChanged(syncer::SyncService* sync) {
-  DCHECK(CalledOnValidThread());
-  // Most of these calls will be no-ops but SyncService handles that just fine.
-  sync_service_->DataTypePreconditionChanged(type());
+  return pref_service_->GetBoolean(history_disabled_pref_name_)
+             ? PreconditionState::kMustStopAndKeepData
+             : PreconditionState::kPreconditionsMet;
 }
 
 void SessionModelTypeController::OnSavingBrowserHistoryPrefChanged() {

components/sync_sessions/session_model_type_controller.h

@@ -11,7 +11,6 @@
 #include "base/macros.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/sync/driver/model_type_controller.h"
-#include "components/sync/driver/sync_service_observer.h"
 
 class PrefService;
 
@@ -22,8 +21,7 @@ class SyncService;
 namespace sync_sessions {
 
 // Overrides LoadModels to check if history sync is allowed by policy.
-class SessionModelTypeController : public syncer::ModelTypeController,
-                                   public syncer::SyncServiceObserver {
+class SessionModelTypeController : public syncer::ModelTypeController {
  public:
   SessionModelTypeController(
       syncer::SyncService* sync_service,
@@ -35,9 +33,6 @@ class SessionModelTypeController : public syncer::ModelTypeController,
   // DataTypeController overrides.
   PreconditionState GetPreconditionState() const override;
 
-  // syncer::SyncServiceObserver implementation.
-  void OnStateChanged(syncer::SyncService* sync) override;
-
  private:
   void OnSavingBrowserHistoryPrefChanged();
 

components/sync_user_events/DEPS

@@ -5,5 +5,4 @@ include_rules = [
   "+components/sync/model",
   "+components/sync/protocol",
   "+components/sync/test/model",
-  "+google_apis/gaia",
 ]

components/sync_user_events/user_event_model_type_controller.cc

@@ -7,10 +7,8 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "components/sync/driver/sync_auth_util.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
-#include "google_apis/gaia/google_service_auth_error.h"
 
 namespace syncer {
 
@@ -46,16 +44,9 @@ void UserEventModelTypeController::Stop(syncer::ShutdownReason shutdown_reason,
 
 DataTypeController::PreconditionState
 UserEventModelTypeController::GetPreconditionState() const {
-  if (sync_service_->GetUserSettings()->IsUsingSecondaryPassphrase()) {
-    return PreconditionState::kMustStopAndClearData;
-  }
-  // TODO(crbug.com/938819): Remove the syncer::IsWebSignout() check once we
-  // stop sync in this state. Also remove the "+google_apis/gaia" include
-  // dependency and the "//google_apis" build dependency of sync_user_events.
-  if (syncer::IsWebSignout(sync_service_->GetAuthError())) {
-    return PreconditionState::kMustStopAndClearData;
-  }
-  return PreconditionState::kPreconditionsMet;
+  return sync_service_->GetUserSettings()->IsUsingSecondaryPassphrase()
+             ? PreconditionState::kMustStopAndClearData
+             : PreconditionState::kPreconditionsMet;
 }
 
 void UserEventModelTypeController::OnStateChanged(syncer::SyncService* sync) {