Replacing ClearStore with RemoveLoginsCreatedBetween

RemoveLoginsCreatedBetween now accepts base::OnceCallback<void(bool)> as
a completion to be compatible with ClearStore.

Bug: 1226042
Change-Id: If4774792d34018dfecaf796eebb19e1144d55572
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3017999
Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#901023}

chrome/browser/password_manager/android/password_store_bridge.cc

@@ -89,7 +89,8 @@ void PasswordStoreBridge::GetAllCredentials(
 }
 
 void PasswordStoreBridge::ClearAllPasswords(JNIEnv* env) {
-  password_store_->ClearStore(
+  password_store_->RemoveLoginsCreatedBetween(
+      base::Time(), base::Time::Max(),
       base::BindOnce(&PasswordStoreBridge::OnPasswordStoreCleared,
                      weak_factory_.GetWeakPtr()));
 }

chrome/browser/profiles/profile_manager.cc

@@ -1924,7 +1924,7 @@ void ProfileManager::OnLoadProfileForProfileDeletion(
             .get();
     if (password_store.get()) {
       password_store->RemoveLoginsCreatedBetween(
-          base::Time(), base::Time::Max(), base::OnceClosure());
+          base::Time(), base::Time::Max(), base::DoNothing());
     }
 
     // The Profile Data doesn't get wiped until Chrome closes. Since we promised

components/password_manager/core/browser/mock_password_store_interface.h

@@ -32,7 +32,7 @@ class MockPasswordStoreInterface : public PasswordStoreInterface {
               (override));
   MOCK_METHOD(void,
               RemoveLoginsCreatedBetween,
-              (base::Time, base::Time, base::OnceClosure),
+              (base::Time, base::Time, base::OnceCallback<void(bool)>),
               (override));
   MOCK_METHOD(void,
               DisableAutoSignInForOrigins,

components/password_manager/core/browser/password_store.cc

@@ -137,9 +137,10 @@ void PasswordStore::RemoveLoginsByURLAndTime(
       std::move(completion), std::move(sync_completion));
 }
 
-void PasswordStore::RemoveLoginsCreatedBetween(base::Time delete_begin,
-                                               base::Time delete_end,
-                                               base::OnceClosure completion) {
+void PasswordStore::RemoveLoginsCreatedBetween(
+    base::Time delete_begin,
+    base::Time delete_end,
+    base::OnceCallback<void(bool)> completion) {
   DCHECK(main_task_runner_->RunsTasksInCurrentSequence());
   ScheduleTask(
       base::BindOnce(&PasswordStore::RemoveLoginsCreatedBetweenInternal, this,
@@ -322,12 +323,6 @@ void PasswordStore::RemoveFieldInfoByTime(base::Time remove_begin,
                               std::move(completion)));
 }
 
-void PasswordStore::ClearStore(base::OnceCallback<void(bool)> completion) {
-  DCHECK(main_task_runner_->RunsTasksInCurrentSequence());
-  ScheduleTask(base::BindOnce(&PasswordStore::ClearStoreInternal, this,
-                              std::move(completion)));
-}
-
 void PasswordStore::AddObserver(Observer* observer) {
   observers_->AddObserver(observer);
 }
@@ -530,7 +525,7 @@ void PasswordStore::UpdateLoginWithPrimaryKeyInternal(
 void PasswordStore::RemoveLoginsCreatedBetweenInternal(
     base::Time delete_begin,
     base::Time delete_end,
-    base::OnceClosure completion) {
+    base::OnceCallback<void(bool)> completion) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
   TRACE_EVENT0("passwords",
                "PasswordStore::RemoveLoginsCreatedBetweenInternal");
@@ -543,8 +538,10 @@ void PasswordStore::RemoveLoginsCreatedBetweenInternal(
   // sync codebase needs to update metadata atomically together with the login
   // data.
   CommitTransaction();
-  if (completion)
-    main_task_runner_->PostTask(FROM_HERE, std::move(completion));
+  if (completion) {
+    main_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(std::move(completion), !changes.empty()));
+  }
 }
 
 void PasswordStore::RemoveStatisticsByOriginAndTimeInternal(
@@ -596,18 +593,6 @@ void PasswordStore::RemoveFieldInfoByTimeInternal(
     main_task_runner_->PostTask(FROM_HERE, std::move(completion));
 }
 
-void PasswordStore::ClearStoreInternal(
-    base::OnceCallback<void(bool)> completion) {
-  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
-  bool should_clear = !IsEmpty();
-  if (should_clear)
-    DeleteAndRecreateDatabaseFile();
-  if (completion) {
-    main_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(std::move(completion), should_clear));
-  }
-}
-
 std::vector<std::unique_ptr<PasswordForm>> PasswordStore::GetLoginsImpl(
     const PasswordFormDigest& form) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());

components/password_manager/core/browser/password_store.h

@@ -108,9 +108,10 @@ class PasswordStore : protected PasswordStoreSync,
       base::OnceClosure completion,
       base::OnceCallback<void(bool)> sync_completion =
           base::NullCallback()) override;
-  void RemoveLoginsCreatedBetween(base::Time delete_begin,
-                                  base::Time delete_end,
-                                  base::OnceClosure completion) override;
+  void RemoveLoginsCreatedBetween(
+      base::Time delete_begin,
+      base::Time delete_end,
+      base::OnceCallback<void(bool)> completion) override;
   void DisableAutoSignInForOrigins(
       const base::RepeatingCallback<bool(const GURL&)>& origin_filter,
       base::OnceClosure completion) override;
@@ -172,12 +173,6 @@ class PasswordStore : protected PasswordStoreSync,
                              base::Time remove_end,
                              base::OnceClosure completion);
 
