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 1d8a8998bb8e1f71fb3bad5aab849eaa745481d5..b4ea2593c18162c95ff395c3e34230337cf92313 100644
--- a/components/password_manager/core/browser/password_store_default_unittest.cc
+++ b/components/password_manager/core/browser/password_store_default_unittest.cc
@@ -16,6 +16,7 @@
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/password_store_change.h"
+#include "components/password_manager/core/browser/password_store_common_unittest.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/password_store_default.h"
 #include "testing/gmock/include/gmock/gmock.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,62 +75,78 @@ 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/10/21 16:47:57]

Why do you need a base class if it's not used without
PasswordStoreDefaultTestDelegate?

[Timo Reimann, 2015/11/17 23:10:56]

To have initialize() be called automatically through the base class constructor
once any of the two PasswordStoreDefaultTestDelegate constructors is invoked.
Otherwise, there would be a chance we'd miss triggering initialization for the
existing and any future overloaded constructors.

I did remove ~PasswordStoreDefaultTestDelegateBase() though by moving
terminate() into PasswordStoreDefaultTestDelegate and calling it from the child
class destructor. Also missed to use public inheritance.

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

Invoke your method from both constructors. Note that a delegating constructor is
also allowed in the C++11 style guide.

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

Done.

The downside now is that I cannot set |store_| in the initializer list anymore
because temp_dir_.CreateUniqueTempDir() needs to be called first. A delegating
constructor did not seem to help here either.

(On the plus side, I could now remove
PasswordStoreDefaultTestDelegate::CreateInitializedStore()).

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

[vasilii, 2015/10/21 16:47:57]

Why is it protected?

[Timo Reimann, 2015/11/17 23:10:56]

Because PasswordStoreDefaultTestDelegate::test_login_db_file_path() is accessing
it.

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

[vasilii, 2015/10/21 16:47:57]

Add noncopyable macros here and to the delegate.

[Timo Reimann, 2015/11/17 23:10:56]

Done. Added to the Mac delegate as well.

 
-  base::FilePath test_login_db_file_path() const {
-    return temp_dir_.path().Append(FILE_PATH_LITERAL("login_test"));
+class PasswordStoreDefaultTestDelegate : PasswordStoreDefaultTestDelegateBase {
+ public:
+  PasswordStoreDefaultTestDelegate() : store_(CreateInitializedStore()) {}
+  PasswordStoreDefaultTestDelegate(scoped_ptr<LoginDatabase> database)
+      : store_(CreateInitializedStore(database.Pass())) {}
+  ~PasswordStoreDefaultTestDelegate() {
+    base::MessageLoop::current()->RunUntilIdle();
+    store_->Shutdown();
   }
 
+  PasswordStoreDefault* store() { return store_.get(); }
+
+  void FinishAsyncProcessing() { base::MessageLoop::current()->RunUntilIdle(); }
+
+ private:
+  scoped_refptr<PasswordStoreDefault> store_;

[vasilii, 2015/10/21 16:47:57]

Member declaration is after methods declaration.

[Timo Reimann, 2015/11/17 23:10:55]

Fixed.

+
   scoped_refptr<PasswordStoreDefault> CreateInitializedStore() {
+    return CreateInitializedStore(
+        make_scoped_ptr(new LoginDatabase(test_login_db_file_path())));
+  }
+
+  scoped_refptr<PasswordStoreDefault> CreateInitializedStore(
+      scoped_ptr<LoginDatabase> database) {
     scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
         base::ThreadTaskRunnerHandle::Get(),

[vasilii, 2015/10/21 16:47:57]

Should you use |message_loop_|? Otherwise why do you need |message_loop_|.

[Timo Reimann, 2015/11/17 23:10:55]

Another case of copying code over from the existing implementation as-is. Is it
possibly referenced through base::MessageLoop::current()?

TODO: Check if removing makes a difference.

-        base::ThreadTaskRunnerHandle::Get(),
-        make_scoped_ptr(new LoginDatabase(test_login_db_file_path()))));
+        base::ThreadTaskRunnerHandle::Get(), database.Pass()));
     store->Init(syncer::SyncableService::StartSyncFlare());
 
     return store;
   }
 
-  base::MessageLoopForUI message_loop_;
-  base::ScopedTempDir temp_dir_;
+  base::FilePath test_login_db_file_path() const {
+    return temp_dir_.path().Append(FILE_PATH_LITERAL("login_test"));
+  }
 };
 
+INSTANTIATE_TYPED_TEST_CASE_P(Default,
+                              PasswordStoreCommonTest,
+                              PasswordStoreDefaultTestDelegate);
+
+class PasswordStoreDefaultTest : public testing::Test {};

[vasilii, 2015/10/21 16:47:57]

You don't need an empty class. Just change TEST_F to TEST.

[Timo Reimann, 2015/11/17 23:10:55]

Of course! Done.

+
 ACTION(STLDeleteElements0) {
   STLDeleteContainerPointers(arg0.begin(), arg0.end());
 }
 
 TEST_F(PasswordStoreDefaultTest, NonASCIIData) {
-  scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
-      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(),
-      make_scoped_ptr(new LoginDatabase(test_login_db_file_path()))));
-  store->Init(syncer::SyncableService::StartSyncFlare());
+  PasswordStoreDefaultTestDelegate delegate;
+  PasswordStoreDefault* store = delegate.store();
 
   // Some non-ASCII password form data.
   static const PasswordFormData form_data[] = {
@@ -169,15 +181,11 @@ TEST_F(PasswordStoreDefaultTest, NonASCIIData) {
   store->GetAutofillableLogins(&consumer);
 
   base::MessageLoop::current()->RunUntilIdle();
-
-  store->Shutdown();
 }
 
 TEST_F(PasswordStoreDefaultTest, Notifications) {
-  scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
-      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(),
-      make_scoped_ptr(new LoginDatabase(test_login_db_file_path()))));
-  store->Init(syncer::SyncableService::StartSyncFlare());
+  PasswordStoreDefaultTestDelegate delegate;
+  PasswordStoreDefault* store = delegate.store();
 
   scoped_ptr<PasswordForm> form =
       CreatePasswordFormFromDataForTesting(CreateTestPasswordFormData());
@@ -222,18 +230,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(new PasswordStoreDefault(
-      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(),
-      make_scoped_ptr<LoginDatabase>(new BadLoginDatabase)));
-
-  bad_store->Init(syncer::SyncableService::StartSyncFlare());
+  PasswordStoreDefaultTestDelegate delegate(
+      make_scoped_ptr<LoginDatabase>(new BadLoginDatabase));
+  PasswordStoreDefault* bad_store = delegate.store();
   base::MessageLoop::current()->RunUntilIdle();
   ASSERT_EQ(nullptr, bad_store->login_db());
 
@@ -286,85 +291,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