Reland "Filter out credentials with non-matching schemes"

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index d1f35e2b919dfc50bf189b55899d35c27e75c08a..a87ad075ccfce431e028935bdc1b1e824adde07e 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -462,82 +462,89 @@ void PasswordFormManager::SetSubmittedForm(const autofill::PasswordForm& form) {
   }
 }
 
-void PasswordFormManager::ProcessMatches(
-    const std::vector<const PasswordForm*>& non_federated,
-    size_t filtered_count) {
+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();
-  blacklisted_matches_.clear();
-  new_blacklisted_.reset();
 
-  std::unique_ptr<BrowserSavePasswordProgressLogger> logger;
-  if (password_manager_util::IsLoggingActive(client_)) {
-    logger.reset(
-        new BrowserSavePasswordProgressLogger(client_->GetLogManager()));
-    logger->LogMessage(Logger::STRING_PROCESS_MATCHES_METHOD);
-  }
+  if (matches.empty())
+    return;
+
+  // Compute scores.
+  std::vector<uint32_t> credential_scores(matches.size());
+  std::transform(
+      matches.begin(), matches.end(), credential_scores.begin(),
+      [this](const PasswordForm* match) { return ScoreResult(*match); });

[dvadym, 2016/09/05 10:58:05]

Great, I like this STL style is better that it was previously with loops.

[vabr (Chromium), 2016/09/05 11:39:10]

Thanks, I also find it better in the way it lets the specific code stand out in
the lambda at the end.

+
+  const uint32_t best_score =
+      *std::max_element(credential_scores.begin(), credential_scores.end());
 
-  // Create a copy of |non_federated| which we can reorder and prune.
-  std::vector<const PasswordForm*> all_matches(non_federated);
-
-  // The next step is to reorder |all_matches| into three consequent blocks:
-  // (A) relevant blacklist entries
-  // (B) non-relevant blacklist entries
-  // (C) non-blacklisted matches
-
-  auto begin_nonblacklisted =  // start of block (C)
-      std::partition(
-          all_matches.begin(), all_matches.end(),
-          [](const PasswordForm* form) { return form->blacklisted_by_user; });
-
-  auto begin_nonrelevant =  // start of block (B)
-      std::partition(
-          all_matches.begin(), begin_nonblacklisted,
-          [this](const PasswordForm* form) { return IsBlacklistMatch(*form); });
-
-  // Now compute scores for forms in block (C).
-  const size_t non_blacklist_count = all_matches.end() - begin_nonblacklisted;
-  std::vector<uint32_t> credential_scores;  // scores for forms from (C)
-  credential_scores.reserve(non_blacklist_count);
-  uint32_t best_score = 0;
   std::map<base::string16, uint32_t> best_scores;  // best scores for usernames
-  for (auto it = begin_nonblacklisted; it != all_matches.end(); ++it) {
-    const PasswordForm& login = **it;
-    uint32_t current_score = ScoreResult(login);
-    best_score = std::max(best_score, current_score);
-    best_scores[login.username_value] =
-        std::max(best_scores[login.username_value], current_score);
-    credential_scores.push_back(current_score);
+
+  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]);
   }
 
-  not_best_matches_.reserve(non_blacklist_count - best_scores.size());
+  // Assign best, non-best and preferred matches.
+  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 < non_blacklist_count; ++i) {
-    const PasswordForm* login = *(begin_nonblacklisted + i);
-    DCHECK(!login->blacklisted_by_user);
-    const base::string16& username = login->username_value;
+  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(login);
+      not_best_matches_.push_back(match);
       continue;
     }
 
     if (!preferred_match_ && credential_scores[i] == best_score)
-      preferred_match_ = login;
+      preferred_match_ = match;
 
     // If there is another best-score match for the same username then leave it
     // and add the current form to |not_best_matches_|.
     auto best_match_username = best_matches_.find(username);
     if (best_match_username == best_matches_.end()) {
-      best_matches_.insert(std::make_pair(username, login));
+      best_matches_.insert(std::make_pair(username, match));
     } else {
-      not_best_matches_.push_back(login);
+      not_best_matches_.push_back(match);
     }
   }
+}
+
+void PasswordFormManager::ProcessMatches(
+    const std::vector<const PasswordForm*>& non_federated,
+    size_t filtered_count) {
+  blacklisted_matches_.clear();
+  new_blacklisted_.reset();
+
+  std::unique_ptr<BrowserSavePasswordProgressLogger> logger;
+  if (password_manager_util::IsLoggingActive(client_)) {
+    logger.reset(
+        new BrowserSavePasswordProgressLogger(client_->GetLogManager()));
+    logger->LogMessage(Logger::STRING_PROCESS_MATCHES_METHOD);
+  }
 
-  all_matches.erase(begin_nonrelevant, all_matches.end());
-  blacklisted_matches_ = std::move(all_matches);
+  // 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(),
+               [this](const PasswordForm* form) { return IsMatch(*form); });
+  ScoreMatches(matches);
+
+  // Copy out blacklisted matches.
+  blacklisted_matches_.resize(std::count_if(
+      non_federated.begin(), non_federated.end(),
+      [this](const PasswordForm* form) { return IsBlacklistMatch(*form); }));
+  std::copy_if(
+      non_federated.begin(), non_federated.end(), blacklisted_matches_.begin(),
+      [this](const PasswordForm* form) { return IsBlacklistMatch(*form); });
 
   UMA_HISTOGRAM_COUNTS(
       "PasswordManager.NumPasswordsNotShown",
@@ -1165,14 +1172,20 @@ uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
   return score;
 }
 
+bool PasswordFormManager::IsMatch(const autofill::PasswordForm& form) const {
+  return !form.blacklisted_by_user && form.scheme == observed_form_.scheme;
+}
+
 bool PasswordFormManager::IsBlacklistMatch(
     const autofill::PasswordForm& blacklisted_form) const {
-  DCHECK(blacklisted_form.blacklisted_by_user);
-
-  if (blacklisted_form.is_public_suffix_match)
-    return false;
-  if (blacklisted_form.origin.GetOrigin() != observed_form_.origin.GetOrigin())
+  if (!blacklisted_form.blacklisted_by_user ||
+      blacklisted_form.is_public_suffix_match ||
+      blacklisted_form.scheme != observed_form_.scheme ||
+      blacklisted_form.origin.GetOrigin() !=

[dvadym, 2016/09/05 10:58:05]

Acknowledgement, removing comparison of submit/username/password elements, they
are mostly garbage

[vabr (Chromium), 2016/09/05 11:39:10]

There might be a misunderstanding here. I don't think I removed any checks about
elements (they should be still below). What I did here was turn the DCHECK into
a real condition, before I wanted this method to handle that case now. I also
added the PasswordForm::Scheme check and squashed all these early exits into one
if-return statement.

[dvadym, 2016/09/05 11:48:37]

Ah, sorry, I misinterpered changes. Acknowledge your changes.

+          observed_form_.origin.GetOrigin()) {
     return false;
+  }
+
   if (observed_form_.scheme == PasswordForm::SCHEME_HTML) {
     return (blacklisted_form.origin.path() == observed_form_.origin.path()) ||
            (AreStringsEqualOrEmpty(blacklisted_form.submit_element,