Remove sync suppression logic while local UPM migration is pending

Behavior changes are behind flag.
Partial revert of crrev.com/c/5088712. The suppression logic will
have to be implemented differently, remove for now. After this CL it is
possible to enable password sync while the migration is pending, but in
*transport* mode, without any upload of existing data.
This setup of overriding the SyncMode to kTransportOnly in the
controller can potentially be reused on Desktop in the future, if we
decide to always run passwords in transport mode ahead of fully
deprecating sync-the-feature. So gate it behind a new
kEnablePasswordsAccountStorageForSyncingUsers toggle. This toggle
will also be consumed by IsOptedInForAccountStorage() in future CLs.

Unrelated to that, also add a comment next to the usage of the
corresponding flag for non-syncing users.

Bug: 1509058
Change-Id: I70fe3497e46bb0f03611fa96844bc71eb6f3a300
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5262157
Auto-Submit: Victor Vianna <victorvianna@google.com>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1255656}

diff --git a/chrome/browser/password_manager/android/password_manager_android_util_unittest.cc b/chrome/browser/password_manager/android/password_manager_android_util_unittest.cc
index fd95c26..5bd4987c 100644
--- a/chrome/browser/password_manager/android/password_manager_android_util_unittest.cc
+++ b/chrome/browser/password_manager/android/password_manager_android_util_unittest.cc

@@ -399,7 +399,8 @@
     SignInAndEnableSync();
     ASSERT_TRUE(
         SyncDataTypeActiveWaiter(sync_service(), syncer::PREFERENCES).Wait());
-    ASSERT_FALSE(sync_service()->GetActiveDataTypes().Has(syncer::PASSWORDS));
+    // TODO(crbug.com/1509058): Re-implement sync suppression and uncomment.
+    // ASSERT_FALSE(sync_service()->GetActiveDataTypes().Has(syncer::PASSWORDS));
 
     // Pretend the migration finished.
     // TODO(crbug.com/1495626): Once the migration is implemented, make this a

diff --git a/components/password_manager/core/browser/sync/password_model_type_controller.cc b/components/password_manager/core/browser/sync/password_model_type_controller.cc
index 5fb7294e..5ca69c4 100644
--- a/components/password_manager/core/browser/sync/password_model_type_controller.cc
+++ b/components/password_manager/core/browser/sync/password_model_type_controller.cc

@@ -12,9 +12,6 @@
 #include "build/build_config.h"
 #include "components/password_manager/core/browser/features/password_features.h"
 #include "components/password_manager/core/browser/features/password_manager_features_util.h"
-#include "components/password_manager/core/common/password_manager_pref_names.h"
-#include "components/prefs/pref_change_registrar.h"
-#include "components/prefs/pref_service.h"
 #include "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h"
 #include "components/sync/base/features.h"
 #include "components/sync/base/model_type.h"
@@ -39,13 +36,6 @@
       identity_manager_(identity_manager),
       sync_service_(sync_service) {
   identity_manager_observation_.Observe(identity_manager_);
-#if BUILDFLAG(IS_ANDROID)
-  // Unretained() is safe because this object outlives `local_upm_pref_`.
-  local_upm_pref_.Init(
-      prefs::kPasswordsUseUPMLocalAndSeparateStores, pref_service_,
-      base::BindRepeating(&PasswordModelTypeController::OnLocalUpmPrefChanged,
-                          base::Unretained(this)));
-#endif
 }
 
 PasswordModelTypeController::~PasswordModelTypeController() = default;
@@ -55,18 +45,11 @@
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
   syncer::ConfigureContext overridden_context = configure_context;
-#if BUILDFLAG(IS_ANDROID)
-  switch (GetLocalUpmPrefValue()) {
-    case prefs::UseUpmLocalAndSeparateStoresState::kOff:
-      break;
-    case prefs::UseUpmLocalAndSeparateStoresState::kOn:
-      overridden_context.sync_mode = syncer::SyncMode::kTransportOnly;
-      break;
-    case prefs::UseUpmLocalAndSeparateStoresState::kOffAndMigrationPending:
-      // Disallowed by GetPreconditionState().
-      NOTREACHED_NORETURN();
+  if (features_util::CanCreateAccountStore(pref_service_) &&
+      base::FeatureList::IsEnabled(
+          syncer::kEnablePasswordsAccountStorageForSyncingUsers)) {
+    overridden_context.sync_mode = syncer::SyncMode::kTransportOnly;
   }
-#endif
   ModelTypeController::LoadModels(overridden_context, model_load_callback);
 }
 
