Make FormFetcher a PasswordStoreConsumer

components/password_manager/core/browser/form_fetcher_impl_unittest.cc

Index: components/password_manager/core/browser/form_fetcher_impl_unittest.cc
diff --git a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
index 1059fc7c016961e38afb31cde3e5b886c51d9d55..301bc7ec7bd75e0504d830133eb0958948543854 100644
--- a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
+++ b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
@@ -11,10 +11,15 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/statistics_table.h"
 #include "components/password_manager/core/browser/stub_credentials_filter.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
@@ -29,6 +34,7 @@ using base::StringPiece;
 using testing::_;
 using testing::IsEmpty;
 using testing::Pointee;
+using testing::Return;
 using testing::UnorderedElementsAre;
 
 namespace password_manager {
@@ -80,13 +86,18 @@ class FakePasswordManagerClient : public StubPasswordManagerClient {
     filter_ = std::move(filter);
   }
 
+  void set_store(PasswordStore* store) { store_ = store; }
+
  private:
   const CredentialsFilter* GetStoreResultFilter() const override {
     return filter_ ? filter_.get()
                    : StubPasswordManagerClient::GetStoreResultFilter();
   }
 
+  PasswordStore* GetPasswordStore() const override { return store_; }
+
   std::unique_ptr<CredentialsFilter> filter_;
+  PasswordStore* store_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(FakePasswordManagerClient);
 };
@@ -124,70 +135,95 @@ PasswordForm CreateAndroidFederated() {
 
 class FormFetcherImplTest : public testing::Test {
  public:
-  FormFetcherImplTest() : form_fetcher_(&client_) {}
+  FormFetcherImplTest()
+      : form_digest_(PasswordForm::SCHEME_HTML,
+                     "http://accounts.google.com",
+                     GURL("http://accounts.google.com/a/LoginAuth")) {
+    mock_store_ = new MockPasswordStore();
+    client_.set_store(mock_store_.get());
+
+    form_fetcher_ = base::MakeUnique<FormFetcherImpl>(form_digest_, &client_);
+  }
 
-  ~FormFetcherImplTest() override = default;
+  ~FormFetcherImplTest() override { mock_store_->ShutdownOnUIThread(); }
 
  protected:
+  // A wrapper around form_fetcher_.Fetch(), adding the call expectations.
+  void Fetch() {
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+    EXPECT_CALL(*mock_store_, GetSiteStatsMock(_))
+        .WillOnce(Return(std::vector<InteractionsStats*>()));
+#endif
+    EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
+    form_fetcher_->Fetch();
+    base::RunLoop().RunUntilIdle();
+    testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+  }
+
+  base::MessageLoop message_loop_;  // Used by mock_store_.
+  PasswordStore::FormDigest form_digest_;
+  std::unique_ptr<FormFetcherImpl> form_fetcher_;
+  MockConsumer consumer_;
+  scoped_refptr<MockPasswordStore> mock_store_;
   FakePasswordManagerClient client_;
-  FormFetcherImpl form_fetcher_;
-  testing::NiceMock<MockConsumer> consumer_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FormFetcherImplTest);
 };
 