-  // Deletes and re-creates the whole PasswordStore, unless it is already empty
-  // anyway. If |completion| is not null, it will be posted to the
-  // |main_task_runner_| once the process is complete. The bool parameter
-  // indicates whether any data was actually cleared.
-  void ClearStore(base::OnceCallback<void(bool)> completion);
-
   // Schedules the given |task| to be run on the PasswordStore's TaskRunner.
   bool ScheduleTask(base::OnceClosure task);
 
@@ -391,9 +386,10 @@ class PasswordStore : protected PasswordStoreSync,
   void RemoveLoginInternal(const PasswordForm& form);
   void UpdateLoginWithPrimaryKeyInternal(const PasswordForm& new_form,
                                          const PasswordForm& old_primary_key);
-  void RemoveLoginsCreatedBetweenInternal(base::Time delete_begin,
-                                          base::Time delete_end,
-                                          base::OnceClosure completion);
+  void RemoveLoginsCreatedBetweenInternal(
+      base::Time delete_begin,
+      base::Time delete_end,
+      base::OnceCallback<void(bool)> completion);
   void RemoveStatisticsByOriginAndTimeInternal(
       const base::RepeatingCallback<bool(const GURL&)>& origin_filter,
       base::Time delete_begin,
@@ -414,8 +410,6 @@ class PasswordStore : protected PasswordStoreSync,
                                      base::Time remove_end,
                                      base::OnceClosure completion);
 
-  void ClearStoreInternal(base::OnceCallback<void(bool)> completion);
-
   // Finds all PasswordForms with a signon_realm that is equal to, or is a
   // PSL-match to that of |form|, and takes care of notifying the consumer with
   // the results when done.

components/password_manager/core/browser/password_store_interface.h