@@ -76,20 +59,6 @@
   ModelTypeController::Stop(fate, std::move(callback));
 }
 
-syncer::DataTypeController::PreconditionState
-PasswordModelTypeController::GetPreconditionState() const {
-#if BUILDFLAG(IS_ANDROID)
-  // If the local UPM migration is pending, wait until it succeeds/fails, so
-  // LoadModels() knows whether to override SyncMode to kTransportOnly or not.
-  return GetLocalUpmPrefValue() == prefs::UseUpmLocalAndSeparateStoresState::
-                                       kOffAndMigrationPending
-             ? PreconditionState::kMustStopAndKeepData
-             : PreconditionState::kPreconditionsMet;
-#else
-  return PreconditionState::kPreconditionsMet;
-#endif
-}
-
 void PasswordModelTypeController::OnAccountsInCookieUpdated(
     const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
     const GoogleServiceAuthError& error) {
@@ -121,24 +90,4 @@
 #endif  // !BUILDFLAG(IS_IOS) && !BUILDFLAG(IS_ANDROID)
 }
 
-#if BUILDFLAG(IS_ANDROID)
-prefs::UseUpmLocalAndSeparateStoresState
-PasswordModelTypeController::GetLocalUpmPrefValue() const {
-  auto value = static_cast<prefs::UseUpmLocalAndSeparateStoresState>(
-      local_upm_pref_.GetValue());
-  switch (value) {
-    case prefs::UseUpmLocalAndSeparateStoresState::kOff:
-    case prefs::UseUpmLocalAndSeparateStoresState::kOn:
-    case prefs::UseUpmLocalAndSeparateStoresState::kOffAndMigrationPending:
-      return value;
-  }
-  NOTREACHED_NORETURN();
-}
-
-void PasswordModelTypeController::OnLocalUpmPrefChanged() {
-  // No-ops are fine.
-  sync_service_->DataTypePreconditionChanged(type());
-}
-#endif
-
 }  // namespace password_manager

diff --git a/components/password_manager/core/browser/sync/password_model_type_controller.h b/components/password_manager/core/browser/sync/password_model_type_controller.h
index 4d29332..60cc903 100644
--- a/components/password_manager/core/browser/sync/password_model_type_controller.h
+++ b/components/password_manager/core/browser/sync/password_model_type_controller.h

@@ -10,7 +10,6 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
-#include "components/prefs/pref_member.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/service/model_type_controller.h"
@@ -53,7 +52,6 @@
   void LoadModels(const syncer::ConfigureContext& configure_context,
                   const ModelLoadCallback& model_load_callback) override;
   void Stop(syncer::SyncStopMetadataFate fate, StopCallback callback) override;
-  PreconditionState GetPreconditionState() const override;
 
   // IdentityManager::Observer overrides.
   void OnAccountsInCookieUpdated(
@@ -62,16 +60,7 @@
   void OnAccountsCookieDeletedByUserAction() override;
 
  private:
-#if BUILDFLAG(IS_ANDROID)
-  prefs::UseUpmLocalAndSeparateStoresState GetLocalUpmPrefValue() const;
-
-  void OnLocalUpmPrefChanged();
-#endif
-
   const raw_ptr<PrefService> pref_service_;
-#if BUILDFLAG(IS_ANDROID)
-  IntegerPrefMember local_upm_pref_;
-#endif
   const raw_ptr<signin::IdentityManager> identity_manager_;
   const raw_ptr<syncer::SyncService> sync_service_;
 

diff --git a/components/password_manager/core/browser/sync/password_model_type_controller_unittest.cc b/components/password_manager/core/browser/sync/password_model_type_controller_unittest.cc
index f34b645..84f27b4 100644
--- a/components/password_manager/core/browser/sync/password_model_type_controller_unittest.cc
+++ b/components/password_manager/core/browser/sync/password_model_type_controller_unittest.cc

@@ -105,18 +105,6 @@
   context.configuration_start_time = base::Time::Now();
   controller()->LoadModels(context, base::DoNothing());
 }
-
-TEST_F(PasswordModelTypeControllerTest, BlockSyncIfUPMLocalMigrationPending) {
-  pref_service()->SetInteger(
-      password_manager::prefs::kPasswordsUseUPMLocalAndSeparateStores,
-      static_cast<int>(
-          password_manager::prefs::UseUpmLocalAndSeparateStoresState::
-              kOffAndMigrationPending));
-
-  EXPECT_EQ(
-      controller()->GetPreconditionState(),
-      PasswordModelTypeController::PreconditionState::kMustStopAndKeepData);
-}
 #endif  // BUILDFLAG(IS_ANDROID)
 
 }  // namespace

