Implement origin-based deletion for passwords in PasswordDefaultMac.

components/password_manager/core/browser/password_store_default_unittest.cc

Index: components/password_manager/core/browser/password_store_default_unittest.cc
diff --git a/components/password_manager/core/browser/password_store_default_unittest.cc b/components/password_manager/core/browser/password_store_default_unittest.cc
index 53080d96c80a383e0ba77a9c71807496c643e130..07b75f303cdf411ea4f058cdee5320475a078137 100644
--- a/components/password_manager/core/browser/password_store_default_unittest.cc
+++ b/components/password_manager/core/browser/password_store_default_unittest.cc
@@ -18,6 +18,7 @@
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/password_store_default.h"
+#include "components/password_manager/core/browser/password_store_origin_unittest.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/origin.h"
@@ -43,11 +44,6 @@ class MockPasswordStoreConsumer : public PasswordStoreConsumer {
   }
 };
 
-class MockPasswordStoreObserver : public PasswordStore::Observer {
- public:
-  MOCK_METHOD1(OnLoginsChanged, void(const PasswordStoreChangeList& changes));
-};
-
 // A mock LoginDatabase that simulates a failing Init() method.
 class BadLoginDatabase : public LoginDatabase {
  public:
@@ -79,64 +75,107 @@ PasswordFormData CreateTestPasswordFormData() {
   return data;
 }
 
-PasswordFormData CreateTestPasswordFormDataByOrigin(const char* origin_url) {
-  PasswordFormData data = {PasswordForm::SCHEME_HTML,
-                           origin_url,
-                           origin_url,
-                           origin_url,
-                           L"submit_element",
-                           L"username_element",
-                           L"password_element",
-                           L"username_value",
-                           L"password_value",
-                           true,
-                           false,
-                           base::Time::Now().ToDoubleT()};
-  return data;
-}
-
 }  // anonymous namespace
 
-class PasswordStoreDefaultTest : public testing::Test {
+class PasswordStoreDefaultTestDelegateBase {

[vasilii, 2015/11/18 16:27:42]

Why do you need a base class if it's unused.

[Timo Reimann, 2015/11/18 23:42:15]

In order to guarantee invocation of the initialization method. See also my
comment in patch set 1:
https://codereview.chromium.org/1414463004/diff/1/components/password_manager...

[vasilii, 2015/11/23 15:04:56]

Very technical reason. PasswordStoreDefaultTestDelegateBase should be removed. I
replied to your previous comment.

[Timo Reimann, 2015/11/24 02:36:09]

I removed it.

+ public:
+  PasswordStoreDefaultTestDelegateBase();
+
  protected:
-  void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  }
+  base::ScopedTempDir temp_dir_;

[vasilii, 2015/11/18 16:27:42]

Should be private.

[Timo Reimann, 2015/11/18 23:42:15]

True if we decide to get rid of PasswordStoreDefaultTestDelegateBase. For now,
the member is accessed by the inheriting class PasswordStoreDefaultTestDelegate
through its terminate() method.

 
-  void TearDown() override {
-    base::MessageLoop::current()->RunUntilIdle();
-    ASSERT_TRUE(temp_dir_.Delete());
-  }
+ private:
+  void initialize();

[vasilii, 2015/11/18 16:27:42]

Get rid of the method by inlining.

[Timo Reimann, 2015/11/18 23:42:15]

I defined the method in the class body. In case you meant to inline directly in
the constructor: That's not possible because Google Test doesn't allow ASSERT_*
statements in constructors and destructors.

 
-  base::FilePath test_login_db_file_path() const {
-    return temp_dir_.path().Append(FILE_PATH_LITERAL("login_test"));
-  }
+  base::MessageLoopForUI message_loop_;
 
-  scoped_refptr<PasswordStoreDefault> CreateInitializedStore() {
-    return CreateInitializedStore(
-        make_scoped_ptr(new LoginDatabase(test_login_db_file_path())));
-  }
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreDefaultTestDelegateBase);
+};
+
+PasswordStoreDefaultTestDelegateBase::PasswordStoreDefaultTestDelegateBase() {
+  initialize();
+}
+
+void PasswordStoreDefaultTestDelegateBase::initialize() {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+}
+
+class PasswordStoreDefaultTestDelegate
+    : public PasswordStoreDefaultTestDelegateBase {
+ public:
+  PasswordStoreDefaultTestDelegate();
+  explicit PasswordStoreDefaultTestDelegate(scoped_ptr<LoginDatabase> database);
+  ~PasswordStoreDefaultTestDelegate();
+
+  void terminate();

[vasilii, 2015/11/18 16:27:42]

Also inline

[Timo Reimann, 2015/11/18 23:42:15]

Done; see my comment above though.

[vasilii, 2015/11/23 15:04:56]

I meant get rid of the method by inlining it.

[Timo Reimann, 2015/11/24 02:36:09]

As mentioned with initialize(), you cannot call Google Test ASSERT_* statements
in constructors or destructors, so I cannot inline the method in
~PasswordStoreDefaultTestDelegate().

+
+  PasswordStoreDefault* store() { return store_.get(); }
+
+  void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); }
+
+ private:
+  scoped_refptr<PasswordStoreDefault> CreateInitializedStore();
 
   scoped_refptr<PasswordStoreDefault> CreateInitializedStore(
-      scoped_ptr<LoginDatabase> database) {
-    scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
-        base::ThreadTaskRunnerHandle::Get(),
-        base::ThreadTaskRunnerHandle::Get(), database.Pass()));
-    store->Init(syncer::SyncableService::StartSyncFlare());
+      scoped_ptr<LoginDatabase> database);
 
