Move & Simplify algorithm for finding best matches in Password Manager.

NewPasswordFormManager (details go/new-cpm-design-refactoring) needs
to find best matches. This CL implements new best matches finding
algorithm, that will be used for now by both PasswordFormManager
and NewPasswordFormManager.

Scoring algorithm is greatly simplified. All that we need to compare
matches is PSL/non-PSL match, preferred/non-preferred.

Since now algorithm of finding best matches is in separate function,
we can test it. So tests were written.

Bug: 831123
Change-Id: I543aa185e489777007c0c19bcc5924f9964696b6
Reviewed-on: https://chromium-review.googlesource.com/1023936
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553079}

components/password_manager/core/browser/password_form_manager.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <algorithm>
+#include <iterator>
 #include <map>
 #include <memory>
 #include <utility>
@@ -574,60 +575,6 @@ void PasswordFormManager::SaveSubmittedFormTypeForMetrics(
   metrics_recorder_->SetSubmittedFormType(type);
 }
 
-void PasswordFormManager::ScoreMatches(
-    const std::vector<const PasswordForm*>& matches) {
-  DCHECK(std::all_of(
-      matches.begin(), matches.end(),
-      [](const PasswordForm* match) { return !match->blacklisted_by_user; }));
-
-  preferred_match_ = nullptr;
-  best_matches_.clear();
-  not_best_matches_.clear();
-
-  if (matches.empty())
-    return;
-
-  // Compute scores.
-  std::vector<uint32_t> credential_scores(matches.size());
-  for (size_t i = 0; i < matches.size(); ++i)
-    credential_scores[i] = ScoreResult(*matches[i]);
-
-  const uint32_t best_score =
-      *std::max_element(credential_scores.begin(), credential_scores.end());
-
-  // Compute best score for each username.
-  std::map<base::string16, uint32_t> best_scores;
-  for (size_t i = 0; i < matches.size(); ++i) {
-    uint32_t& score = best_scores[matches[i]->username_value];
-    score = std::max(score, credential_scores[i]);
-  }
-
-  // Find the best match for each username, move the rest to
-  // |non_best_matches_|. Also assign the overall best match to
-  // |preferred_match_|.
-  not_best_matches_.reserve(matches.size() - best_scores.size());
-  // Fill |best_matches_| with the best-scoring credentials for each username.
-  for (size_t i = 0; i < matches.size(); ++i) {
-    const PasswordForm* const match = matches[i];
-    const base::string16& username = match->username_value;
-
-    if (credential_scores[i] < best_scores[username]) {
-      not_best_matches_.push_back(match);
-      continue;
-    }
-
-    if (!preferred_match_ && credential_scores[i] == best_score)
-      preferred_match_ = match;
-
-    // If there is already another best-score match for the same username, leave
-    // it and add the current form to |not_best_matches_|.
-    if (best_matches_.find(username) != best_matches_.end())
-      not_best_matches_.push_back(match);
-    else
-      best_matches_.insert(std::make_pair(username, match));
-  }
-}
-
 void PasswordFormManager::ProcessMatches(
     const std::vector<const PasswordForm*>& non_federated,
     size_t filtered_count) {
@@ -642,19 +589,19 @@ void PasswordFormManager::ProcessMatches(
   }
 
   // Copy out and score non-blacklisted matches.
-  std::vector<const PasswordForm*> matches(std::count_if(
-      non_federated.begin(), non_federated.end(),
-      [this](const PasswordForm* form) { return IsMatch(*form); }));
-  std::copy_if(non_federated.begin(), non_federated.end(), matches.begin(),
+  std::vector<const PasswordForm*> matches;
+  std::copy_if(non_federated.begin(), non_federated.end(),
+               std::back_inserter(matches),
                [this](const PasswordForm* form) { return IsMatch(*form); });
-  ScoreMatches(matches);
+
+  password_manager_util::FindBestMatches(std::move(matches), &best_matches_,
+                                         &not_best_matches_, &preferred_match_);
 
   // Copy out blacklisted matches.
-  blacklisted_matches_.resize(std::count_if(
-      non_federated.begin(), non_federated.end(),
-      [this](const PasswordForm* form) { return IsBlacklistMatch(*form); }));
+  blacklisted_matches_.clear();
   std::copy_if(
-      non_federated.begin(), non_federated.end(), blacklisted_matches_.begin(),
+      non_federated.begin(), non_federated.end(),
+      std::back_inserter(blacklisted_matches_),
       [this](const PasswordForm* form) { return IsBlacklistMatch(*form); });
 
   UMA_HISTOGRAM_COUNTS(
@@ -1215,72 +1162,6 @@ void PasswordFormManager::CreatePendingCredentials() {
     pending_credentials_.type = PasswordForm::TYPE_GENERATED;
 }
 
-uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
-  DCHECK(!candidate.blacklisted_by_user);
-  // For scoring of candidate login data:
-  // The most important element that should match is the signon_realm followed
-  // by the origin, the action, the password name, the submit button name, and
-  // finally the username input field name.
-  // If public suffix origin match was not used, it gives an addition of
-  // 128 (1 << 7).
-  // Exact origin match gives an addition of 64 (1 << 6) + # of matching url
-  // dirs.
-  // Partial match gives an addition of 32 (1 << 5) + # matching url dirs
-  // That way, a partial match cannot trump an exact match even if
-  // the partial one matches all other attributes (action, elements) (and
-  // regardless of the matching depth in the URL path).
-
-  // When comparing path segments, only consider at most 63 of them, so that the
-  // potential gain from shared path prefix is not more than from an exact
-  // origin match.
-  const size_t kSegmentCountCap = 63;
-  const size_t capped_form_path_segment_count =
-      std::min(form_path_segments_.size(), kSegmentCountCap);
-
-  uint32_t score = 0u;
-  if (!candidate.is_public_suffix_match) {
-    score += 1u << 8;
-  }
-
-  if (candidate.preferred)
-    score += 1u << 7;
-
-  if (candidate.origin == observed_form_.origin) {
-    // This check is here for the most common case which
-    // is we have a single match in the db for the given host,
-    // so we don't generally need to walk the entire URL path (the else
-    // clause).
-    score += (1u << 6) + static_cast<uint32_t>(capped_form_path_segment_count);
-  } else {
-    // Walk the origin URL paths one directory at a time to see how
-    // deep the two match.
-    std::vector<std::string> candidate_path_segments =
-        SplitPathToSegments(candidate.origin.path());
-    size_t depth = 0u;
-    const size_t max_dirs = std::min(capped_form_path_segment_count,
-                                     candidate_path_segments.size());
-    while ((depth < max_dirs) &&
-           (form_path_segments_[depth] == candidate_path_segments[depth])) {
-      depth++;
-      score++;
-    }
-    // do we have a partial match?
-    score += (depth > 0u) ? 1u << 5 : 0u;
-  }
-  if (observed_form_.scheme == PasswordForm::SCHEME_HTML) {
-    if (candidate.action == observed_form_.action)
-      score += 1u << 3;
-    if (candidate.password_element == observed_form_.password_element)
-      score += 1u << 2;
-    if (candidate.submit_element == observed_form_.submit_element)
-      score += 1u << 1;
-    if (candidate.username_element == observed_form_.username_element)
-      score += 1u << 0;
-  }
-
-  return score;
-}
-
 bool PasswordFormManager::IsMatch(const autofill::PasswordForm& form) const {
   return !form.blacklisted_by_user && form.scheme == observed_form_.scheme;
 }

components/password_manager/core/browser/password_form_manager.h

@@ -329,20 +329,12 @@ class PasswordFormManager : public FormFetcher::Consumer {
   // Trigger filling of HTTP auth dialog and update |manager_action_|.
   void ProcessLoginPrompt();
 
-  // Given all non-blacklisted |matches|, computes their score and populates
-  // |best_matches_|, |preferred_match_| and |non_best_matches_| accordingly.
-  void ScoreMatches(const std::vector<const autofill::PasswordForm*>& matches);
-
   // Helper for Save in the case that best_matches.size() == 0, meaning
   // we have no prior record of this form/username/password and the user
   // has opted to 'Save Password'. The previously preferred login from
   // |best_matches_| will be reset.
   void SaveAsNewLogin();
 
-  // Helper for OnGetPasswordStoreResults to score an individual result
-  // against the observed_form_.
-  uint32_t ScoreResult(const autofill::PasswordForm& candidate) const;
-
   // Returns true iff |form| is a non-blacklisted match for |observed_form_|.
   bool IsMatch(const autofill::PasswordForm& form) const;
 

components/password_manager/core/browser/password_manager_util.cc

@@ -21,6 +21,8 @@
 #include "components/prefs/pref_service.h"
 #include "components/sync/driver/sync_service.h"
 
+using autofill::PasswordForm;
+
 namespace password_manager_util {
 namespace {
 
@@ -72,6 +74,15 @@ void StartCleaningBlacklisted(
   new BlacklistedCredentialsCleaner(store.get(), prefs);
 }
 
+// Return true if
+// 1.|lhs| is non-PSL match, |rhs| is PSL match or
+// 2.|lhs| and |rhs| have the same value of |is_public_suffix_match|, and |lhs|
+// is preferred while |rhs| is not preferred.
+bool IsBetterMatch(const PasswordForm* lhs, const PasswordForm* rhs) {
+  return std::make_pair(!lhs->is_public_suffix_match, lhs->preferred) >
+         std::make_pair(!rhs->is_public_suffix_match, rhs->preferred);
+}
+
 }  // namespace
 
 password_manager::PasswordSyncState GetPasswordSyncState(
@@ -202,4 +213,37 @@ void CleanUserDataInBlacklistedCredentials(
   }
 }
 
+void FindBestMatches(
+    std::vector<const PasswordForm*> matches,
+    std::map<base::string16, const PasswordForm*>* best_matches,
+    std::vector<const PasswordForm*>* not_best_matches,
+    const PasswordForm** preferred_match) {
+  DCHECK(std::all_of(
+      matches.begin(), matches.end(),
+      [](const PasswordForm* match) { return !match->blacklisted_by_user; }));
+  DCHECK(best_matches);
+  DCHECK(not_best_matches);
+  DCHECK(preferred_match);
+
+  *preferred_match = nullptr;
+  best_matches->clear();
+  not_best_matches->clear();
+
+  if (matches.empty())
+    return;
+
+  // Sort matches using IsBetterMatch predicate.
+  std::sort(matches.begin(), matches.end(), IsBetterMatch);
+  for (const auto* match : matches) {
+    const base::string16& username = match->username_value;
+    // The first match for |username| in the sorted array is best match.
+    if (best_matches->find(username) == best_matches->end())
+      best_matches->insert(std::make_pair(username, match));
+    else
+      not_best_matches->push_back(match);
+  }
+
+  *preferred_match = *matches.begin();
+}
+
 }  // namespace password_manager_util

components/password_manager/core/browser/password_manager_util.h

@@ -5,10 +5,12 @@
 #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>
 
 #include "base/callback.h"
+#include "base/strings/string16.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "ui/gfx/native_widget_types.h"
 
@@ -81,6 +83,18 @@ void CleanUserDataInBlacklistedCredentials(
     PrefService* prefs,
     int delay_in_seconds);
 
+// Given all non-blacklisted |matches|, finds and populates
+// |best_matches_|, |preferred_match_| and |non_best_matches_| accordingly.
+// For comparing credentials the following rule is used: non-psl match is better
+// than psl match, preferred match is better than non-preferred match. In case
+// of tie, an arbitrary credential from the tied ones is chosen for
+// |best_matches| and preferred_match.
+void FindBestMatches(
+    std::vector<const autofill::PasswordForm*> matches,
+    std::map<base::string16, const autofill::PasswordForm*>* best_matches,
+    std::vector<const autofill::PasswordForm*>* not_best_matches,
+    const autofill::PasswordForm** preferred_match);
+
 }  // namespace password_manager_util
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_UTIL_H_

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_task_environment.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_manager_test_utils.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
@@ -17,6 +18,8 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using autofill::PasswordForm;
+
 namespace password_manager_util {
 namespace {
 
@@ -128,4 +131,119 @@ TEST(PasswordManagerUtil, CleanBlacklistedUsernamePassword) {
   password_store->ShutdownOnUIThread();
 }
 
+TEST(PasswordManagerUtil, FindBestMatches) {
+  const int kNotFound = -1;
+  struct TestMatch {
+    bool is_psl_match;
+    bool preferred;
+    std::string username;
+  };
+  struct TestCase {
+    const char* description;
+    std::vector<TestMatch> matches;
+    int expected_preferred_match_index;
+    std::map<std::string, size_t> expected_best_matches_indices;
+  } test_cases[] = {
+      {"Empty matches", {}, kNotFound, {}},
+      {"1 preferred non-psl match",
+       {{.is_psl_match = false, .preferred = true, .username = "u"}},
+       0,
+       {{"u", 0}}},
+      {"1 non-preferred psl match",
+       {{.is_psl_match = true, .preferred = false, .username = "u"}},
+       0,
+       {{"u", 0}}},
+      {"2 matches with the same username",
+       {{.is_psl_match = false, .preferred = false, .username = "u"},
+        {.is_psl_match = false, .preferred = true, .username = "u"}},
+       1,
+       {{"u", 1}}},
+      {"2 matches with different usernames, preferred taken",
+       {{.is_psl_match = false, .preferred = false, .username = "u1"},
+        {.is_psl_match = false, .preferred = true, .username = "u2"}},
+       1,
+       {{"u1", 0}, {"u2", 1}}},
+      {"2 matches with different usernames, non-psl much taken",
+       {{.is_psl_match = false, .preferred = false, .username = "u1"},
+        {.is_psl_match = true, .preferred = true, .username = "u2"}},
+       0,
+       {{"u1", 0}, {"u2", 1}}},
+      {"8 matches, 3 usernames",
+       {{.is_psl_match = false, .preferred = false, .username = "u2"},
+        {.is_psl_match = true, .preferred = false, .username = "u3"},
+        {.is_psl_match = true, .preferred = false, .username = "u1"},
+        {.is_psl_match = false, .preferred = true, .username = "u3"},
+        {.is_psl_match = true, .preferred = false, .username = "u1"},
+        {.is_psl_match = false, .preferred = false, .username = "u2"},
+        {.is_psl_match = true, .preferred = true, .username = "u3"},
+        {.is_psl_match = false, .preferred = false, .username = "u1"}},
+       3,
+       {{"u1", 7}, {"u2", 0}, {"u3", 3}}},
+
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    SCOPED_TRACE(testing::Message("Test description: ")
+                 << test_case.description);
+    // Convert TestMatch to PasswordForm.
+    std::vector<PasswordForm> owning_matches;
+    for (const TestMatch match : test_case.matches) {
+      PasswordForm form;
+      form.is_public_suffix_match = match.is_psl_match;
+      form.preferred = match.preferred;
+      form.username_value = base::ASCIIToUTF16(match.username);
+      owning_matches.push_back(form);
+    }
+    std::vector<const PasswordForm*> matches;
+    for (const PasswordForm& match : owning_matches)
+      matches.push_back(&match);
+
+    std::map<base::string16, const PasswordForm*> best_matches;
+    std::vector<const PasswordForm*> not_best_matches;
+    const PasswordForm* preferred_match = nullptr;
+
+    FindBestMatches(matches, &best_matches, &not_best_matches,
+                    &preferred_match);
+
+    if (test_case.expected_preferred_match_index == kNotFound) {
+      // Case of empty |matches|.
+      EXPECT_FALSE(preferred_match);
+      EXPECT_TRUE(best_matches.empty());
+      EXPECT_TRUE(not_best_matches.empty());
+    } else {
+      // Check |preferred_match|.
+      EXPECT_EQ(matches[test_case.expected_preferred_match_index],
+                preferred_match);
+      // Check best matches.
+      ASSERT_EQ(test_case.expected_best_matches_indices.size(),
+                best_matches.size());
+
+      for (const auto& username_match : best_matches) {
+        std::string username = base::UTF16ToUTF8(username_match.first);
+        ASSERT_NE(test_case.expected_best_matches_indices.end(),
+                  test_case.expected_best_matches_indices.find(username));
+        size_t expected_index =
+            test_case.expected_best_matches_indices.at(username);
+        size_t actual_index = std::distance(
+            matches.begin(),
+            std::find(matches.begin(), matches.end(), username_match.second));
+        EXPECT_EQ(expected_index, actual_index);
+      }
+
+      // Check non-best matches.
+      ASSERT_EQ(matches.size(), best_matches.size() + not_best_matches.size());
+      for (const PasswordForm* form : not_best_matches) {
+        // A non-best match form must not be in |best_matches|.
+        EXPECT_NE(best_matches[form->username_value], form);
+
+        matches.erase(std::remove(matches.begin(), matches.end(), form),
+                      matches.end());
+      }
+      // Expect that all non-best matches were found in |matches| and only best
+      // matches left.
+      EXPECT_EQ(best_matches.size(), matches.size());
+    }
+  }
+}
+
 }  // namespace password_manager_util