-// Check that no PasswordStore results are handled correctly.
+// Check that the absence of PasswordStore results is handled correctly.
 TEST_F(FormFetcherImplTest, NoStoreResults) {
+  Fetch();
   EXPECT_CALL(consumer_, ProcessMatches(_, _)).Times(0);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
-  form_fetcher_.AddConsumer(&consumer_);
-  EXPECT_EQ(FormFetcher::State::WAITING, form_fetcher_.GetState());
+  form_fetcher_->AddConsumer(&consumer_);
+  EXPECT_EQ(FormFetcher::State::WAITING, form_fetcher_->GetState());
 }
 
 // Check that empty PasswordStore results are handled correctly.
 TEST_F(FormFetcherImplTest, Empty) {
-  form_fetcher_.AddConsumer(&consumer_);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
+  Fetch();
+  form_fetcher_->AddConsumer(&consumer_);
   EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
-  form_fetcher_.SetResults(std::vector<std::unique_ptr<PasswordForm>>());
-  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_.GetState());
-  EXPECT_THAT(form_fetcher_.GetFederatedMatches(), IsEmpty());
+  form_fetcher_->OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<PasswordForm>>());
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_THAT(form_fetcher_->GetFederatedMatches(), IsEmpty());
 }
 
 // Check that non-federated PasswordStore results are handled correctly.
 TEST_F(FormFetcherImplTest, NonFederated) {
+  Fetch();
   PasswordForm non_federated = CreateNonFederated();
-  form_fetcher_.AddConsumer(&consumer_);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
+  form_fetcher_->AddConsumer(&consumer_);
   std::vector<std::unique_ptr<PasswordForm>> results;
   results.push_back(base::MakeUnique<PasswordForm>(non_federated));
   EXPECT_CALL(consumer_,
               ProcessMatches(UnorderedElementsAre(Pointee(non_federated)), 0u));
-  form_fetcher_.SetResults(std::move(results));
-  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_.GetState());
-  EXPECT_THAT(form_fetcher_.GetFederatedMatches(), IsEmpty());
+  form_fetcher_->OnGetPasswordStoreResults(std::move(results));
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_THAT(form_fetcher_->GetFederatedMatches(), IsEmpty());
 }
 
 // Check that federated PasswordStore results are handled correctly.
 TEST_F(FormFetcherImplTest, Federated) {
+  Fetch();
   PasswordForm federated = CreateFederated();
   PasswordForm android_federated = CreateAndroidFederated();
-  form_fetcher_.AddConsumer(&consumer_);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
+  form_fetcher_->AddConsumer(&consumer_);
   std::vector<std::unique_ptr<PasswordForm>> results;
   results.push_back(base::MakeUnique<PasswordForm>(federated));
   results.push_back(base::MakeUnique<PasswordForm>(android_federated));
   EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
-  form_fetcher_.SetResults(std::move(results));
-  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_.GetState());
+  form_fetcher_->OnGetPasswordStoreResults(std::move(results));
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
   EXPECT_THAT(
-      form_fetcher_.GetFederatedMatches(),
+      form_fetcher_->GetFederatedMatches(),
       UnorderedElementsAre(Pointee(federated), Pointee(android_federated)));
 }
 
 // Check that mixed PasswordStore results are handled correctly.
 TEST_F(FormFetcherImplTest, Mixed) {
+  Fetch();
   PasswordForm federated1 = CreateFederated();
   federated1.username_value = ASCIIToUTF16("user");
   PasswordForm federated2 = CreateFederated();
@@ -201,8 +237,7 @@ TEST_F(FormFetcherImplTest, Mixed) {
   PasswordForm non_federated3 = CreateNonFederated();
   non_federated3.username_value = ASCIIToUTF16("user_D");
 
-  form_fetcher_.AddConsumer(&consumer_);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
+  form_fetcher_->AddConsumer(&consumer_);
   std::vector<std::unique_ptr<PasswordForm>> results;
   results.push_back(base::MakeUnique<PasswordForm>(federated1));
   results.push_back(base::MakeUnique<PasswordForm>(federated2));
@@ -215,15 +250,16 @@ TEST_F(FormFetcherImplTest, Mixed) {
                                                   Pointee(non_federated2),
                                                   Pointee(non_federated3)),
                              0u));
