Filter duplicates in the account chooser.

components/password_manager/core/browser/credential_manager_pending_request_task.cc

Index: components/password_manager/core/browser/credential_manager_pending_request_task.cc
diff --git a/components/password_manager/core/browser/credential_manager_pending_request_task.cc b/components/password_manager/core/browser/credential_manager_pending_request_task.cc
index 96a5ed29ea91e8e1a698653f97c98e5910250ddf..5c1e6e9bb69bbc33f6116e32e6bccf6b4289a212 100644
--- a/components/password_manager/core/browser/credential_manager_pending_request_task.cc
+++ b/components/password_manager/core/browser/credential_manager_pending_request_task.cc
@@ -5,6 +5,7 @@
 #include "components/password_manager/core/browser/credential_manager_pending_request_task.h"
 
 #include <algorithm>
+#include <map>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
@@ -24,27 +25,45 @@ namespace {
 
 // Send a UMA histogram about if |local_results| has empty or duplicate
 // usernames.
-void ReportAccountChooserMetrics(
-    const ScopedVector<autofill::PasswordForm>& local_results,
-    bool had_empty_username) {
-  std::vector<base::string16> usernames;
-  for (const auto* form : local_results)
-    usernames.push_back(form->username_value);
-  std::sort(usernames.begin(), usernames.end());
-  bool has_duplicates =
-      std::adjacent_find(usernames.begin(), usernames.end()) != usernames.end();
+void ReportAccountChooserMetrics(bool had_duplicates, bool had_empty_username) {
   metrics_util::AccountChooserUsabilityMetric metric;
-  if (had_empty_username && has_duplicates)
+  if (had_empty_username && had_duplicates)
     metric = metrics_util::ACCOUNT_CHOOSER_EMPTY_USERNAME_AND_DUPLICATES;
   else if (had_empty_username)
     metric = metrics_util::ACCOUNT_CHOOSER_EMPTY_USERNAME;
-  else if (has_duplicates)
+  else if (had_duplicates)
     metric = metrics_util::ACCOUNT_CHOOSER_DUPLICATES;
   else
     metric = metrics_util::ACCOUNT_CHOOSER_LOOKS_OK;
   metrics_util::LogAccountChooserUsability(metric);
 }
 
+// Returns true iff |form1| is better suitable for showing in the account
+// chooser than |form2|. Inspired by PasswordFormManager::ScoreResult.
+bool IsBetterMatch(const autofill::PasswordForm& form1,
+                   const autofill::PasswordForm& form2) {
+  if (!form1.is_public_suffix_match && form2.is_public_suffix_match)
+    return true;
+  if (!form1.preferred && form2.preferred)

[vabr (Chromium), 2016/08/24 15:56:17]

Should this be the other way round? (I.e. preferred = better ?) If so, could you
write a test for that as well?

[vasilii, 2016/08/24 16:51:15]

Done.

+    return true;
+  return form1.date_created > form2.date_created;
+}
+
+// Remove duplicates in |forms| before displaying them in the account chooser.
+void FilterDuplicates(ScopedVector<autofill::PasswordForm>* forms) {
+  std::map<base::string16, std::unique_ptr<autofill::PasswordForm>> credentials;
+  for (auto& form : *forms) {
+    auto it = credentials.find(form->username_value);
+    if (it == credentials.end() || IsBetterMatch(*form, *it->second)) {
+      credentials[form->username_value] = base::WrapUnique(form);
+      form = nullptr;
+    }
+  }
+  forms->clear();
+  for (auto& form_pair : credentials)
+    forms->push_back(std::move(form_pair.second));
+}
+
 }  // namespace
 
 CredentialManagerPendingRequestTask::CredentialManagerPendingRequestTask(
@@ -126,6 +145,10 @@ void CredentialManagerPendingRequestTask::OnGetPasswordStoreResults(
   const bool has_empty_username = (begin_empty != local_results.end());
   local_results.erase(begin_empty, local_results.end());
 
+  const size_t local_results_size = local_results.size();
+  FilterDuplicates(&local_results);
+  const bool has_duplicates = (local_results_size != local_results.size());
+
   if ((local_results.empty() && federated_results.empty())) {
     delegate_->SendCredential(send_callback_, CredentialInfo());
     return;
@@ -157,9 +180,10 @@ void CredentialManagerPendingRequestTask::OnGetPasswordStoreResults(
   // or if the user chooses not to return a credential, and the credential the
   // user chooses if they pick one.
   std::unique_ptr<autofill::PasswordForm> potential_autosignin_form(
-      new autofill::PasswordForm(*local_results[0]));
+      can_use_autosignin ? new autofill::PasswordForm(*local_results[0])
+                         : nullptr);
   if (!zero_click_only_)
-    ReportAccountChooserMetrics(local_results, has_empty_username);
+    ReportAccountChooserMetrics(has_duplicates, has_empty_username);
   if (zero_click_only_ ||
       !delegate_->client()->PromptUserToChooseCredentials(
           std::move(local_results), std::move(federated_results), origin_,