diff --git a/components/sync/base/features.cc b/components/sync/base/features.cc
index b21461a..a548ab05 100644
--- a/components/sync/base/features.cc
+++ b/components/sync/base/features.cc

@@ -88,6 +88,15 @@
 #endif
 );
 
+BASE_FEATURE(kEnablePasswordsAccountStorageForSyncingUsers,
+             "EnablePasswordsAccountStorageForSyncingUsers",
+#if BUILDFLAG(IS_ANDROID)
+             base::FEATURE_ENABLED_BY_DEFAULT
+#else
+             base::FEATURE_DISABLED_BY_DEFAULT
+#endif
+);
+
 BASE_FEATURE(kEnablePasswordsAccountStorageForNonSyncingUsers,
              "EnablePasswordsAccountStorageForNonSyncingUsers",
 #if BUILDFLAG(IS_ANDROID)

diff --git a/components/sync/base/features.h b/components/sync/base/features.h
index 81ebcd8e..9eb050d 100644
--- a/components/sync/base/features.h
+++ b/components/sync/base/features.h

@@ -88,9 +88,21 @@
         &kSyncEnableContactInfoDataTypeForDasherUsers,
         "enable_for_google_accounts", false};
 
-// Necessary condition for signed-in non-syncing users to be able to exchange
-// passwords with the sync server.
-// TODO(crbug.com/1509058): Expand docs when the other changes have landed.
+// For users who support separate "profile" and "account" password stores -
+// see password_manager::features_util::CanCreateAccountStore() - and have
+// sync-the-feature on, enabling this flag means:
+// - New passwords are saved to the account store if the passwords data type is
+//   "selected", and to the profile store otherwise. When the flag is disabled,
+//   saves always happen to the profile store.
+// - The account store is synced. When the flag is disabled, the profile one is.
+BASE_DECLARE_FEATURE(kEnablePasswordsAccountStorageForSyncingUsers);
+// For users who support separate "profile" and "account" password stores -
+// see password_manager::features_util::CanCreateAccountStore() - and have
+// sync-the-transport on, enabling this flag means:
+// - New passwords are saved to the account store if the passwords data type is
+//   "selected", and to the profile store otherwise. When the flag is disabled,
+//   saves always happen to the profile store.
+// - The account store is synced. When the flag is disabled, no store is.
 BASE_DECLARE_FEATURE(kEnablePasswordsAccountStorageForNonSyncingUsers);
 
 // Enables a separate account-scoped storage for preferences, for syncing users.

diff --git a/components/sync/service/sync_prefs.cc b/components/sync/service/sync_prefs.cc
index 62b4d42..5c1afd4 100644
--- a/components/sync/service/sync_prefs.cc
+++ b/components/sync/service/sync_prefs.cc

@@ -674,6 +674,13 @@
       return base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos) &&
              base::FeatureList::IsEnabled(kEnablePreferencesAccountStorage);
     case UserSelectableType::kPasswords:
+      // WARNING: This should actually be checking
+      // password_manager::features_util::CanCreateAccountStore() too, otherwise
+      // a crash can happen. But a) it would require a cyclic dependency, and
+      // b) by the time kEnablePasswordsAccountStorageForNonSyncingUsers is
+      // rolled out on Android, CanCreateAccountStore() should always return
+      // true (or at least it can be some trivial GmsCore version check and live
+      // in components/sync/).
       return base::FeatureList::IsEnabled(
           kEnablePasswordsAccountStorageForNonSyncingUsers);
     case UserSelectableType::kAutofill:

diff --git a/components/sync/service/sync_user_settings_impl.h b/components/sync/service/sync_user_settings_impl.h
index 3d805ac..b8c47e5 100644
--- a/components/sync/service/sync_user_settings_impl.h
+++ b/components/sync/service/sync_user_settings_impl.h

@@ -61,6 +61,10 @@
       SyncFirstSetupCompleteSource source) override;
 #endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
   bool IsSyncEverythingEnabled() const override;
+  // TODO(b/321217859): On Android, temporarily remove kPasswords from the
+  // selected types while the local UPM migration is ongoing. This was
+  // previously implemented via GetPreconditionState() but that only affects
+  // the "active" state of the data type, not the "enabled" one.
   UserSelectableTypeSet GetSelectedTypes() const override;
   bool IsTypeManagedByPolicy(UserSelectableType type) const override;
   bool IsTypeManagedByCustodian(UserSelectableType type) const override;