password_manager_util::GetMatchForUpdating: accept vector instead of map

This is mostly for consistency with GetBestMatches.

Bug: 1011399
Change-Id: I7369ee04a735c830b30d20bd1b826a66b77d759a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845714
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704555}

components/password_manager/core/browser/password_form_manager.cc

@@ -916,7 +916,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_, ByUsername(GetBestMatches()));
+      *parsed_submitted_form_, GetBestMatches());
   if (saved_form) {
     // A similar credential exists in the store already.
     pending_credentials_ = *saved_form;
@@ -1150,7 +1150,7 @@ void PasswordFormManager::CalculateFillingAssistanceMetric(
 
 void PasswordFormManager::SavePendingToStore(bool update) {
   const PasswordForm* saved_form = password_manager_util::GetMatchForUpdating(
-      *parsed_submitted_form_, ByUsername(GetBestMatches()));
+      *parsed_submitted_form_, GetBestMatches());
   if ((update || password_overridden_) &&
       !pending_credentials_.IsFederatedCredential()) {
     DCHECK(saved_form);

components/password_manager/core/browser/password_manager_util.cc

@@ -60,6 +60,18 @@ bool IsBetterMatchUsingLastUsed(const PasswordForm* lhs,
          std::make_pair(!rhs->is_public_suffix_match, rhs->date_last_used);
 }
 
+// Returns a form with the given |username_value| from |credentials|, or nullptr
+// if none exists.
+const PasswordForm* FindByUsername(
+    const std::vector<const PasswordForm*>& credentials,
+    const base::string16& username_value) {
+  for (const auto* form : credentials) {
+    if (form->username_value == username_value)
+      return form;
+  }
+  return nullptr;
+}
+
 }  // namespace
 
 // Update |credential| to reflect usage.
@@ -261,7 +273,7 @@ void FindBestMatches(
 
 const PasswordForm* GetMatchForUpdating(
     const PasswordForm& submitted_form,
-    const std::map<base::string16, const PasswordForm*>& credentials) {
+    const std::vector<const PasswordForm*>& credentials) {
   // This is the case for the credential management API. It should not depend on
   // form managers. Once that's the case, this should be turned into a DCHECK.
   // TODO(crbug/947030): turn it into a DCHECK.
@@ -269,10 +281,11 @@ const PasswordForm* GetMatchForUpdating(
     return nullptr;
 
   // Try to return form with matching |username_value|.
-  auto it = credentials.find(submitted_form.username_value);
-  if (it != credentials.end()) {
-    if (!it->second->is_public_suffix_match)
-      return it->second;
+  const PasswordForm* username_match =
+      FindByUsername(credentials, submitted_form.username_value);
+  if (username_match) {
+    if (!username_match->is_public_suffix_match)
+      return username_match;
 
     const auto& password_to_save = submitted_form.new_password_value.empty()
                                        ? submitted_form.password_value
@@ -286,24 +299,25 @@ const PasswordForm* GetMatchForUpdating(
     // that the autofilled credentials and |submitted_password_form|
     // actually correspond to two different accounts (see
     // http://crbug.com/385619).
-    return password_to_save == it->second->password_value ? it->second
-                                                          : nullptr;
+    return password_to_save == username_match->password_value ? username_match
+                                                              : nullptr;
   }
 
   // Next attempt is to find a match by password value. It should not be tried
   // when the username was actually detected.
   if (submitted_form.type == PasswordForm::Type::kApi ||
-      !submitted_form.username_value.empty())
+      !submitted_form.username_value.empty()) {
     return nullptr;
+  }
 
-  for (const auto& stored_match : credentials) {
-    if (stored_match.second->password_value == submitted_form.password_value)
-      return stored_match.second;
+  for (const PasswordForm* stored_match : credentials) {
+    if (stored_match->password_value == submitted_form.password_value)
+      return stored_match;
   }
 
   // Last try. The submitted form had no username but a password. Assume that
   // it's an existing credential.
-  return credentials.empty() ? nullptr : credentials.begin()->second;
+  return credentials.empty() ? nullptr : credentials.front();
 }
 
 autofill::PasswordForm MakeNormalizedBlacklistedForm(

components/password_manager/core/browser/password_manager_util.h

@@ -5,7 +5,6 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_UTIL_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_UTIL_H_
 
-#include <map>
 #include <memory>
 #include <vector>
 
@@ -126,7 +125,7 @@ void FindBestMatches(
 // PSL and Android matches.
 const autofill::PasswordForm* GetMatchForUpdating(
     const autofill::PasswordForm& submitted_form,
-    const std::map<base::string16, const autofill::PasswordForm*>& credentials);
+    const std::vector<const autofill::PasswordForm*>& credentials);
 
 // This method creates a blacklisted form with |digests|'s scheme, signon_realm
 // and origin. This is done to avoid storing PII and to have a normalized unique

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -67,16 +67,6 @@ autofill::PasswordForm GetTestProxyCredential() {
   return form;
 }
 
-std::map<base::string16, const autofill::PasswordForm*> MapFromCredentials(
-    const std::vector<const autofill::PasswordForm*>& forms) {
-  std::map<base::string16, const autofill::PasswordForm*> result;
-  for (const autofill::PasswordForm* form : forms) {
-    auto inserted = result.emplace(form->username_value, form);
-    EXPECT_TRUE(inserted.second);
-  }
-  return result;
-}
-
 }  // namespace
 
 using password_manager::UnorderedPasswordFormElementsAre;
@@ -390,8 +380,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_MatchUsername) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.password_value = base::ASCIIToUTF16("new_password");
 
-  EXPECT_EQ(&stored,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(&stored, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_RejectUnknownUsername) {
@@ -399,8 +388,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_RejectUnknownUsername) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.username_value = base::ASCIIToUTF16("other_username");
 
-  EXPECT_EQ(nullptr,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(nullptr, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_FederatedCredential) {
@@ -409,8 +397,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_FederatedCredential) {
   parsed.password_value.clear();
   parsed.federation_origin = url::Origin::Create(GURL(kTestFederationURL));
 
-  EXPECT_EQ(nullptr,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(nullptr, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_MatchUsernamePSL) {
@@ -418,8 +405,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_MatchUsernamePSL) {
   stored.is_public_suffix_match = true;
   autofill::PasswordForm parsed = GetTestCredential();
 
-  EXPECT_EQ(&stored,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(&stored, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_MatchUsernamePSLAnotherPassword) {
@@ -428,8 +414,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_MatchUsernamePSLAnotherPassword) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.password_value = base::ASCIIToUTF16("new_password");
 
-  EXPECT_EQ(nullptr,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(nullptr, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil,
@@ -440,8 +425,7 @@ TEST(PasswordManagerUtil,
   parsed.new_password_value = parsed.password_value;
   parsed.password_value.clear();
 
-  EXPECT_EQ(&stored,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(&stored, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil,
@@ -452,8 +436,7 @@ TEST(PasswordManagerUtil,
   parsed.new_password_value = base::ASCIIToUTF16("new_password");
   parsed.password_value.clear();
 
-  EXPECT_EQ(nullptr,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(nullptr, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameFindByPassword) {
@@ -461,8 +444,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameFindByPassword) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.username_value.clear();
 
-  EXPECT_EQ(&stored,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(&stored, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameFindByPasswordPSL) {
@@ -471,8 +453,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameFindByPasswordPSL) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.username_value.clear();
 
-  EXPECT_EQ(&stored,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(&stored, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameCMAPI) {
@@ -483,8 +464,7 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernameCMAPI) {
 
   // In case of the Credential Management API we know for sure that the site
   // meant empty username. Don't try any other heuristics.
-  EXPECT_EQ(nullptr,
-            GetMatchForUpdating(parsed, MapFromCredentials({&stored})));
+  EXPECT_EQ(nullptr, GetMatchForUpdating(parsed, {&stored}));
 }
 
 TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernamePickFirst) {
@@ -501,10 +481,9 @@ TEST(PasswordManagerUtil, GetMatchForUpdating_EmptyUsernamePickFirst) {
   autofill::PasswordForm parsed = GetTestCredential();
   parsed.username_value.clear();
 
-  // The credential with the first username is picked.
-  EXPECT_EQ(&stored1,
-            GetMatchForUpdating(
-                parsed, MapFromCredentials({&stored3, &stored2, &stored1})));
+  // The first credential is picked (arbitrarily).
+  EXPECT_EQ(&stored3,
+            GetMatchForUpdating(parsed, {&stored3, &stored2, &stored1}));
 }
 
 TEST(PasswordManagerUtil, MakeNormalizedBlacklistedForm_Android) {