Fix saving of password when choosing in a saving prompt.

When the user chooses a password in the save prompt from the combobox,
this password has to be saved.

The current implementation is the following:
1.When the user chooses another password, UpdatePasswordValue is called
2.UpdatePasswordValue sets password_value to this password.
3.CreatePendingCredentials is called (it detects what case it's - Save,
Update etc and creates credentials to write to store).

But if there is new_password_value in submitted_form, new_password_value
will be chosen to save instead of the user chosen. That's incorrect and
this CL fixes that. Namely: it removes new_password_value which makes
that user chosen value is saved.

Along the way, it was fixed calculating password_element, that fixes
sending votes.

Bug: 831123, 919487
Change-Id: I3f202224782eb1a1220df3e0ee2076f0ff2da23b
Reviewed-on: https://chromium-review.googlesource.com/c/1451982
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629101}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -328,8 +328,19 @@ void NewPasswordFormManager::UpdatePasswordValue(
   parsed_submitted_form_->password_value = new_password;
   parsed_submitted_form_->password_element.clear();
 
-  // TODO(https://crbug.com/831123): Implement processing password editing votes
-  // after implementation of |all_possible_passwords|.
+  // The user updated a password from the prompt. It means that heuristics were
+  // wrong. So clear new password, since it is likely wrong.
+  parsed_submitted_form_->new_password_value.clear();
+  parsed_submitted_form_->new_password_element.clear();
+
+  for (const ValueElementPair& pair :
+       parsed_submitted_form_->all_possible_passwords) {
+    if (pair.first == new_password) {
+      parsed_submitted_form_->password_element = pair.second;
+      break;
+    }
+  }
+
   CreatePendingCredentials();
 }
 

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -1047,7 +1047,7 @@ TEST_F(NewPasswordFormManagerTest, UpdateUsernameToAlreadyExisting) {
   EXPECT_TRUE(form_manager_->IsPasswordOverridden());
 }
 
-TEST_F(NewPasswordFormManagerTest, UpdatePasswordEmptyStore) {
+TEST_F(NewPasswordFormManagerTest, UpdatePasswordValueEmptyStore) {
   TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
   fetcher_->SetNonFederated({}, 0u);
 
@@ -1063,9 +1063,16 @@ TEST_F(NewPasswordFormManagerTest, UpdatePasswordEmptyStore) {
 
   CheckPendingCredentials(expected, form_manager_->GetPendingCredentials());
   EXPECT_TRUE(form_manager_->IsNewLogin());
+
+  // TODO(https://crbug.com/928690): implement not sending incorrect votes and
+  // check that StartUploadRequest is not called.
+  EXPECT_CALL(mock_autofill_download_manager_,
+              StartUploadRequest(_, _, _, _, _, _))
+      .Times(1);
+  form_manager_->Save();
 }
 
-TEST_F(NewPasswordFormManagerTest, UpdatePasswordToAlreadyExisting) {
+TEST_F(NewPasswordFormManagerTest, UpdatePasswordValueToAlreadyExisting) {
   TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
   fetcher_->SetNonFederated({&saved_match_}, 0u);
 
@@ -1083,6 +1090,50 @@ TEST_F(NewPasswordFormManagerTest, UpdatePasswordToAlreadyExisting) {
   EXPECT_FALSE(form_manager_->IsPasswordOverridden());
 }
 
+TEST_F(NewPasswordFormManagerTest, UpdatePasswordValueMultiplePasswordFields) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
+  FormData form = observed_form_only_password_fields_;
+
+  CreateFormManager(form);
+  fetcher_->SetNonFederated({}, 0u);
+  base::string16 password = ASCIIToUTF16("password1");
+  base::string16 pin = ASCIIToUTF16("pin");
+  form.fields[0].value = password;
+  form.fields[1].value = pin;
+  form_manager_->ProvisionallySave(form, &driver_);
+
+  // Check that a second password field is chosen for saving.
+  EXPECT_EQ(pin, form_manager_->GetPendingCredentials().password_value);
+
+  PasswordForm expected = form_manager_->GetPendingCredentials();
+  expected.password_value = password;
+  expected.password_element = form.fields[0].name;
+
+  // Simulate that the user updates value to save for the first password field.
+  form_manager_->UpdatePasswordValue(password);
+
+  // Check that newly created pending credentials are correct.
+  CheckPendingCredentials(expected, form_manager_->GetPendingCredentials());
+  EXPECT_TRUE(form_manager_->IsNewLogin());
+
+  // Check that a vote is sent for the field with the value which is chosen by
+  // the user.
+  std::map<base::string16, ServerFieldType> expected_types;
+  expected_types[expected.password_element] = autofill::PASSWORD;
+
+  EXPECT_CALL(mock_autofill_download_manager_,
+              StartUploadRequest(UploadedAutofillTypesAre(expected_types),
+                                 false, _, _, true, nullptr));
+
+  // Check that the password which was chosen by the user is saved.
+  MockFormSaver& form_saver = MockFormSaver::Get(form_manager_.get());
+  PasswordForm saved_form;
+  EXPECT_CALL(form_saver, Save(_, _)).WillOnce(SaveArg<0>(&saved_form));
+
+  form_manager_->Save();
+  CheckPendingCredentials(expected, saved_form);
+}
+
 TEST_F(NewPasswordFormManagerTest, PermanentlyBlacklist) {
   TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
   fetcher_->SetNonFederated({}, 0u);