FormFetcher::GetBestMatches: return a vector instead of a map

This is a followup to crrev.com/c/1844969.
GetBestMatches now returns the best matches in a vector, instead of in
a map keyed by username.
For now, PasswordFormManager still sorts the matches by username;
that'll be addressed in another followup.

TBRing simple call site update in website_login_fetcher_impl.cc
TBR=arbesser@google.com

Bug: 1011399
Change-Id: Iaefbe3a29603ea319adcbeddadd6b908d029ab74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845225Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703717}

chrome/browser/ui/passwords/manage_passwords_state_unittest.cc

@@ -116,7 +116,7 @@ ManagePasswordsStateTest::CreateFormManager(
     const std::vector<const PasswordForm*>& federated_matches) {
   auto form_manager = std::make_unique<MockPasswordFormManagerForUI>();
   EXPECT_CALL(*form_manager, GetBestMatches())
-      .WillOnce(testing::ReturnRef(*best_matches));
+      .WillOnce(testing::Return(*best_matches));
   EXPECT_CALL(*form_manager, GetFederatedMatches())
       .WillOnce(Return(federated_matches));
   EXPECT_CALL(*form_manager, GetOrigin())

components/autofill_assistant/browser/website_login_fetcher_impl.cc

@@ -85,9 +85,9 @@ class WebsiteLoginFetcherImpl::PendingFetchLoginsRequest
   // From PendingRequest:
   void OnFetchCompleted() override {
     std::vector<Login> logins;
-    for (const auto& match : form_fetcher_->GetBestMatches()) {
-      logins.emplace_back(match.second->origin,
-                          base::UTF16ToUTF8(match.second->username_value));
+    for (const auto* match : form_fetcher_->GetBestMatches()) {
+      logins.emplace_back(match->origin,
+                          base::UTF16ToUTF8(match->username_value));
     }
     std::move(callback_).Run(logins);
     PendingRequest::OnFetchCompleted();

components/password_manager/core/browser/fake_form_fetcher.cc

@@ -53,8 +53,8 @@ const std::vector<const PasswordForm*>& FakeFormFetcher::GetAllRelevantMatches()
   return non_federated_same_scheme_;
 }
 
-const std::map<base::string16, const PasswordForm*>&
-FakeFormFetcher::GetBestMatches() const {
+const std::vector<const PasswordForm*>& FakeFormFetcher::GetBestMatches()
+    const {
   return best_matches_;
 }
 
@@ -65,15 +65,10 @@ const PasswordForm* FakeFormFetcher::GetPreferredMatch() const {
 void FakeFormFetcher::SetNonFederated(
     const std::vector<const PasswordForm*>& non_federated) {
   non_federated_ = non_federated;
-  std::vector<const PasswordForm*> best_matches;
   password_manager_util::FindBestMatches(
       non_federated_, scheme_,
       /*sort_matches_by_date_last_used=*/false, &non_federated_same_scheme_,
-      &best_matches, &preferred_match_);
-  best_matches_.clear();
-  for (const auto* match : best_matches) {
-    best_matches_[match->username_value] = match;
-  }
+      &best_matches_, &preferred_match_);
 }
 
 void FakeFormFetcher::SetBlacklisted(bool is_blacklisted) {

components/password_manager/core/browser/fake_form_fetcher.h

@@ -62,8 +62,8 @@ class FakeFormFetcher : public FormFetcher {
   const std::vector<const autofill::PasswordForm*>& GetAllRelevantMatches()
       const override;
 
-  const std::map<base::string16, const autofill::PasswordForm*>&
-  GetBestMatches() const override;
+  const std::vector<const autofill::PasswordForm*>& GetBestMatches()
+      const override;
 
   const autofill::PasswordForm* GetPreferredMatch() const override;
 
@@ -95,7 +95,7 @@ class FakeFormFetcher : public FormFetcher {
   std::vector<const autofill::PasswordForm*> non_federated_;
   std::vector<const autofill::PasswordForm*> federated_;
   std::vector<const autofill::PasswordForm*> non_federated_same_scheme_;
-  std::map<base::string16, const autofill::PasswordForm*> best_matches_;
+  std::vector<const autofill::PasswordForm*> best_matches_;
   const autofill::PasswordForm* preferred_match_ = nullptr;
   bool is_blacklisted_ = false;
 

components/password_manager/core/browser/form_fetcher.h

@@ -5,7 +5,6 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_FORM_FETCHER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_FORM_FETCHER_H_
 
-#include <map>
 #include <memory>
 #include <vector>
 
@@ -83,10 +82,10 @@ class FormFetcher {
   GetAllRelevantMatches() const = 0;
 
   // Nonblacklisted matches obtained from the backend.
-  virtual const std::map<base::string16, const autofill::PasswordForm*>&
-  GetBestMatches() const = 0;
+  virtual const std::vector<const autofill::PasswordForm*>& GetBestMatches()
+      const = 0;
 
-  // Pointer to an preferred entry in the map returned by GetBestMatches().
+  // Pointer to a preferred entry in the vector returned by GetBestMatches().
   virtual const autofill::PasswordForm* GetPreferredMatch() const = 0;
 
   // Fetches stored matching logins. In addition the statistics is fetched on

components/password_manager/core/browser/form_fetcher_impl.cc

@@ -116,8 +116,8 @@ const std::vector<const PasswordForm*>& FormFetcherImpl::GetAllRelevantMatches()
   return non_federated_same_scheme_;
 }
 
-const std::map<base::string16, const PasswordForm*>&
-FormFetcherImpl::GetBestMatches() const {
+const std::vector<const PasswordForm*>& FormFetcherImpl::GetBestMatches()
+    const {
   return best_matches_;
 }
 
@@ -216,15 +216,10 @@ std::unique_ptr<FormFetcher> FormFetcherImpl::Clone() {
   result->non_federated_ = MakeCopies(non_federated_);
   result->federated_ = MakeCopies(federated_);
   result->is_blacklisted_ = is_blacklisted_;
-  std::vector<const PasswordForm*> best_matches;
   password_manager_util::FindBestMatches(
       MakeWeakCopies(result->non_federated_), form_digest_.scheme,
       sort_matches_by_date_last_used_, &result->non_federated_same_scheme_,
-      &best_matches, &result->preferred_match_);
-  result->best_matches_.clear();
-  for (const auto* match : best_matches) {
-    result->best_matches_[match->username_value] = match;
-  }
+      &result->best_matches_, &result->preferred_match_);
 
   result->interactions_stats_ = interactions_stats_;
   result->state_ = state_;
@@ -239,15 +234,10 @@ void FormFetcherImpl::ProcessPasswordStoreResults(
   state_ = State::NOT_WAITING;
   SplitResults(std::move(results));
 
-  std::vector<const PasswordForm*> best_matches;
   password_manager_util::FindBestMatches(
       MakeWeakCopies(non_federated_), form_digest_.scheme,
       sort_matches_by_date_last_used_, &non_federated_same_scheme_,
-      &best_matches, &preferred_match_);
-  best_matches_.clear();
-  for (const auto* match : best_matches) {
-    best_matches_[match->username_value] = match;
-  }
+      &best_matches_, &preferred_match_);
 
   for (auto* consumer : consumers_)
     consumer->OnFetchCompleted();

components/password_manager/core/browser/form_fetcher_impl.h

@@ -57,8 +57,8 @@ class FormFetcherImpl : public FormFetcher,
   const std::vector<const autofill::PasswordForm*>& GetAllRelevantMatches()
       const override;
 
-  const std::map<base::string16, const autofill::PasswordForm*>&
-  GetBestMatches() const override;
+  const std::vector<const autofill::PasswordForm*>& GetBestMatches()
+      const override;
 
   const autofill::PasswordForm* GetPreferredMatch() const override;
 
@@ -112,8 +112,8 @@ class FormFetcherImpl : public FormFetcher,
   std::vector<const autofill::PasswordForm*> non_federated_same_scheme_;
 
   // Set of nonblacklisted PasswordForms from the password store that best match
-  // the form being managed by |this|, indexed by username.
-  std::map<base::string16, const autofill::PasswordForm*> best_matches_;
+  // the form being managed by |this|.
+  std::vector<const autofill::PasswordForm*> best_matches_;
 
   // Convenience pointer to entry in |best_matches_| that is marked as
   // preferred. This is only allowed to be null if there are no best matches at

components/password_manager/core/browser/mock_password_form_manager_for_ui.h

@@ -18,9 +18,8 @@ class MockPasswordFormManagerForUI : public PasswordFormManagerForUI {
   ~MockPasswordFormManagerForUI() override;
 
   MOCK_CONST_METHOD0(GetOrigin, const GURL&());
-  MOCK_CONST_METHOD0(
-      GetBestMatches,
-      const std::map<base::string16, const autofill::PasswordForm*>&());
+  MOCK_CONST_METHOD0(GetBestMatches,
+                     std::map<base::string16, const autofill::PasswordForm*>());
   MOCK_CONST_METHOD0(GetFederatedMatches,
                      std::vector<const autofill::PasswordForm*>());
   MOCK_CONST_METHOD0(GetPendingCredentials, const autofill::PasswordForm&());

components/password_manager/core/browser/password_form_manager.cc

@@ -262,9 +262,12 @@ const GURL& PasswordFormManager::GetOrigin() const {
                                        : observed_form_.url;
 }
 
-const std::map<base::string16, const PasswordForm*>&
+std::map<base::string16, const PasswordForm*>
 PasswordFormManager::GetBestMatches() const {
-  return form_fetcher_->GetBestMatches();
+  std::map<base::string16, const PasswordForm*> best_matches;
+  for (const auto* match : form_fetcher_->GetBestMatches())
+    best_matches[match->username_value] = match;
+  return best_matches;
 }
 
 std::vector<const autofill::PasswordForm*>
@@ -316,8 +319,7 @@ void PasswordFormManager::Save() {
     SanitizePossibleUsernames(&pending_credentials_);
     pending_credentials_.date_created = base::Time::Now();
     votes_uploader_.SendVotesOnSave(observed_form_, *parsed_submitted_form_,
-                                    form_fetcher_->GetBestMatches(),
-                                    &pending_credentials_);
+                                    GetBestMatches(), &pending_credentials_);
     SavePendingToStore(false /*update*/);
   } else {
     ProcessUpdate();
@@ -789,9 +791,9 @@ void PasswordFormManager::Fill() {
 #endif
 
   SendFillInformationToRenderer(
-      client_, driver_.get(), *observed_password_form.get(),
-      form_fetcher_->GetBestMatches(), form_fetcher_->GetFederatedMatches(),
-      form_fetcher_->GetPreferredMatch(), metrics_recorder_.get());
+      client_, driver_.get(), *observed_password_form.get(), GetBestMatches(),
+      form_fetcher_->GetFederatedMatches(), form_fetcher_->GetPreferredMatch(),
+      metrics_recorder_.get());
 }
 
 void PasswordFormManager::FillForm(const FormData& observed_form) {
@@ -896,7 +898,7 @@ void PasswordFormManager::CreatePendingCredentials() {
 
   // Calculate the user's action based on existing matches and the submitted
   // form.
-  metrics_recorder_->CalculateUserAction(form_fetcher_->GetBestMatches(),
+  metrics_recorder_->CalculateUserAction(GetBestMatches(),
                                          *parsed_submitted_form_);
 
   // This function might be called multiple times so set variables that are
@@ -908,7 +910,7 @@ void PasswordFormManager::CreatePendingCredentials() {
   // Look for the actually submitted credentials in the list of previously saved
   // credentials that were available to autofilling.
   const PasswordForm* saved_form = password_manager_util::GetMatchForUpdating(
-      *parsed_submitted_form_, form_fetcher_->GetBestMatches());
+      *parsed_submitted_form_, GetBestMatches());
   if (saved_form) {
     // A similar credential exists in the store already.
     pending_credentials_ = *saved_form;
@@ -1047,9 +1049,8 @@ void PasswordFormManager::ProcessUpdate() {
   }
 
   if (pending_credentials_.times_used == 1) {
-    votes_uploader_.UploadFirstLoginVotes(form_fetcher_->GetBestMatches(),
-                                          pending_credentials_,
-                                          *parsed_submitted_form_);
+    votes_uploader_.UploadFirstLoginVotes(
+        GetBestMatches(), pending_credentials_, *parsed_submitted_form_);
   }
 }
 
@@ -1142,7 +1143,7 @@ void PasswordFormManager::CalculateFillingAssistanceMetric(
 
 void PasswordFormManager::SavePendingToStore(bool update) {
   const PasswordForm* saved_form = password_manager_util::GetMatchForUpdating(
-      *parsed_submitted_form_, form_fetcher_->GetBestMatches());
+      *parsed_submitted_form_, GetBestMatches());
   if ((update || password_overridden_) &&
       !pending_credentials_.IsFederatedCredential()) {
     DCHECK(saved_form);

components/password_manager/core/browser/password_form_manager.h

@@ -135,8 +135,8 @@ class PasswordFormManager : public PasswordFormManagerForUI,
 
   // PasswordFormManagerForUI:
   const GURL& GetOrigin() const override;
-  const std::map<base::string16, const autofill::PasswordForm*>&
-  GetBestMatches() const override;
+  std::map<base::string16, const autofill::PasswordForm*> GetBestMatches()
+      const override;
   std::vector<const autofill::PasswordForm*> GetFederatedMatches()
       const override;
   const autofill::PasswordForm& GetPendingCredentials() const override;

components/password_manager/core/browser/password_form_manager_for_ui.h

@@ -32,8 +32,8 @@ class PasswordFormManagerForUI {
   virtual const GURL& GetOrigin() const = 0;
 
   // Returns the best saved matches for the observed form.
-  // TODO(crbug.com/831123): it should return a vector.
-  virtual const std::map<base::string16, const autofill::PasswordForm*>&
+  // TODO(crbug.com/1011399): it should return a reference to a vector.
+  virtual std::map<base::string16, const autofill::PasswordForm*>
   GetBestMatches() const = 0;
 
   // Returns the federated saved matches for the observed form.

components/password_manager/core/browser/password_generation_state.cc

@@ -47,8 +47,7 @@ class PasswordDataForUI : public PasswordFormManagerForUI {
 
   // PasswordFormManagerForUI:
   const GURL& GetOrigin() const override;
-  const std::map<base::string16, const PasswordForm*>& GetBestMatches()
-      const override;
+  std::map<base::string16, const PasswordForm*> GetBestMatches() const override;
   std::vector<const PasswordForm*> GetFederatedMatches() const override;
   const PasswordForm& GetPendingCredentials() const override;
   metrics_util::CredentialSourceType GetCredentialSource() const override;
@@ -96,7 +95,7 @@ const GURL& PasswordDataForUI::GetOrigin() const {
   return pending_form_.origin;
 }
 
-const std::map<base::string16, const PasswordForm*>&
+std::map<base::string16, const PasswordForm*>
 PasswordDataForUI::GetBestMatches() const {
   return matches_;
 }