-    return store;
-  }
+  base::FilePath test_login_db_file_path() const;
 
-  base::MessageLoopForUI message_loop_;
-  base::ScopedTempDir temp_dir_;
+  scoped_refptr<PasswordStoreDefault> store_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreDefaultTestDelegate);
 };
 
+PasswordStoreDefaultTestDelegate::PasswordStoreDefaultTestDelegate()
+    : store_(CreateInitializedStore()) {}
+
+PasswordStoreDefaultTestDelegate::PasswordStoreDefaultTestDelegate(
+    scoped_ptr<LoginDatabase> database)
+    : store_(CreateInitializedStore(database.Pass())) {}
+
+PasswordStoreDefaultTestDelegate::~PasswordStoreDefaultTestDelegate() {
+  terminate();
+}
+
+void PasswordStoreDefaultTestDelegate::terminate() {
+  base::MessageLoop::current()->RunUntilIdle();
+  ASSERT_TRUE(temp_dir_.Delete());
+  store_->Shutdown();

[vasilii, 2015/11/18 16:27:42]

First Shutdown, then everything else.

[Timo Reimann, 2015/11/18 23:42:15]

Done.

+}
+
+scoped_refptr<PasswordStoreDefault>
+PasswordStoreDefaultTestDelegate::CreateInitializedStore() {
+  return CreateInitializedStore(
+      make_scoped_ptr(new LoginDatabase(test_login_db_file_path())));
+}
+
+scoped_refptr<PasswordStoreDefault>
+PasswordStoreDefaultTestDelegate::CreateInitializedStore(
+    scoped_ptr<LoginDatabase> database) {
+  scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(),
+      database.Pass()));
+  store->Init(syncer::SyncableService::StartSyncFlare());
+
+  return store;
+}
+
+base::FilePath PasswordStoreDefaultTestDelegate::test_login_db_file_path()
+    const {
+  return temp_dir_.path().Append(FILE_PATH_LITERAL("login_test"));
+}
+
+INSTANTIATE_TYPED_TEST_CASE_P(Default,
+                              PasswordStoreOriginTest,
+                              PasswordStoreDefaultTestDelegate);
+
 ACTION(STLDeleteElements0) {
   STLDeleteContainerPointers(arg0.begin(), arg0.end());
 }
 
