[Password Manager] Send username correction votes

components/password_manager/core/browser/password_form_manager_unittest.cc

Index: components/password_manager/core/browser/password_form_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc
index 30f6cfe82ded7c955a88b4775daf7f377d8c03d9..0984348d370a30ee499f638eeabef59add28c716 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -47,6 +47,7 @@
 using autofill::FieldPropertiesFlags;
 using autofill::FieldPropertiesMask;
 using autofill::PasswordForm;
+using autofill::PossibleUsernamePair;
 using base::ASCIIToUTF16;
 using ::testing::_;
 using ::testing::IsEmpty;
@@ -330,8 +331,8 @@ class PasswordFormManagerTest : public testing::Test {
     saved_match_.preferred = true;
     saved_match_.username_value = ASCIIToUTF16("test@gmail.com");
     saved_match_.password_value = ASCIIToUTF16("test1");
-    saved_match_.other_possible_usernames.push_back(
-        ASCIIToUTF16("test2@gmail.com"));
+    saved_match_.other_possible_usernames.push_back(PossibleUsernamePair(
+        ASCIIToUTF16("test2@gmail.com"), ASCIIToUTF16("full_name")));
 
     psl_saved_match_ = saved_match_;
     psl_saved_match_.is_public_suffix_match = true;
@@ -401,15 +402,7 @@ class PasswordFormManagerTest : public testing::Test {
     autofill::ServerFieldTypeSet expected_available_field_types;
     FieldTypeMap expected_types;
     expected_types[ASCIIToUTF16("full_name")] = autofill::UNKNOWN_TYPE;
-
-    // When we're voting for an account creation form, we should also vote
-    // for its username field.
-    if (field_type && *field_type == autofill::ACCOUNT_CREATION_PASSWORD) {
-      expected_types[match.username_element] = autofill::USERNAME;
-      expected_available_field_types.insert(autofill::USERNAME);
-    } else {
-      expected_types[match.username_element] = autofill::UNKNOWN_TYPE;
-    }
+    expected_types[match.username_element] = autofill::UNKNOWN_TYPE;
 
     if (field_type) {
       expected_available_field_types.insert(*field_type);
@@ -934,7 +927,6 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) {
   PasswordForm actual_saved_form;
 
   autofill::ServerFieldTypeSet expected_available_field_types;
-  expected_available_field_types.insert(autofill::USERNAME);
   expected_available_field_types.insert(autofill::ACCOUNT_CREATION_PASSWORD);
   EXPECT_CALL(
       *client()->mock_driver()->mock_autofill_download_manager(),
@@ -1166,7 +1158,8 @@ TEST_F(PasswordFormManagerTest, TestAlternateUsername_NoChange) {
 
   PasswordForm saved_form = *saved_match();
   saved_form.other_possible_usernames.push_back(
-      ASCIIToUTF16("other_possible@gmail.com"));
+      PossibleUsernamePair(ASCIIToUTF16("other_possible@gmail.com"),
+                           ASCIIToUTF16("other_username")));
 
   fake_form_fetcher()->SetNonFederated({&saved_form}, 0u);
 
@@ -1201,8 +1194,8 @@ TEST_F(PasswordFormManagerTest, TestAlternateUsername_NoChange) {
 TEST_F(PasswordFormManagerTest, TestAlternateUsername_OtherUsername) {
   EXPECT_CALL(*client()->mock_driver(), AllowPasswordGenerationForForm(_));
 
-  const base::string16 kOtherUsername =
-      ASCIIToUTF16("other_possible@gmail.com");
+  const PossibleUsernamePair kOtherUsername(
+      ASCIIToUTF16("other_possible@gmail.com"), ASCIIToUTF16("other_username"));
   PasswordForm saved_form = *saved_match();
   saved_form.other_possible_usernames.push_back(kOtherUsername);
 
@@ -1211,7 +1204,7 @@ TEST_F(PasswordFormManagerTest, TestAlternateUsername_OtherUsername) {
   // The user chooses an alternative username.
   PasswordForm login(*observed_form());
   login.preferred = true;
-  login.username_value = kOtherUsername;
+  login.username_value = kOtherUsername.first;
   login.password_value = saved_match()->password_value;
 
   form_manager()->ProvisionallySave(
@@ -1229,7 +1222,7 @@ TEST_F(PasswordFormManagerTest, TestAlternateUsername_OtherUsername) {
 
   // |other_possible_usernames| should also be empty, but username_value should
   // be changed to match |new_username|.
-  EXPECT_EQ(kOtherUsername, saved_result.username_value);
+  EXPECT_EQ(kOtherUsername.first, saved_result.username_value);
   EXPECT_TRUE(saved_result.other_possible_usernames.empty());
 }
 
@@ -1331,14 +1324,16 @@ TEST_F(PasswordFormManagerTest, TestBestCredentialsForEachUsernameAreIncluded) {
 }
 
 TEST_F(PasswordFormManagerTest, TestSanitizePossibleUsernames) {
-  const base::string16 kUsernameOther = ASCIIToUTF16("other username");
+  const PossibleUsernamePair kUsernameOther(ASCIIToUTF16("other username"),
+                                            ASCIIToUTF16("other_username_id"));
 
   fake_form_fetcher()->SetNonFederated(std::vector<const PasswordForm*>(), 0u);
 
   PasswordForm credentials(*observed_form());
-  credentials.other_possible_usernames.push_back(ASCIIToUTF16("543-43-1234"));
   credentials.other_possible_usernames.push_back(
-      ASCIIToUTF16("378282246310005"));
+      PossibleUsernamePair(ASCIIToUTF16("543-43-1234"), ASCIIToUTF16("id1")));
+  credentials.other_possible_usernames.push_back(PossibleUsernamePair(
+      ASCIIToUTF16("378282246310005"), ASCIIToUTF16("id2")));
   credentials.other_possible_usernames.push_back(kUsernameOther);
   credentials.username_value = ASCIIToUTF16("test@gmail.com");
   credentials.preferred = true;
@@ -1359,19 +1354,24 @@ TEST_F(PasswordFormManagerTest, TestSanitizePossibleUsernames) {
 }
 
 TEST_F(PasswordFormManagerTest, TestSanitizePossibleUsernamesDuplicates) {
-  const base::string16 kUsernameEmail = ASCIIToUTF16("test@gmail.com");
-  const base::string16 kUsernameDuplicate = ASCIIToUTF16("duplicate");
-  const base::string16 kUsernameRandom = ASCIIToUTF16("random");
+  const PossibleUsernamePair kUsernameSsn(ASCIIToUTF16("511-32-9830"),
+                                          ASCIIToUTF16("ssn_id"));
+  const PossibleUsernamePair kUsernameEmail(ASCIIToUTF16("test@gmail.com"),
+                                            ASCIIToUTF16("email_id"));
+  const PossibleUsernamePair kUsernameDuplicate(ASCIIToUTF16("duplicate"),
+                                                ASCIIToUTF16("duplicate_id"));
+  const PossibleUsernamePair kUsernameRandom(ASCIIToUTF16("random"),
+                                             ASCIIToUTF16("random_id"));
 
   fake_form_fetcher()->SetNonFederated(std::vector<const PasswordForm*>(), 0u);
 
   PasswordForm credentials(*observed_form());
-  credentials.other_possible_usernames.push_back(ASCIIToUTF16("511-32-9830"));
+  credentials.other_possible_usernames.push_back(kUsernameSsn);
   credentials.other_possible_usernames.push_back(kUsernameDuplicate);
   credentials.other_possible_usernames.push_back(kUsernameDuplicate);
   credentials.other_possible_usernames.push_back(kUsernameRandom);
   credentials.other_possible_usernames.push_back(kUsernameEmail);
-  credentials.username_value = kUsernameEmail;
+  credentials.username_value = kUsernameEmail.first;
   credentials.preferred = true;
 
   // Pass in ALLOW_OTHER_POSSIBLE_USERNAMES, although it will not make a
@@ -2898,4 +2898,49 @@ TEST_F(PasswordFormManagerTest, DoesManageDifferentSignonRealmSameDrivers) {
       form_manager()->DoesManage(submitted_form, client()->driver().get()));
 }
 
+TEST_F(PasswordFormManagerTest, UploadUsernameCorrectionVote) {
+  // Observed and saved forms have the same password, but different usernames.
+  PasswordForm new_login(*observed_form());
+  new_login.username_value = saved_match()->other_possible_usernames[0].first;
+  new_login.password_value = saved_match()->password_value;
+
+  fake_form_fetcher()->SetNonFederated({saved_match()}, 0u);
+  form_manager()->ProvisionallySave(
+      new_login, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  // No match found (because usernames are different).
+  EXPECT_TRUE(form_manager()->IsNewLogin());
+
+  // Checks the username correction vote is saved.

[vabr (Chromium), 2017/03/16 21:00:42]

Does this duplicate the check on line 2940?
If yes, and if the only reason for the username_correction_vote() accessor in
PasswordFormManager is peeking at it in tests, then I strongly suggest removing
the accessor. PasswordFormManager has unfortunately grown to a state when most
of its internal data is exposed through accessors, making the tests dependent on
its internal representation. That's bad and means brittle tests and a lot of
changes if we ever change how data is stored in PasswordFormManager.

[kolos1, 2017/03/17 14:09:34]

Yes, we could get rid of this check. Fixed.

+  PasswordForm expected_username_vote(*saved_match());
+  expected_username_vote.username_element =
+      saved_match()->other_possible_usernames[0].second;
+  ASSERT_TRUE(form_manager()->username_correction_vote());
+  EXPECT_EQ(expected_username_vote,
+            *form_manager()->username_correction_vote());
+
+  // Checks the upload.
+  autofill::ServerFieldTypeSet expected_available_field_types;
+  expected_available_field_types.insert(autofill::USERNAME);
+
+  FormStructure expected_upload(expected_username_vote.form_data);
+
+  std::string expected_login_signature =
+      FormStructure(form_manager()->observed_form().form_data)
+          .FormSignatureAsStr();
+
+  std::map<base::string16, autofill::ServerFieldType> expected_types;
+  expected_types[expected_username_vote.username_element] = autofill::USERNAME;
+  expected_types[expected_username_vote.password_element] =
+      autofill::UNKNOWN_TYPE;
+  expected_types[ASCIIToUTF16("Email")] = autofill::UNKNOWN_TYPE;
+
+  EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
+              StartUploadRequest(
+                  CheckUploadedAutofillTypesAndSignature(
+                      expected_upload.FormSignatureAsStr(), expected_types),
+                  false, expected_available_field_types,
+                  expected_login_signature, true));
+  form_manager()->Save();
+}
+
 }  // namespace password_manager