@@ -91,10 +91,12 @@ class PasswordStoreInterface : public RefcountedKeyedService {
 
   // Removes all logins created in the given date range. If `completion` is not
   // null, it will be run after deletions have been completed and notification
-  // have been sent out.
-  virtual void RemoveLoginsCreatedBetween(base::Time delete_begin,
-                                          base::Time delete_end,
-                                          base::OnceClosure completion) = 0;
+  // have been sent out. If any logins were removed 'true' will be passed to a
+  // completion, 'false' otherwise.
+  virtual void RemoveLoginsCreatedBetween(
+      base::Time delete_begin,
+      base::Time delete_end,
+      base::OnceCallback<void(bool)> completion) = 0;
 
   // Sets the 'skip_zero_click' flag for all credentials that match
   // `origin_filter`. `completion` will be run after these modifications are

components/password_manager/core/browser/password_store_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
@@ -346,9 +347,9 @@ TEST_F(PasswordStoreTest, RemoveLoginsCreatedBetweenCallbackIsCalled) {
 
   EXPECT_CALL(mock_observer, OnLoginsChanged(_, testing::SizeIs(1u)));
   base::RunLoop run_loop;
-  store->RemoveLoginsCreatedBetween(base::Time::FromDoubleT(0),
-                                    base::Time::FromDoubleT(2),
-                                    run_loop.QuitClosure());
+  store->RemoveLoginsCreatedBetween(
+      base::Time::FromDoubleT(0), base::Time::FromDoubleT(2),
+      base::BindLambdaForTesting([&run_loop](bool) { run_loop.Quit(); }));
   run_loop.Run();
   testing::Mock::VerifyAndClearExpectations(&mock_observer);
 
@@ -384,9 +385,9 @@ TEST_F(PasswordStoreTest, InsecureCredentialsObserverOnRemoveLogin) {
 
   MockInsecureCredentialsConsumer consumer;
   base::RunLoop run_loop;
-  store->RemoveLoginsCreatedBetween(base::Time::FromDoubleT(0),
-                                    base::Time::FromDoubleT(2),
-                                    run_loop.QuitClosure());
+  store->RemoveLoginsCreatedBetween(
+      base::Time::FromDoubleT(0), base::Time::FromDoubleT(2),
+      base::BindLambdaForTesting([&run_loop](bool) { run_loop.Quit(); }));
   run_loop.Run();
 
   EXPECT_CALL(consumer, OnGetInsecureCredentials(testing::IsEmpty()));

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

@@ -210,7 +210,8 @@ void PasswordModelTypeController::MaybeClearStore(
   if (features_util::IsOptedInForAccountStorage(pref_service_, sync_service_)) {
     RecordClearedOnStartup(ClearedOnStartup::kOptedInSoNoNeedToClear);
   } else {
-    account_password_store_for_cleanup->ClearStore(
+    account_password_store_for_cleanup->RemoveLoginsCreatedBetween(
+        base::Time(), base::Time::Max(),
         base::BindOnce(&PasswordStoreClearDone));
   }
 }

ios/chrome/browser/browsing_data/browsing_data_remover_impl.mm

@@ -422,8 +422,11 @@ void BrowsingDataRemoverImpl::RemoveImpl(base::Time delete_begin,
             .get();
 
     if (password_store) {
+      // It doesn't matter whether any logins were removed so bool argument can
+      // be omitted.
       password_store->RemoveLoginsCreatedBetween(
-          delete_begin, delete_end, CreatePendingTaskCompletionClosure());
+          delete_begin, delete_end,
+          IgnoreArgument<bool>(CreatePendingTaskCompletionClosure()));
     }
   }
 

ios/chrome/browser/passwords/password_manager_app_interface.mm

@@ -97,7 +97,7 @@ class PasswordStoreConsumerHelper : public PasswordStoreConsumer {
           .get();
   // Remove credentials stored during executing the test.
   passwordStore->RemoveLoginsCreatedBetween(base::Time(), base::Time::Now(),
-                                            base::OnceClosure());
+                                            base::DoNothing());
 }
 
 + (int)storedCredentialsCount {

ios/chrome/browser/ui/autofill/autofill_app_interface.mm

@@ -147,7 +147,7 @@ void SaveLocalPasswordForm(const GURL& url) {
 // Removes all credentials stored.
 void ClearPasswordStore() {
   GetPasswordStore()->RemoveLoginsCreatedBetween(base::Time(), base::Time(),
-                                                 base::OnceClosure());
+                                                 base::DoNothing());
   TestStoreConsumer consumer;
 }
 

ios/chrome/browser/ui/settings/password/passwords_settings_app_interface.mm

@@ -137,7 +137,7 @@ PasswordForm CreateSampleFormWithIndex(int index) {
 
 bool ClearPasswordStore() {
   GetPasswordStore()->RemoveLoginsCreatedBetween(base::Time(), base::Time(),
-                                                 base::OnceClosure());
+                                                 base::DoNothing());
   FakeStoreConsumer consumer;
   if (!consumer.FetchStoreResults()) {
     return false;