-  form_fetcher_.SetResults(std::move(results));
-  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_.GetState());
-  EXPECT_THAT(form_fetcher_.GetFederatedMatches(),
+  form_fetcher_->OnGetPasswordStoreResults(std::move(results));
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_THAT(form_fetcher_->GetFederatedMatches(),
               UnorderedElementsAre(Pointee(federated1), Pointee(federated2),
                                    Pointee(federated3)));
 }
 
 // Check that PasswordStore results are filtered correctly.
 TEST_F(FormFetcherImplTest, Filtered) {
+  Fetch();
   PasswordForm federated = CreateFederated();
   federated.username_value = ASCIIToUTF16("user");
   PasswordForm non_federated1 = CreateNonFederated();
@@ -234,8 +270,7 @@ TEST_F(FormFetcherImplTest, Filtered) {
   // Set up a filter to remove all credentials with the username "user".
   client_.set_filter(base::MakeUnique<NameFilter>("user"));
 
-  form_fetcher_.AddConsumer(&consumer_);
-  form_fetcher_.set_state(FormFetcher::State::WAITING);
+  form_fetcher_->AddConsumer(&consumer_);
   std::vector<std::unique_ptr<PasswordForm>> results;
   results.push_back(base::MakeUnique<PasswordForm>(federated));
   results.push_back(base::MakeUnique<PasswordForm>(non_federated1));
@@ -245,20 +280,88 @@ TEST_F(FormFetcherImplTest, Filtered) {
   EXPECT_CALL(consumer_,
               ProcessMatches(UnorderedElementsAre(Pointee(non_federated2)),
                              kNumFiltered));
-  form_fetcher_.SetResults(std::move(results));
-  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_.GetState());
+  form_fetcher_->OnGetPasswordStoreResults(std::move(results));
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
   // However, federated results should not be filtered.
-  EXPECT_THAT(form_fetcher_.GetFederatedMatches(),
+  EXPECT_THAT(form_fetcher_->GetFederatedMatches(),
               UnorderedElementsAre(Pointee(federated)));
 }
 
 // Check that stats from PasswordStore are handled correctly.
 TEST_F(FormFetcherImplTest, Stats) {
-  form_fetcher_.AddConsumer(&consumer_);
+  Fetch();
+  form_fetcher_->AddConsumer(&consumer_);
   std::vector<std::unique_ptr<InteractionsStats>> stats;
   stats.push_back(base::MakeUnique<InteractionsStats>());
-  form_fetcher_.SetStats(std::move(stats));
-  EXPECT_EQ(1u, form_fetcher_.GetInteractionsStats().size());
+  form_fetcher_->OnGetSiteStatistics(std::move(stats));
+  EXPECT_EQ(1u, form_fetcher_->GetInteractionsStats().size());
+}
+
+// Test that multiple calls of Fetch() are handled gracefully, and that they
+// always result in passing the most up-to-date information to the consumers.
+TEST_F(FormFetcherImplTest, Update_Reentrance) {
+  Fetch();
+  form_fetcher_->AddConsumer(&consumer_);
+  // The fetcher is currently waiting for a store response, after it fired a
+  // GetLogins request during the Fetch() above. The second and third Fetch
+  // (below) won't cause a GetLogins right now, but will ensure that a second
+  // GetLogins will be called later.
+  form_fetcher_->Fetch();
+  form_fetcher_->Fetch();
+
+  // First response from the store, should be ignored.
+  PasswordForm form_a = CreateNonFederated();
+  form_a.username_value = ASCIIToUTF16("a@gmail.com");
+  std::vector<std::unique_ptr<PasswordForm>> old_results;
+  old_results.push_back(base::MakeUnique<PasswordForm>(form_a));
+  // Because of the pending updates, the old PasswordStore results are not
+  // forwarded to the consumers.
+  EXPECT_CALL(consumer_, ProcessMatches(_, _)).Times(0);
+  // Delivering the first results will trigger the new GetLogins call, because
+  // of the Fetch() above.
+  EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
+  form_fetcher_->OnGetPasswordStoreResults(std::move(old_results));
+
+  // Second response from the store should not be ignored.
+  PasswordForm form_b = CreateNonFederated();
+  form_b.username_value = ASCIIToUTF16("b@gmail.com");
+
+  PasswordForm form_c = CreateNonFederated();
+  form_c.username_value = ASCIIToUTF16("c@gmail.com");
+
+  EXPECT_CALL(consumer_,
+              ProcessMatches(
+                  UnorderedElementsAre(Pointee(form_b), Pointee(form_c)), 0u));
+  std::vector<std::unique_ptr<PasswordForm>> results;
+  results.push_back(base::MakeUnique<PasswordForm>(form_b));
+  results.push_back(base::MakeUnique<PasswordForm>(form_c));
+  form_fetcher_->OnGetPasswordStoreResults(std::move(results));
+}
+
+#if !defined(OS_IOS) && !defined(OS_ANDROID)
+TEST_F(FormFetcherImplTest, FetchStatistics) {
+  InteractionsStats stats;
+  stats.origin_domain = form_digest_.origin.GetOrigin();
+  stats.username_value = ASCIIToUTF16("some username");
+  stats.dismissal_count = 5;
+  std::vector<InteractionsStats*> db_stats;
+  db_stats.push_back(new InteractionsStats(stats));
+  EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
+  EXPECT_CALL(*mock_store_, GetSiteStatsMock(stats.origin_domain))
+      .WillOnce(Return(db_stats));
+  form_fetcher_->Fetch();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_THAT(form_fetcher_->GetInteractionsStats(),
+              UnorderedElementsAre(Pointee(stats)));
+}
+#else
+TEST_F(FormFetcherImplTest, DontFetchStatistics) {
+  EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
+  EXPECT_CALL(*mock_store_, GetSiteStatsMock(_)).Times(0);
+  form_fetcher_->Fetch();
+  base::RunLoop().RunUntilIdle();
 }
+#endif
 
 }  // namespace password_manager