Filter duplicates in the account chooser.

components/password_manager/content/browser/credential_manager_impl_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/content/browser/credential_manager_impl.h"
 
 #include <stdint.h>
 
 #include <string>
 #include <tuple>
@@ skipping 22 matching lines @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_renderer_host.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserContext;
 using content::WebContents;
 
 using testing::_;
+using testing::Pointee;
+using testing::UnorderedElementsAre;
 
 namespace password_manager {
 
 namespace {
 
 const char kTestWebOrigin[] = "https://example.com/";
 const char kTestAndroidRealm1[] = "android://hash@com.example.one.android/";
 const char kTestAndroidRealm2[] = "android://hash@com.example.two.android/";
 
 class MockPasswordManagerClient : public StubPasswordManagerClient {
@@ skipping 37 matching lines @@
   }
 
   PasswordStore* GetPasswordStore() const override { return store_; }
 
   PrefService* GetPrefs() override { return &prefs_; }
 
   bool PromptUserToChooseCredentials(
       ScopedVector<autofill::PasswordForm> local_forms,
       ScopedVector<autofill::PasswordForm> federated_forms,
       const GURL& origin,
-      const CredentialsCallback& callback) {
+      const CredentialsCallback& callback) override {
     EXPECT_FALSE(local_forms.empty() && federated_forms.empty());
     const autofill::PasswordForm* form =
         local_forms.empty() ? federated_forms[0] : local_forms[0];
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback, base::Owned(new autofill::PasswordForm(*form))));
     PromptUserToChooseCredentialsPtr(local_forms.get(), federated_forms.get(),
                                      origin, callback);
     return true;
   }
@@ skipping 577 matching lines @@
   EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _, _))
       .Times(testing::Exactly(0));
   EXPECT_CALL(*client_, NotifyUserAutoSigninPtr(_)).Times(testing::Exactly(0));
 
   std::vector<GURL> federations;
   ExpectCredentialType(false, true, federations,
                        CredentialType::CREDENTIAL_TYPE_EMPTY);
 }
 
 TEST_F(CredentialManagerImplTest,
-       CredentialManagerOnRequestCredentialWithEmptyAndNonUsernames) {
+       CredentialManagerOnRequestCredentialWithEmptyAndNonemptyUsernames) {
   store_->AddLogin(form_);
   autofill::PasswordForm empty = form_;
   empty.username_value.clear();
   store_->AddLogin(empty);
+  autofill::PasswordForm duplicate = form_;
+  duplicate.username_element = base::ASCIIToUTF16("username_element");

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

nit: If this is meant to be different from form_.username_element, it would be
good to point that out. You can change it to "different_username_element",
assign explicitly another string to form_, or write an ASSERT_NE.

(Also below.)

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

Done.

+  store_->AddLogin(duplicate);
 
   std::vector<GURL> federations;
   ExpectZeroClickSignInSuccess(false, true, federations,
                                CredentialType::CREDENTIAL_TYPE_PASSWORD);
 }
 
 TEST_F(CredentialManagerImplTest,
+       CredentialManagerOnRequestCredentialWithDuplicates) {
+  // Add 5 credentials. Two pairs of duplicates and one empty username. There
+  // should be just two in the account chooser.
+  store_->AddLogin(form_);
+  autofill::PasswordForm empty = form_;
+  empty.username_value.clear();
+  store_->AddLogin(empty);
+  autofill::PasswordForm duplicate = form_;
+  duplicate.username_element = base::ASCIIToUTF16("username_element");
+  duplicate.is_public_suffix_match = true;
+  store_->AddLogin(duplicate);
+
+  store_->AddLogin(origin_path_form_);
+  duplicate = origin_path_form_;
+  duplicate.username_element = base::ASCIIToUTF16("username_element");
+  duplicate.is_public_suffix_match = true;
+  store_->AddLogin(duplicate);
+
+  EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(
+                            UnorderedElementsAre(Pointee(form_),
+                                                 Pointee(origin_path_form_)),
+                            testing::IsEmpty(), _, _))
+      .Times(testing::Exactly(1));

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

optional: This line is not necessary (omitting is equivalent), but if you want
it to emphasise that this should be just once, I'm fine with leaving it as is.

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

It was copy-paste.

+
+  bool called = false;
+  mojom::CredentialManagerError error;
+  base::Optional<CredentialInfo> credential;
+  std::vector<GURL> federations;
+  CallGet(false, true, federations,
+          base::Bind(&GetCredentialCallback, &called, &error, &credential));
+
+  RunAllPendingTasks();
+}
+
+TEST_F(CredentialManagerImplTest,
        CredentialManagerOnRequestCredentialWithCrossOriginPasswordStore) {
   store_->AddLogin(cross_origin_form_);
 
   std::vector<GURL> federations;
   EXPECT_CALL(*client_, PromptUserToSavePasswordPtr(
                             _, CredentialSourceType::CREDENTIAL_SOURCE_API))
       .Times(testing::Exactly(0));
   EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _, _))
       .Times(testing::Exactly(0));
   EXPECT_CALL(*client_, NotifyUserAutoSigninPtr(_)).Times(testing::Exactly(0));
@@ skipping 595 matching lines @@
                             _, CredentialSourceType::CREDENTIAL_SOURCE_API));
   CallStore(info, base::Bind(&RespondCallback, &called));
   // Allow the PasswordFormManager to talk to the password store
   RunAllPendingTasks();
 
   ASSERT_TRUE(client_->pending_manager());
   EXPECT_TRUE(client_->pending_manager()->IsBlacklisted());
 }
 
 }  // namespace password_manager