-TEST_F(PasswordStoreDefaultTest, NonASCIIData) {
-  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
+TEST(PasswordStoreDefaultTest, NonASCIIData) {
+  PasswordStoreDefaultTestDelegate delegate;
+  PasswordStoreDefault* store = delegate.store();
 
   // Some non-ASCII password form data.
   static const PasswordFormData form_data[] = {
@@ -171,12 +210,11 @@ TEST_F(PasswordStoreDefaultTest, NonASCIIData) {
   store->GetAutofillableLogins(&consumer);
 
   base::MessageLoop::current()->RunUntilIdle();
-
-  store->Shutdown();
 }
 
-TEST_F(PasswordStoreDefaultTest, Notifications) {
-  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
+TEST(PasswordStoreDefaultTest, Notifications) {
+  PasswordStoreDefaultTestDelegate delegate;
+  PasswordStoreDefault* store = delegate.store();
 
   scoped_ptr<PasswordForm> form =
       CreatePasswordFormFromDataForTesting(CreateTestPasswordFormData());
@@ -221,16 +259,15 @@ TEST_F(PasswordStoreDefaultTest, Notifications) {
   base::MessageLoop::current()->RunUntilIdle();
 
   store->RemoveObserver(&observer);
-  store->Shutdown();
 }
 
 // Verify that operations on a PasswordStore with a bad database cause no
 // explosions, but fail without side effect, return no data and trigger no
 // notifications.
-TEST_F(PasswordStoreDefaultTest, OperationsOnABadDatabaseSilentlyFail) {
-  scoped_refptr<PasswordStoreDefault> bad_store =
-      CreateInitializedStore(make_scoped_ptr(new BadLoginDatabase));
-
+TEST(PasswordStoreDefaultTest, OperationsOnABadDatabaseSilentlyFail) {
+  PasswordStoreDefaultTestDelegate delegate(
+      make_scoped_ptr<LoginDatabase>(new BadLoginDatabase));
+  PasswordStoreDefault* bad_store = delegate.store();
   base::MessageLoop::current()->RunUntilIdle();
   ASSERT_EQ(nullptr, bad_store->login_db());
 
@@ -285,85 +322,6 @@ TEST_F(PasswordStoreDefaultTest, OperationsOnABadDatabaseSilentlyFail) {
 
   // Ensure no notifications and no explosions during shutdown either.
   bad_store->RemoveObserver(&mock_observer);
-  bad_store->Shutdown();
-}
-
-TEST_F(PasswordStoreDefaultTest,
-       RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime) {
-  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
-
-  const char origin_url[] = "http://foo.example.com";
-  scoped_ptr<autofill::PasswordForm> form =
-      CreatePasswordFormFromDataForTesting(
-          CreateTestPasswordFormDataByOrigin(origin_url));
-  store->AddLogin(*form);
-  base::MessageLoop::current()->RunUntilIdle();
-
-  MockPasswordStoreObserver observer;
-  store->AddObserver(&observer);
-  EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange(
-                            PasswordStoreChange::REMOVE, *form))));
-
-  const url::Origin origin((GURL(origin_url)));
-  base::RunLoop run_loop;
-  store->RemoveLoginsByOriginAndTime(origin, base::Time(), base::Time::Max(),
-                                     run_loop.QuitClosure());
-  run_loop.Run();
-
-  store->RemoveObserver(&observer);
-  store->Shutdown();
-}
-
-TEST_F(PasswordStoreDefaultTest,
-       RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin) {
-  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
-
-  const char origin_url[] = "http://foo.example.com";
-  scoped_ptr<autofill::PasswordForm> form =
-      CreatePasswordFormFromDataForTesting(
-          CreateTestPasswordFormDataByOrigin(origin_url));
-  store->AddLogin(*form);
-  base::MessageLoop::current()->RunUntilIdle();
-
-  MockPasswordStoreObserver observer;
-  store->AddObserver(&observer);
-  EXPECT_CALL(observer, OnLoginsChanged(_)).Times(0);
-
-  const url::Origin other_origin(GURL("http://bar.example.com"));
-  base::RunLoop run_loop;
-  store->RemoveLoginsByOriginAndTime(other_origin, base::Time(),
-                                     base::Time::Max(), run_loop.QuitClosure());
-  run_loop.Run();
-
-  store->RemoveObserver(&observer);
-  store->Shutdown();
-}
-
-TEST_F(PasswordStoreDefaultTest,
-       RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) {
-  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
-
-  const char origin_url[] = "http://foo.example.com";
-  scoped_ptr<autofill::PasswordForm> form =
-      CreatePasswordFormFromDataForTesting(
-          CreateTestPasswordFormDataByOrigin(origin_url));
-  store->AddLogin(*form);
-  base::MessageLoop::current()->RunUntilIdle();
-
-  MockPasswordStoreObserver observer;
-  store->AddObserver(&observer);
-  EXPECT_CALL(observer, OnLoginsChanged(_)).Times(0);
-
-  const url::Origin origin((GURL(origin_url)));
-  base::Time time_after_creation_date =
-      form->date_created + base::TimeDelta::FromDays(1);
-  base::RunLoop run_loop;
-  store->RemoveLoginsByOriginAndTime(origin, time_after_creation_date,
-                                     base::Time::Max(), run_loop.QuitClosure());
-  run_loop.Run();
-
-  store->RemoveObserver(&observer);
-  store->Shutdown();
 }
 
 }  // namespace password_manager