Clean up launched feature RecoverPasswordsForSyncUsers

Bug: none
Change-Id: I93bab47f0dd609e8f71c9427f8ae2821e995193e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868953Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707334}

components/password_manager/core/browser/sync/password_sync_bridge.cc

@@ -172,9 +172,7 @@ bool AreLocalAndRemotePasswordsEqual(
 }
 
 bool ShouldRecoverPasswordsDuringMerge() {
-  return base::FeatureList::IsEnabled(
-             features::kRecoverPasswordsForSyncUsers) &&
-         !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
+  return !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
 }
 
 // A simple class for scoping a password store sync transaction. If the

components/password_manager/core/browser/sync/password_syncable_service.cc

@@ -452,9 +452,7 @@ void PasswordSyncableService::CreateOrUpdateEntry(
 }
 
 bool PasswordSyncableService::ShouldRecoverPasswordsDuringMerge() const {
-  return base::FeatureList::IsEnabled(
-             features::kRecoverPasswordsForSyncUsers) &&
-         !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
+  return !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
 }
 
 syncer::SyncData SyncDataFromPassword(

components/password_manager/core/browser/sync/password_syncable_service.h

@@ -88,14 +88,8 @@ class PasswordSyncableService : public syncer::SyncableService {
 
   // Returns true if corrupted passwords should be deleted from the local
   // database when merging data.
-  // There are two features that handle recovering lost passwords.
-  // RecoverPasswordsForSyncUsers recovers passwords for sync users when merging
-  // data with Sync. If that feature is disabled, this method returns false.
-  // Other feature, DeleteCorruptedPasswords, is introduced after the first
-  // feature and recovers both Sync and non-Sync users internally in
-  // LoginDatabase. When that feature is enabled, this method returns false.
-  // After launching DeleteCorruptedPasswords, RecoverPasswordsForSyncUsers
-  // and related code is to be cleaned up.
+  // This is true if the feature DeleteCorruptedPasswords is disabled, as it
+  // recovers both Sync and non-Sync users internally in LoginDatabase.
   bool ShouldRecoverPasswordsDuringMerge() const;
 
   // The factory that creates sync errors. |SyncError| has rich data

components/password_manager/core/browser/sync/password_syncable_service_unittest.cc

@@ -518,10 +518,8 @@ TEST_F(PasswordSyncableServiceTest, FailedReadFromPasswordStore) {
                           syncer::PASSWORDS);
   EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
       .WillOnce(Return(false));
-  if (base::FeatureList::IsEnabled(features::kRecoverPasswordsForSyncUsers)) {
-    EXPECT_CALL(*password_store(), DeleteUndecryptableLogins())
-        .WillOnce(Return(DatabaseCleanupResult::kDatabaseUnavailable));
-  }
+  EXPECT_CALL(*password_store(), DeleteUndecryptableLogins())
+      .WillOnce(Return(DatabaseCleanupResult::kDatabaseUnavailable));
   EXPECT_CALL(*error_factory, CreateAndUploadError(_, _))
       .WillOnce(Return(error));
   // ActOnPasswordStoreChanges() below shouldn't generate any changes for Sync.
@@ -539,37 +537,13 @@ TEST_F(PasswordSyncableServiceTest, FailedReadFromPasswordStore) {
   service()->ActOnPasswordStoreChanges(list);
 }
 
-// Disable feature for deleting undecryptable logins.
-TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersDisabled) {
+// Test that passwords are recovered for Sync users using the older logic (i.e.
+// recover passwords only for Sync users) when the feature for deleting
+// corrupted passwords for all users is disabled.
+TEST_F(PasswordSyncableServiceTest, RecoverPasswordsOnlyForSyncUsers) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndDisableFeature(
-      features::kRecoverPasswordsForSyncUsers);
-  auto error_factory = std::make_unique<syncer::SyncErrorFactoryMock>();
-  syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
-                          "Failed to get passwords from store.",
-                          syncer::PASSWORDS);
-  EXPECT_CALL(*error_factory, CreateAndUploadError(_, _))
-      .WillOnce(Return(error));
-
-  EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
-      .WillOnce(Return(false));
-  EXPECT_CALL(*password_store(), DeleteUndecryptableLogins()).Times(0);
-
-  syncer::SyncMergeResult result = service()->MergeDataAndStartSyncing(
-      syncer::PASSWORDS, SyncDataList(), std::move(processor_),
-      std::move(error_factory));
-  EXPECT_TRUE(result.error().IsSet());
-}
-
-// Test that passwords are recovered for Sync users using the older feature
-// (kRecoverPasswordsForSyncUsers) when feature for recovering passwords for
-// Sync users is enabled, while feature for deleting corrupted passwords for
-// all users is disabled.
-TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersEnabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      {features::kRecoverPasswordsForSyncUsers},
-      {features::kDeleteCorruptedPasswords});
+      features::kDeleteCorruptedPasswords);
 
   EXPECT_CALL(*processor_, ProcessSyncChanges(_, IsEmpty()));
   EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
