Handle origin-specific password deletion in BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

Index: chrome/browser/browsing_data/browsing_data_remover_unittest.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
index 9a824c451912b984f3ebd83924185826d54a678d..80cabd84b0316152f611926ea22752430a952c65 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
@@ -878,6 +878,29 @@ class RemoveDownloadsTester {
   DISALLOW_COPY_AND_ASSIGN(RemoveDownloadsTester);
 };
 
+class RemovePasswordsTester {
+ public:
+  explicit RemovePasswordsTester(TestingProfile* testing_profile) {
+    PasswordStoreFactory::GetInstance()->SetTestingFactoryAndUse(
+        testing_profile,
+        password_manager::BuildPasswordStore<
+            content::BrowserContext,
+            testing::NiceMock<password_manager::MockPasswordStore>>);

[Timo Reimann, 2016/02/01 10:07:20]

Note that I additionally wrapped the mocked password store into a NiceMock here
in order to prevent emitting warnings for NotifyLoginsChanged being called for
the new test. While this is specific to the new test, the Google Mock
documentation recommends to use NiceMocks by default[1]. Thus, it seemed fine to
me to put it into the general password helper class.

[1]
https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#...
(last paragraph)

+
+    store_ = static_cast<password_manager::MockPasswordStore*>(
+        PasswordStoreFactory::GetInstance()
+            ->GetForProfile(testing_profile, ServiceAccessType::EXPLICIT_ACCESS)
+            .get());
+  }
+
+  password_manager::MockPasswordStore* store() { return store_; }
+
+ private:
+  password_manager::MockPasswordStore* store_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemovePasswordsTester);
+};
+
 // Test Class ----------------------------------------------------------------
 
 class BrowsingDataRemoverTest : public testing::Test {
@@ -2157,18 +2180,22 @@ TEST_F(BrowsingDataRemoverTest, RemoveSameOriginDownloads) {
 }
 
 TEST_F(BrowsingDataRemoverTest, RemovePasswordStatistics) {
-  PasswordStoreFactory::GetInstance()->SetTestingFactoryAndUse(
-      GetProfile(),
-      password_manager::BuildPasswordStore<
-          content::BrowserContext, password_manager::MockPasswordStore>);
-  password_manager::MockPasswordStore* store =
-      static_cast<password_manager::MockPasswordStore*>(
-          PasswordStoreFactory::GetInstance()
-              ->GetForProfile(GetProfile(), ServiceAccessType::EXPLICIT_ACCESS)
-              .get());
-  EXPECT_CALL(*store, RemoveStatisticsCreatedBetweenImpl(base::Time(),
-                                                         base::Time::Max()));
+  RemovePasswordsTester tester(GetProfile());
+
+  EXPECT_CALL(*tester.store(), RemoveStatisticsCreatedBetweenImpl(
+                                   base::Time(), base::Time::Max()));
   BlockUntilBrowsingDataRemoved(
       BrowsingDataRemover::EVERYTHING,
       BrowsingDataRemover::REMOVE_HISTORY, false);
 }
+
+TEST_F(BrowsingDataRemoverTest, RemoveSameOriginPasswords) {
+  RemovePasswordsTester tester(GetProfile());
+  const url::Origin expectedOrigin(kOrigin1);
+
+  EXPECT_CALL(*tester.store(),
+              RemoveLoginsByOriginAndTimeImpl(SameOrigin(expectedOrigin), _, _))
+      .WillOnce(Return(password_manager::PasswordStoreChangeList()));
+  BlockUntilOriginDataRemoved(BrowsingDataRemover::EVERYTHING,
+                              BrowsingDataRemover::REMOVE_PASSWORDS, kOrigin1);

[Mike West, 2016/02/04 18:25:11]

Can you add a test that passes in a URL that produces a unique origin?
`data:text/html,...` for instance shouldn't actually remove any data from the
system.

[Timo Reimann, 2016/02/05 01:12:38]

(Assuming you meant that it shouldn't remove any *origin-specific* data, I went
ahead and:)

Added tests for both the previously existing download case and the password one.
I'm passing in GURL() to account for the switch to checking for emptiness.

+}