Implement origin-based deletion for passwords in PasswordDefaultMac.

components/password_manager/core/browser/password_store_common_unittest.h

Index: components/password_manager/core/browser/password_store_common_unittest.h
diff --git a/components/password_manager/core/browser/password_store_common_unittest.h b/components/password_manager/core/browser/password_store_common_unittest.h
new file mode 100644
index 0000000000000000000000000000000000000000..9d768f2076abc707611f32d52f7cd49353a30654
--- /dev/null
+++ b/components/password_manager/core/browser/password_store_common_unittest.h
@@ -0,0 +1,139 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_STORE_COMMON_UNITTEST_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_STORE_COMMON_UNITTEST_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/password_store.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/origin.h"
+
+using autofill::PasswordForm;
+using password_manager::PasswordStore;
+using testing::_;
+using testing::ElementsAre;
+
+namespace password_manager {
+
+namespace {
+
+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",
+                           // Ignore cleared password values on Mac.
+                           // crbug.com/466638
+                           nullptr, true, false, base::Time::Now().ToDoubleT()};

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

I'd prefer a const number here so the tests are reproducible.

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

Good point, done.

+  return data;
+}
+
+}  // namespace
+
+// Collection of testcases common to all platform-specific stores.
+// The template type T represents a test delegate that must implement the
+// following methods:
+//    // Returns a pointer to a fully initialized store for polymorphic usage.
+//    PasswordStore* store();
+//
+//    // Finishes all asnychronous processing on the store.
+//    void FinishAsyncProcessing();
+template <typename T>
+class PasswordStoreCommonTest : public testing::Test {

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

If it's only about deletion by origin then I'd name the class accordingly. Or
you want to move more common tests into PasswordStoreCommonTest?

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

Generally, I consider the class to be a suitable for holding generic tests. I
currently don't have any tests planned other than origin-based ones, however, so
I followed your advice and renamed it to PasswordStoreOriginTest.

+ protected:
+  PasswordStoreCommonTest() : store_(delegate_.store()) {}

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

I'd move the init/deinit to SetUp()/TearDown().

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

May I ask for your rationale? The Google Test documentation[1] generally speaks
in favor of fixture constructors/destructors, and we don't seem to hit any of
the exceptional cases.

[1]https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/destructor_of_the_test_fixture_or_t

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

Acknowledged.

+
+  ~PasswordStoreCommonTest() {
+    this->store_->Shutdown();

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

I'd separate the logic as following. The delegate is responsible for
creating/destroying/initializing the password store. PasswordStoreCommonTest is
responsible for the tests only.

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

Moved store destruction into the delegates. Looks like this was the only bit
missing in order to follow your suggestion.

+    this->delegate_.FinishAsyncProcessing();
+  }
+
+  T delegate_;
+  PasswordStore* store_;

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

I think you don't need it as you can retrieve from the delegate.

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

Removed and re-routed through the delegate.

+};
+
+TYPED_TEST_CASE_P(PasswordStoreCommonTest);
+
+TYPED_TEST_P(PasswordStoreCommonTest,
+             RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime) {
+  const char origin_url[] = "http://foo.example.com";

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

Declare the string as a constant in the beginning of the file.

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

IMHO, it'd make the RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin test
(which requires two differing origins) harder to read because the required test
data would be split across the file header (shared origin) and the test
implementation (distinct origin). With such few tests at the moment, I'm not
sure it's worth it. WDYT?

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

Acknowledged.

+  scoped_ptr<PasswordForm> form = CreatePasswordFormFromDataForTesting(
+      CreateTestPasswordFormDataByOrigin(origin_url));
+  this->store_->AddLogin(*form);
+  this->delegate_.FinishAsyncProcessing();
+
+  MockPasswordStoreObserver observer;
+  this->store_->AddObserver(&observer);
+  EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange(
+                            PasswordStoreChange::REMOVE, *form))));
+
+  const url::Origin origin((GURL(origin_url)));
+  base::RunLoop run_loop;
+  this->store_->RemoveLoginsByOriginAndTime(
+      origin, base::Time::Time(), base::Time::Max(), run_loop.QuitClosure());
+  run_loop.Run();
+
+  this->store_->RemoveObserver(&observer);
+}
+
+TYPED_TEST_P(PasswordStoreCommonTest,
+             RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin) {
+  const char origin_url[] = "http://foo.example.com";
+  scoped_ptr<autofill::PasswordForm> form =
+      CreatePasswordFormFromDataForTesting(
+          CreateTestPasswordFormDataByOrigin(origin_url));
+  this->store_->AddLogin(*form);
+  this->delegate_.FinishAsyncProcessing();
+
+  MockPasswordStoreObserver observer;
+  this->store_->AddObserver(&observer);
+  EXPECT_CALL(observer, OnLoginsChanged(_)).Times(0);
+
+  const url::Origin other_origin(GURL("http://bar.example.com"));
+  base::RunLoop run_loop;
+  this->store_->RemoveLoginsByOriginAndTime(
+      other_origin, base::Time(), base::Time::Max(), run_loop.QuitClosure());
+  run_loop.Run();
+
+  this->store_->RemoveObserver(&observer);
+}
+
+TYPED_TEST_P(PasswordStoreCommonTest,
+             RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) {
+  const char origin_url[] = "http://foo.example.com";
+  scoped_ptr<autofill::PasswordForm> form =
+      CreatePasswordFormFromDataForTesting(
+          CreateTestPasswordFormDataByOrigin(origin_url));
+  this->store_->AddLogin(*form);
+  this->delegate_.FinishAsyncProcessing();
+
+  MockPasswordStoreObserver observer;
+  this->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;
+  this->store_->RemoveLoginsByOriginAndTime(origin, time_after_creation_date,
+                                            base::Time::Max(),
+                                            run_loop.QuitClosure());
+  run_loop.Run();
+
+  this->store_->RemoveObserver(&observer);
+}
+
+REGISTER_TYPED_TEST_CASE_P(
+    PasswordStoreCommonTest,
+    RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime,
+    RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin,
+    RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval);
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_STORE_COMMON_UNITTEST_H_