@@ -585,15 +559,11 @@ TEST_F(PasswordSyncableServiceTest, RecoverPasswordsForSyncUsersEnabled) {
   EXPECT_FALSE(result.error().IsSet());
 }
 
-// Test that passwords are not recovered using the older feature
-// (kRecoverPasswordForSyncUsers) when merging data if both features for
-// recovering passwords for Sync users and deleting passwords for all users
-// are enabled.
+// Test that passwords are not recovered when merging data if the feature for
+// deleting passwords for all users is enabled.
 TEST_F(PasswordSyncableServiceTest, PasswordRecoveryForAllUsersEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures({features::kRecoverPasswordsForSyncUsers,
-                                        features::kDeleteCorruptedPasswords},
-                                       {});
+  scoped_feature_list.InitAndEnableFeature(features::kDeleteCorruptedPasswords);
 
   auto error_factory = std::make_unique<syncer::SyncErrorFactoryMock>();
   syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
@@ -622,9 +592,8 @@ TEST_F(PasswordSyncableServiceTest, PasswordRecoveryForAllUsersEnabled) {
 #endif
 TEST_F(PasswordSyncableServiceTest, MAYBE_FailedDeleteUndecryptableLogins) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitWithFeatures(
-      {features::kRecoverPasswordsForSyncUsers},
-      {features::kDeleteCorruptedPasswords});
+  scoped_feature_list.InitAndDisableFeature(
+      features::kDeleteCorruptedPasswords);
   auto error_factory = std::make_unique<syncer::SyncErrorFactoryMock>();
   syncer::SyncError error(
       FROM_HERE, syncer::SyncError::DATATYPE_ERROR,

components/password_manager/core/common/password_manager_features.cc

@@ -75,11 +75,6 @@ const base::Feature kPasswordManagerOnboardingAndroid = {
 const base::Feature kPasswordSaveIllustration = {
     "SavePasswordIllustration", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Deletes entries from local database on Mac which cannot be decrypted when
-// merging data with Sync.
-const base::Feature kRecoverPasswordsForSyncUsers = {
-    "RecoverPasswordsForSyncUsers", base::FEATURE_ENABLED_BY_DEFAULT};
-
 // Enables support of filling and saving on username first flow.
 const base::Feature kUsernameFirstFlow = {"UsernameFirstFlow",
                                           base::FEATURE_DISABLED_BY_DEFAULT};

components/password_manager/core/common/password_manager_features.h

@@ -31,7 +31,6 @@ extern const base::Feature kPasswordEditingAndroid;
 extern const base::Feature kPasswordImport;
 extern const base::Feature kPasswordManagerOnboardingAndroid;
 extern const base::Feature kPasswordSaveIllustration;
-extern const base::Feature kRecoverPasswordsForSyncUsers;
 extern const base::Feature kUsernameFirstFlow;
 extern const base::Feature kStickyBubble;