Change interface of FormSaver::Update.

This is a follow up to r649499. The Update method now matches Save() method.
The replacement logic is now in UpdateReplace().

Bug: 936011,831123
Change-Id: I59d65fe77ef3b386e80ba890e140370d190555c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596456
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657675}

components/password_manager/core/browser/form_saver.h

@@ -13,6 +13,7 @@
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/password_store.h"
 
 namespace password_manager {
 
@@ -24,8 +25,10 @@ class FormSaver {
 
   virtual ~FormSaver() = default;
 
-  // Configures the |observed| form to become a blacklist entry and saves it.
-  virtual void PermanentlyBlacklist(autofill::PasswordForm* observed) = 0;
+  // Blacklist the origin described by |digest|. Returns the PasswordForm pushed
+  // to the store.
+  virtual autofill::PasswordForm PermanentlyBlacklist(
+      PasswordStore::FormDigest digest) WARN_UNUSED_RESULT = 0;
 
   // Saves the |pending| form.
   // |matches| are relevant credentials for the site. After saving |pending|,
@@ -35,21 +38,29 @@ class FormSaver {
   // - empty-username credentials with the same password are removed.
   // - if |old_password| is provided, the old credentials with the same username
   //   and the old password are updated to the new password.
-  virtual void Save(const autofill::PasswordForm& pending,
+  virtual void Save(autofill::PasswordForm pending,
                     const std::vector<const autofill::PasswordForm*>& matches,
                     const base::string16& old_password) = 0;
 
-  // Updates the |pending| form and updates the stored preference on
-  // |best_matches|. If |old_primary_key| is given, uses it for saving
-  // |pending|. It also updates the password store with all
-  // |credentials_to_update|.
-  // TODO(crbug/831123): it's a convoluted interface, switch to the one for Save
-  virtual void Update(
-      const autofill::PasswordForm& pending,
-      const std::map<base::string16, const autofill::PasswordForm*>&
-          best_matches,
-      const std::vector<autofill::PasswordForm>* credentials_to_update,
-      const autofill::PasswordForm* old_primary_key) = 0;
+  // Updates the saved credential in the password store sharing the same key as
+  // the |pending| form.
+  // The algorithm for handling |matches| and |old_password| is the same as
+  // above.
+  virtual void Update(autofill::PasswordForm pending,
+                      const std::vector<const autofill::PasswordForm*>& matches,
+                      const base::string16& old_password) = 0;
+
+  // If any of the primary key fields (signon_realm, origin, username_element,
+  // username_value, password_element) are updated, then the this version of
+  // the Update method must be used, which takes |old_primary_key|, i.e., the
+  // old values for the primary key fields (the rest of the fields are ignored).
+  // The algorithm for handling |matches| and |old_password| is the same as
+  // above.
+  virtual void UpdateReplace(
+      autofill::PasswordForm pending,
+      const std::vector<const autofill::PasswordForm*>& matches,
+      const base::string16& old_password,
+      const autofill::PasswordForm& old_unique_key) = 0;
 
   // Removes |form| from the password store.
   virtual void Remove(const autofill::PasswordForm& form) = 0;

components/password_manager/core/browser/form_saver_impl.cc

@@ -42,40 +42,25 @@ void SanitizeFormData(FormData* form) {
   }
 }
 
-}  // namespace
-
-FormSaverImpl::FormSaverImpl(PasswordStore* store) : store_(store) {
-  DCHECK(store);
-}
-
-FormSaverImpl::~FormSaverImpl() = default;
-
-void FormSaverImpl::PermanentlyBlacklist(PasswordForm* observed) {
-  *observed = password_manager_util::MakeNormalizedBlacklistedForm(
-      PasswordStore::FormDigest(*observed));
-  observed->date_created = base::Time::Now();
-  store_->AddLogin(*observed);
-}
-
-void FormSaverImpl::Save(const PasswordForm& pending,
-                         const std::vector<const PasswordForm*>& matches,
-                         const base::string16& old_password) {
+// Do the clean up of |matches| after |pending| was just pushed to the store.
+void PostProcessMatches(const PasswordForm& pending,
+                        const std::vector<const PasswordForm*>& matches,
+                        const base::string16& old_password,
+                        PasswordStore* store) {
   DCHECK(!pending.blacklisted_by_user);
 
-  PasswordForm sanitized_pending(pending);
-  SanitizeFormData(&sanitized_pending.form_data);
-  store_->AddLogin(sanitized_pending);
   // Update existing matches in the password store.
   for (const auto* match : matches) {
     DCHECK(pending.preferred);
-    if (match->IsFederatedCredential())
+    if (match->IsFederatedCredential() ||
+        ArePasswordFormUniqueKeyEqual(pending, *match))
       continue;
     // Delete obsolete empty username credentials.
     const bool same_password = match->password_value == pending.password_value;
     const bool username_was_added =
         match->username_value.empty() && !pending.username_value.empty();
     if (same_password && username_was_added && !match->is_public_suffix_match) {
-      store_->RemoveLogin(*match);
+      store->RemoveLogin(*match);
       continue;
     }
     base::Optional<PasswordForm> form_to_update;
@@ -95,72 +80,64 @@ void FormSaverImpl::Save(const PasswordForm& pending,
     }
     if (form_to_update) {
       SanitizeFormData(&form_to_update->form_data);
-      store_->UpdateLogin(std::move(*form_to_update));
+      store->UpdateLogin(std::move(*form_to_update));
     }
   }
 }
 
-void FormSaverImpl::Update(
-    const PasswordForm& pending,
-    const std::map<base::string16, const PasswordForm*>& best_matches,
-    const std::vector<PasswordForm>* credentials_to_update,
-    const PasswordForm* old_primary_key) {
-  DCHECK(!pending.blacklisted_by_user);
+}  // namespace
 
-  if (!best_matches.empty()) {
-    DCHECK(pending.preferred);
-    UpdatePreferredLoginState(pending.username_value, best_matches);
-  }
-  PasswordForm sanitized_pending(pending);
-  SanitizeFormData(&sanitized_pending.form_data);
-  if (old_primary_key)
-    store_->UpdateLoginWithPrimaryKey(sanitized_pending, *old_primary_key);
-  else
-    store_->UpdateLogin(sanitized_pending);
-
-  if (credentials_to_update) {
-    for (const PasswordForm& credential : *credentials_to_update) {
-      store_->UpdateLogin(credential);
-    }
-  }
+FormSaverImpl::FormSaverImpl(PasswordStore* store) : store_(store) {
+  DCHECK(store);
 }
 
-void FormSaverImpl::Remove(const PasswordForm& form) {
-  store_->RemoveLogin(form);
+FormSaverImpl::~FormSaverImpl() = default;
+
+PasswordForm FormSaverImpl::PermanentlyBlacklist(
+    PasswordStore::FormDigest digest) {
+  PasswordForm blacklisted =
+      password_manager_util::MakeNormalizedBlacklistedForm(std::move(digest));
+  blacklisted.date_created = base::Time::Now();
+  store_->AddLogin(blacklisted);
+  return blacklisted;
 }
 
-std::unique_ptr<FormSaver> FormSaverImpl::Clone() {
-  return std::make_unique<FormSaverImpl>(store_);
+void FormSaverImpl::Save(PasswordForm pending,
+                         const std::vector<const PasswordForm*>& matches,
+                         const base::string16& old_password) {
+  SanitizeFormData(&pending.form_data);
+  store_->AddLogin(pending);
+  // Update existing matches in the password store.
+  PostProcessMatches(pending, matches, old_password, store_);
 }
 
-void FormSaverImpl::UpdatePreferredLoginState(
-    const base::string16& preferred_username,
-    const std::map<base::string16, const PasswordForm*>& best_matches) {
-  for (const auto& key_value_pair : best_matches) {
-    const PasswordForm& form = *key_value_pair.second;
-    if (form.preferred && !form.is_public_suffix_match &&
-        form.username_value != preferred_username) {
-      // This wasn't the selected login but it used to be preferred.
-      PasswordForm update(form);
-      SanitizeFormData(&update.form_data);
-      update.preferred = false;
-      store_->UpdateLogin(update);
-    }
-  }
+void FormSaverImpl::Update(
+    autofill::PasswordForm pending,
+    const std::vector<const autofill::PasswordForm*>& matches,
+    const base::string16& old_password) {
+  SanitizeFormData(&pending.form_data);
+  store_->UpdateLogin(pending);
+  // Update existing matches in the password store.
+  PostProcessMatches(pending, matches, old_password, store_);
 }
 
-void FormSaverImpl::DeleteEmptyUsernameCredentials(
-    const PasswordForm& pending,
-    const std::map<base::string16, const PasswordForm*>& best_matches) {
-  DCHECK(!pending.username_value.empty());
+void FormSaverImpl::UpdateReplace(
+    autofill::PasswordForm pending,
+    const std::vector<const autofill::PasswordForm*>& matches,
+    const base::string16& old_password,
+    const autofill::PasswordForm& old_unique_key) {
+  SanitizeFormData(&pending.form_data);
+  store_->UpdateLoginWithPrimaryKey(pending, old_unique_key);
+  // Update existing matches in the password store.
+  PostProcessMatches(pending, matches, old_password, store_);
+}
 
-  for (const auto& match : best_matches) {
-    const PasswordForm* form = match.second;
-    if (!form->is_public_suffix_match && form->username_value.empty() &&
-        form->password_value == pending.password_value) {
-      store_->RemoveLogin(*form);
-    }
-  }
+void FormSaverImpl::Remove(const PasswordForm& form) {
+  store_->RemoveLogin(form);
+}
+
+std::unique_ptr<FormSaver> FormSaverImpl::Clone() {
+  return std::make_unique<FormSaverImpl>(store_);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/form_saver_impl.h

@@ -24,34 +24,22 @@ class FormSaverImpl : public FormSaver {
   ~FormSaverImpl() override;
 
   // FormSaver:
-  void PermanentlyBlacklist(autofill::PasswordForm* observed) override;
-  void Save(const autofill::PasswordForm& pending,
+  autofill::PasswordForm PermanentlyBlacklist(
+      PasswordStore::FormDigest digest) override WARN_UNUSED_RESULT;
+  void Save(autofill::PasswordForm pending,
             const std::vector<const autofill::PasswordForm*>& matches,
             const base::string16& old_password) override;
-  void Update(const autofill::PasswordForm& pending,
-              const std::map<base::string16, const autofill::PasswordForm*>&
-                  best_matches,
-              const std::vector<autofill::PasswordForm>* credentials_to_update,
-              const autofill::PasswordForm* old_primary_key) override;
+  void Update(autofill::PasswordForm pending,
+              const std::vector<const autofill::PasswordForm*>& matches,
+              const base::string16& old_password) override;
+  void UpdateReplace(autofill::PasswordForm pending,
+                     const std::vector<const autofill::PasswordForm*>& matches,
+                     const base::string16& old_password,
+                     const autofill::PasswordForm& old_unique_key) override;
   void Remove(const autofill::PasswordForm& form) override;
   std::unique_ptr<FormSaver> Clone() override;
 
  private:
-  // Marks all of |best_matches| as not preferred unless the username is
-  // |preferred_username| or the credential is PSL matched.
-  void UpdatePreferredLoginState(
-      const base::string16& preferred_username,
-      const std::map<base::string16, const autofill::PasswordForm*>&
-          best_matches);
-
-  // Iterates over all |best_matches| and deletes from the password store all
-  // which are not PSL-matched, have an empty username, and a password equal to
-  // |pending.password_value|.
-  void DeleteEmptyUsernameCredentials(
-      const autofill::PasswordForm& pending,
-      const std::map<base::string16, const autofill::PasswordForm*>&
-          best_matches);
-
   // The class is stateless. Don't introduce it. The methods are utilities for
   // common tasks on the password store. The state should belong to either a
   // form handler or origin handler which could embed FormSaver.

components/password_manager/core/browser/form_saver_impl_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/test/scoped_task_environment.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/password_manager_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -52,6 +53,18 @@ PasswordForm CreatePending(StringPiece username, StringPiece password) {
   return form;
 }
 
+MATCHER_P(FormWithSomeDate, expected, "") {
+  PasswordForm expected_with_date = expected;
+  expected_with_date.date_created = arg.date_created;
+  return arg == expected_with_date && arg.date_created != base::Time();
+}
+
+enum class SaveOperation {
+  kSave,
+  kUpdate,
+  kReplaceUpdate,
+};
+
 }  // namespace
 
 class FormSaverImplTest : public testing::Test {
@@ -72,126 +85,72 @@ class FormSaverImplTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(FormSaverImplTest);
 };
 
-// Check that blacklisting an observed form sets the right properties and calls
-// the PasswordStore.
-TEST_F(FormSaverImplTest, PermanentlyBlacklist) {
-  PasswordForm observed = CreateObserved();
-  PasswordForm saved;
-
-  observed.blacklisted_by_user = false;
-  observed.preferred = true;
-  observed.username_value = ASCIIToUTF16("user1");
-  observed.username_element = ASCIIToUTF16("user");
-  observed.password_value = ASCIIToUTF16("12345");
-  observed.password_element = ASCIIToUTF16("password");
-  observed.other_possible_usernames = {
-      {ASCIIToUTF16("user2"), ASCIIToUTF16("field")}};
-  observed.origin = GURL("https://www.example.com/foobar");
+class FormSaverImplSaveTest
+    : public FormSaverImplTest,
+      public ::testing::WithParamInterface<SaveOperation> {
+ protected:
+  // Either saves, updates or replaces |pending| according to the test param.
+  void SaveCredential(autofill::PasswordForm pending,
+                      const std::vector<const autofill::PasswordForm*>& matches,
+                      const base::string16& old_password);
+};
 
-  EXPECT_CALL(*mock_store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved));
-  form_saver_.PermanentlyBlacklist(&observed);
-  EXPECT_TRUE(saved.blacklisted_by_user);
-  EXPECT_FALSE(saved.preferred);
-  EXPECT_EQ(base::string16(), saved.username_value);
-  EXPECT_EQ(base::string16(), saved.username_element);
-  EXPECT_EQ(base::string16(), saved.password_value);
-  EXPECT_EQ(base::string16(), saved.password_element);
-  EXPECT_TRUE(saved.other_possible_usernames.empty());
-  EXPECT_EQ(GURL("https://www.example.com/"), saved.origin);
+void FormSaverImplSaveTest::SaveCredential(
+    autofill::PasswordForm pending,
+    const std::vector<const autofill::PasswordForm*>& matches,
+    const base::string16& old_password) {
+  switch (GetParam()) {
+    case SaveOperation::kSave:
+      EXPECT_CALL(*mock_store_, AddLogin(pending));
+      return form_saver_.Save(std::move(pending), matches, old_password);
+    case SaveOperation::kUpdate:
+      EXPECT_CALL(*mock_store_, UpdateLogin(pending));
+      return form_saver_.Update(std::move(pending), matches, old_password);
+    case SaveOperation::kReplaceUpdate: {
+      PasswordForm old_key = CreatePending("some_other_username", "1234");
+      EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(pending, old_key));
+      return form_saver_.UpdateReplace(std::move(pending), matches,
+                                       old_password, old_key);
+    }
+  }
 }
 
-// Check that saving the pending form as new adds the credential to the store
-// (rather than updating).
-TEST_F(FormSaverImplTest, Save_AsNew) {
+// Pushes the credential to the store without any matches.
+TEST_P(FormSaverImplSaveTest, Write_EmptyStore) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
-  PasswordForm saved;
 
-  EXPECT_CALL(*mock_store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  form_saver_.Save(pending, {} /* matches */, base::string16());
-  EXPECT_EQ(ASCIIToUTF16("nameofuser"), saved.username_value);
-  EXPECT_EQ(ASCIIToUTF16("wordToP4a55"), saved.password_value);
+  SaveCredential(pending, {} /* matches */,
+                 base::string16() /* old_password */);
 }
 
-// Check that saving the pending form as not new updates the store with the
+// Pushes the credential to the store with |matches| containing the pending
 // credential.
-TEST_F(FormSaverImplTest, Save_Update) {
+TEST_P(FormSaverImplSaveTest, Write_EmptyStoreWithPending) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
-  PasswordForm saved;
-
-  EXPECT_CALL(*mock_store_, AddLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).WillOnce(SaveArg<0>(&saved));
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  form_saver_.Update(pending, std::map<base::string16, const PasswordForm*>(),
-                     nullptr, nullptr);
-  EXPECT_EQ(ASCIIToUTF16("nameofuser"), saved.username_value);
-  EXPECT_EQ(ASCIIToUTF16("wordToP4a55"), saved.password_value);
-}
 
-// Check that passing other credentials to update to the Save call results in
-// the store being updated with those credentials in addition to the pending
-// one.
-TEST_F(FormSaverImplTest, Save_UpdateAlsoOtherCredentials) {
-  PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
-  PasswordForm related1 = pending;
-  related1.origin = GURL("https://other.example.ca");
-  related1.signon_realm = related1.origin.spec();
-  PasswordForm related2 = pending;
-  related2.origin = GURL("http://complete.example.net");
-  related2.signon_realm = related2.origin.spec();
-  std::vector<PasswordForm> credentials_to_update = {related1, related2};
-  pending.password_value = ASCIIToUTF16("abcd");
-
-  PasswordForm saved[3];
-
-  EXPECT_CALL(*mock_store_, AddLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLogin(_))
-      .WillOnce(SaveArg<0>(&saved[0]))
-      .WillOnce(SaveArg<0>(&saved[1]))
-      .WillOnce(SaveArg<0>(&saved[2]));
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  form_saver_.Update(pending, std::map<base::string16, const PasswordForm*>(),
-                     &credentials_to_update, nullptr);
-  std::set<GURL> different_origins;
-  for (const PasswordForm& form : saved) {
-    different_origins.insert(form.origin);
-  }
-  EXPECT_THAT(different_origins,
-              testing::UnorderedElementsAre(pending.origin, related1.origin,
-                                            related2.origin));
+  SaveCredential(pending, {&pending} /* matches */, pending.password_value);
 }
 
-// Check that if the old primary key is supplied, the appropriate store method
-// for update is used.
-TEST_F(FormSaverImplTest, Save_UpdateWithPrimaryKey) {
-  PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
-  PasswordForm old_key = pending;
-  old_key.username_value = ASCIIToUTF16("old username");
-  PasswordForm saved_new;
-  PasswordForm saved_old;
+// Pushes the credential to the store with |matches| containing the pending
+// credential with an old password.
+TEST_P(FormSaverImplSaveTest, Write_EmptyStoreWithPendingOldPassword) {
+  PasswordForm pending = CreatePending("nameofuser", "old_password");
 
-  EXPECT_CALL(*mock_store_, AddLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _))
-      .WillOnce(DoAll(SaveArg<0>(&saved_new), SaveArg<1>(&saved_old)));
-  form_saver_.Update(pending, std::map<base::string16, const PasswordForm*>(),
-                     nullptr, &old_key);
-  EXPECT_EQ(ASCIIToUTF16("nameofuser"), saved_new.username_value);
-  EXPECT_EQ(ASCIIToUTF16("wordToP4a55"), saved_new.password_value);
-  EXPECT_EQ(ASCIIToUTF16("old username"), saved_old.username_value);
+  SaveCredential(CreatePending("nameofuser", "new_password"),
+                 {&pending} /* matches */, pending.password_value);
 }
 
-// Check that the "preferred" bit of best matches is updated accordingly in the
+// Check that the "preferred" bit of the matches is updated accordingly in the
 // store.
-TEST_F(FormSaverImplTest, Save_AndUpdatePreferredLoginState) {
+TEST_P(FormSaverImplSaveTest, Write_AndUpdatePreferredLoginState) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
   pending.preferred = true;
 
-  // |best_matches| will contain 3 forms
+  // |best_matches| will contain 4 forms
   // - non-PSL matched with a different username.
-  // - PSL-matched with the same username
   // - another non-PSL that is not preferred.
+  // - PSL-matched with the same username
+  // - PSL-matched with another username
   // FormSaver should ignore the pending and PSL-matched one, but should update
   // the non-PSL matched form (with different username) to no longer be
   // preferred.
@@ -201,30 +160,20 @@ TEST_F(FormSaverImplTest, Save_AndUpdatePreferredLoginState) {
   other_non_preferred.preferred = false;
   PasswordForm psl_match = pending;
   psl_match.is_public_suffix_match = true;
-  const std::vector<const PasswordForm*> matches = {&other, &psl_match,
-                                                    &other_non_preferred};
-
-  PasswordForm saved;
-  PasswordForm updated;
+  PasswordForm other_psl_match = psl_match;
+  other_psl_match.username_value = ASCIIToUTF16("othername");
+  const std::vector<const PasswordForm*> matches = {
+      &other, &psl_match, &other_non_preferred, &other_psl_match};
 
-  EXPECT_CALL(*mock_store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).WillOnce(SaveArg<0>(&updated));
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  form_saver_.Save(pending, matches, base::string16());
-  EXPECT_EQ(ASCIIToUTF16("nameofuser"), saved.username_value);
-  EXPECT_EQ(ASCIIToUTF16("wordToP4a55"), saved.password_value);
-  EXPECT_TRUE(saved.preferred);
-  EXPECT_FALSE(saved.is_public_suffix_match);
-  EXPECT_EQ(ASCIIToUTF16("othername"), updated.username_value);
-  EXPECT_EQ(ASCIIToUTF16("wordToP4a55"), updated.password_value);
-  EXPECT_FALSE(updated.preferred);
-  EXPECT_FALSE(updated.is_public_suffix_match);
+  PasswordForm updated = other;
+  updated.preferred = false;
+  EXPECT_CALL(*mock_store_, UpdateLogin(updated));
+  SaveCredential(pending, matches, base::string16());
 }
 
 // Check that storing credentials with a non-empty username results in deleting
-// credentials with the same password but empty username, if present in best
-// matches.
-TEST_F(FormSaverImplTest, Save_AndDeleteEmptyUsernameCredentials) {
+// credentials with the same password but empty username, if present in matches.
+TEST_P(FormSaverImplSaveTest, Write_AndDeleteEmptyUsernameCredentials) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
 
   PasswordForm non_empty_username = pending;
@@ -237,18 +186,15 @@ TEST_F(FormSaverImplTest, Save_AndDeleteEmptyUsernameCredentials) {
   const std::vector<const PasswordForm*> matches = {&non_empty_username,
                                                     &no_username};
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
   EXPECT_CALL(*mock_store_, RemoveLogin(no_username));
-  form_saver_.Save(pending, matches, base::string16());
+  SaveCredential(pending, matches, base::string16());
 }
 
 // Check that storing credentials with a non-empty username does not result in
 // deleting credentials with a different password, even if they have no
 // username.
-TEST_F(FormSaverImplTest,
-       Save_AndDoNotDeleteEmptyUsernameCredentialsWithDifferentPassword) {
+TEST_P(FormSaverImplSaveTest,
+       Write_AndDoNotDeleteEmptyUsernameCredentialsWithDifferentPassword) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
 
   PasswordForm no_username = pending;
@@ -256,34 +202,28 @@ TEST_F(FormSaverImplTest,
   no_username.preferred = false;
   no_username.password_value = ASCIIToUTF16("abcd");
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
   EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, {&no_username}, base::string16());
+  SaveCredential(pending, {&no_username}, base::string16());
 }
 
 // Check that if a credential without username is saved, and another credential
 // with the same password (and a non-empty username) is present in best matches,
 // nothing is deleted.
-TEST_F(FormSaverImplTest, Save_EmptyUsernameWillNotCauseDeletion) {
+TEST_P(FormSaverImplSaveTest, Write_EmptyUsernameWillNotCauseDeletion) {
   PasswordForm pending = CreatePending("", "wordToP4a55");
 
   PasswordForm with_username = pending;
   with_username.username_value = ASCIIToUTF16("nameofuser");
   with_username.preferred = false;
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
   EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, {&with_username}, base::string16());
+  SaveCredential(pending, {&with_username}, base::string16());
 }
 
-// Check that PSL-matched credentials in best matches are exempt from deletion,
+// Check that PSL-matched credentials in matches are exempt from deletion,
 // even if they have an empty username and the same password as the pending
 // credential.
-TEST_F(FormSaverImplTest, Save_AndDoNotDeleteEmptyUsernamePSLCredentials) {
+TEST_P(FormSaverImplSaveTest, Write_AndDoNotDeleteEmptyUsernamePSLCredentials) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
 
   PasswordForm stored = pending;
@@ -292,31 +232,25 @@ TEST_F(FormSaverImplTest, Save_AndDoNotDeleteEmptyUsernamePSLCredentials) {
   no_username_psl.is_public_suffix_match = true;
   const std::vector<const PasswordForm*> matches = {&stored, &no_username_psl};
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
   EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, matches, base::string16());
+  SaveCredential(pending, matches, base::string16());
 }
 
 // Check that on storing a credential, other credentials with the same password
 // are not removed, as long as they have a non-empty username.
-TEST_F(FormSaverImplTest, Save_AndDoNotDeleteNonEmptyUsernameCredentials) {
+TEST_P(FormSaverImplSaveTest, Write_AndDoNotDeleteNonEmptyUsernameCredentials) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
 
   PasswordForm other_username = pending;
   other_username.username_value = ASCIIToUTF16("other username");
   other_username.preferred = false;
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
-  EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
   EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, {&other_username}, base::string16());
+  SaveCredential(pending, {&other_username}, base::string16());
 }
 
 // Stores a credential and makes sure that its duplicate is updated.
-TEST_F(FormSaverImplTest, Save_AndUpdatePasswordValuesOnExactMatch) {
+TEST_P(FormSaverImplSaveTest, Write_AndUpdatePasswordValuesOnExactMatch) {
   constexpr char kOldPassword[] = "old_password";
   constexpr char kNewPassword[] = "new_password";
 
@@ -325,39 +259,31 @@ TEST_F(FormSaverImplTest, Save_AndUpdatePasswordValuesOnExactMatch) {
 
   PasswordForm expected_update = duplicate;
   expected_update.password_value = ASCIIToUTF16(kNewPassword);
-  PasswordForm pending = CreatePending("nameofuser", kNewPassword);
 
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
   EXPECT_CALL(*mock_store_, UpdateLogin(expected_update));
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, {&duplicate}, ASCIIToUTF16(kOldPassword));
+  SaveCredential(CreatePending("nameofuser", kNewPassword), {&duplicate},
+                 ASCIIToUTF16(kOldPassword));
 }
 
 // Stores a credential and makes sure that its PSL duplicate is updated.
-TEST_F(FormSaverImplTest, Save_AndUpdatePasswordValuesOnPSLMatch) {
+TEST_P(FormSaverImplSaveTest, Write_AndUpdatePasswordValuesOnPSLMatch) {
   constexpr char kOldPassword[] = "old_password";
   constexpr char kNewPassword[] = "new_password";
-  PasswordForm pending = CreatePending("nameofuser", kOldPassword);
 
-  PasswordForm duplicate = pending;
+  PasswordForm duplicate = CreatePending("nameofuser", kOldPassword);
   duplicate.origin = GURL("https://www.example.in");
   duplicate.signon_realm = duplicate.origin.spec();
   duplicate.is_public_suffix_match = true;
 
   PasswordForm expected_update = duplicate;
   expected_update.password_value = ASCIIToUTF16(kNewPassword);
-  pending.password_value = ASCIIToUTF16(kNewPassword);
-
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
   EXPECT_CALL(*mock_store_, UpdateLogin(expected_update));
-  EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, {&duplicate}, ASCIIToUTF16(kOldPassword));
+  SaveCredential(CreatePending("nameofuser", kNewPassword), {&duplicate},
+                 ASCIIToUTF16(kOldPassword));
 }
 
 // Stores a credential and makes sure that not exact matches are not updated.
-TEST_F(FormSaverImplTest, Save_AndUpdatePasswordValues_IgnoreNonMatches) {
+TEST_P(FormSaverImplSaveTest, Write_AndUpdatePasswordValues_IgnoreNonMatches) {
   constexpr char kOldPassword[] = "old_password";
   constexpr char kNewPassword[] = "new_password";
   PasswordForm pending = CreatePending("nameofuser", kOldPassword);
@@ -377,24 +303,13 @@ TEST_F(FormSaverImplTest, Save_AndUpdatePasswordValues_IgnoreNonMatches) {
       &different_username, &different_password, &empty_username};
 
   pending.password_value = ASCIIToUTF16(kNewPassword);
-
-  EXPECT_CALL(*mock_store_, AddLogin(pending));
   EXPECT_CALL(*mock_store_, UpdateLogin(_)).Times(0);
   EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
-  EXPECT_CALL(*mock_store_, RemoveLogin(_)).Times(0);
-  form_saver_.Save(pending, matches, ASCIIToUTF16(kOldPassword));
-}
-
-// Check that Remove() method is relayed properly.
-TEST_F(FormSaverImplTest, Remove) {
-  PasswordForm form = CreatePending("nameofuser", "wordToP4a55");
-
-  EXPECT_CALL(*mock_store_, RemoveLogin(form));
-  form_saver_.Remove(form);
+  SaveCredential(pending, matches, ASCIIToUTF16(kOldPassword));
 }
 
 // Check that on saving the pending form |form_data| is sanitized.
-TEST_F(FormSaverImplTest, FormDataSanitized) {
+TEST_P(FormSaverImplSaveTest, FormDataSanitized) {
   PasswordForm pending = CreatePending("nameofuser", "wordToP4a55");
   FormFieldData field;
   field.name = ASCIIToUTF16("name");
@@ -408,8 +323,21 @@ TEST_F(FormSaverImplTest, FormDataSanitized) {
   pending.form_data.fields.push_back(field);
 
   PasswordForm saved;
-  EXPECT_CALL(*mock_store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved));
-  form_saver_.Save(pending, {}, base::string16());
+  switch (GetParam()) {
+    case SaveOperation::kSave:
+      EXPECT_CALL(*mock_store_, AddLogin(_)).WillOnce(SaveArg<0>(&saved));
+      return form_saver_.Save(std::move(pending), {}, ASCIIToUTF16(""));
+    case SaveOperation::kUpdate:
+      EXPECT_CALL(*mock_store_, UpdateLogin(_)).WillOnce(SaveArg<0>(&saved));
+      return form_saver_.Update(std::move(pending), {}, ASCIIToUTF16(""));
+    case SaveOperation::kReplaceUpdate: {
+      PasswordForm old_key = CreatePending("some_other_username", "1234");
+      EXPECT_CALL(*mock_store_, UpdateLoginWithPrimaryKey(_, old_key))
+          .WillOnce(SaveArg<0>(&saved));
+      return form_saver_.UpdateReplace(std::move(pending), {}, ASCIIToUTF16(""),
+                                       old_key);
+    }
+  }
 
   ASSERT_EQ(1u, saved.form_data.fields.size());
   const FormFieldData& saved_field = saved.form_data.fields[0];
@@ -423,4 +351,42 @@ TEST_F(FormSaverImplTest, FormDataSanitized) {
   EXPECT_TRUE(saved_field.css_classes.empty());
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         FormSaverImplSaveTest,
+                         ::testing::Values(SaveOperation::kSave,
+                                           SaveOperation::kUpdate,
+                                           SaveOperation::kReplaceUpdate));
+
+// Check that blacklisting an observed form sets the right properties and calls
+// the PasswordStore.
+TEST_F(FormSaverImplTest, PermanentlyBlacklist) {
+  PasswordForm observed = CreateObserved();
+  observed.blacklisted_by_user = false;
+  observed.preferred = true;
+  observed.username_value = ASCIIToUTF16("user1");
+  observed.username_element = ASCIIToUTF16("user");
+  observed.password_value = ASCIIToUTF16("12345");
+  observed.password_element = ASCIIToUTF16("password");
+  observed.other_possible_usernames = {
+      {ASCIIToUTF16("user2"), ASCIIToUTF16("field")}};
+  observed.origin = GURL("https://www.example.com/foobar");
+
+  PasswordForm blacklisted =
+      password_manager_util::MakeNormalizedBlacklistedForm(
+          PasswordStore::FormDigest(observed));
+
+  EXPECT_CALL(*mock_store_, AddLogin(FormWithSomeDate(blacklisted)));
+  PasswordForm result =
+      form_saver_.PermanentlyBlacklist(PasswordStore::FormDigest(observed));
+  EXPECT_THAT(result, FormWithSomeDate(blacklisted));
+}
+
+// Check that Remove() method is relayed properly.
+TEST_F(FormSaverImplTest, Remove) {
+  PasswordForm form = CreatePending("nameofuser", "wordToP4a55");
+
+  EXPECT_CALL(*mock_store_, RemoveLogin(form));
+  form_saver_.Remove(form);
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/new_password_form_manager.cc

@@ -267,21 +267,10 @@ void NewPasswordFormManager::Save() {
     pending_credentials_.date_created = base::Time::Now();
     votes_uploader_.SendVotesOnSave(observed_form_, *parsed_submitted_form_,
                                     best_matches_, &pending_credentials_);
-    base::string16 old_password;
-    if (password_overridden_) {
-      // |pending_credentials_| contains not only a new credential that is to be
-      // stored, but it also implies that existing credentials in the store need
-      // to get the password updated.
-      const PasswordForm* saved_form =
-          password_manager_util::GetMatchForUpdating(*parsed_submitted_form_,
-                                                     best_matches_);
-      DCHECK(saved_form);
-      old_password = saved_form->password_value;
-    }
-    SavePendingToStore(false /*update*/, old_password);
+    SavePendingToStore(false /*update*/);
   } else {
     ProcessUpdate();
-    SavePendingToStore(true /*update*/, base::string16());
+    SavePendingToStore(true /*update*/);
   }
 
   if (pending_credentials_.times_used == 1 &&
@@ -311,7 +300,7 @@ void NewPasswordFormManager::Update(const PasswordForm& credentials_to_update) {
   pending_credentials_.preferred = true;
   is_new_login_ = false;
   ProcessUpdate();
-  SavePendingToStore(true /*update*/, base::string16());
+  SavePendingToStore(true /*update*/);
 
   client_->UpdateFormManagers();
 }
@@ -406,7 +395,8 @@ void NewPasswordFormManager::PermanentlyBlacklist() {
     }
     blacklisted_matches_.push_back(new_blacklisted_.get());
   }
-  form_saver_->PermanentlyBlacklist(new_blacklisted_.get());
+  *new_blacklisted_ = form_saver_->PermanentlyBlacklist(
+      PasswordStore::FormDigest(*new_blacklisted_));
 }
 
 void NewPasswordFormManager::OnPasswordsRevealed() {
@@ -988,29 +978,6 @@ void NewPasswordFormManager::FillHttpAuth() {
   client_->AutofillHttpAuth(best_matches_, *preferred_match_);
 }
 
-std::vector<PasswordForm> NewPasswordFormManager::FindOtherCredentialsToUpdate()
-    const {
-  std::vector<autofill::PasswordForm> credentials_to_update;
-  if (!pending_credentials_.federation_origin.opaque())
-    return credentials_to_update;
-
-  auto updated_password_it =
-      best_matches_.find(pending_credentials_.username_value);
-  DCHECK(best_matches_.end() != updated_password_it);
-  const base::string16& old_password =
-      updated_password_it->second->password_value;
-  for (auto* not_best_match : not_best_matches_) {
-    if (not_best_match->username_value == pending_credentials_.username_value &&
-        not_best_match->password_value == old_password) {
-      credentials_to_update.push_back(*not_best_match);
-      credentials_to_update.back().password_value =
-          pending_credentials_.password_value;
-    }
-  }
-
-  return credentials_to_update;
-}
-
 std::unique_ptr<PasswordForm> NewPasswordFormManager::ParseFormAndMakeLogging(
     const FormData& form,
     FormDataParser::Mode mode) {
@@ -1108,17 +1075,18 @@ std::vector<const PasswordForm*> NewPasswordFormManager::GetAllMatches() const {
   return result;
 }
 
-void NewPasswordFormManager::SavePendingToStore(
-    bool update,
-    const base::string16& old_password) {
+void NewPasswordFormManager::SavePendingToStore(bool update) {
+  const PasswordForm* saved_form = password_manager_util::GetMatchForUpdating(
+      *parsed_submitted_form_, best_matches_);
+  if (update || password_overridden_)
+    DCHECK(saved_form);
+  base::string16 old_password =
+      saved_form ? saved_form->password_value : base::string16();
   if (HasGeneratedPassword()) {
     generation_state_->CommitGeneratedPassword(pending_credentials_,
-                                               best_matches_, nullptr);
+                                               GetAllMatches(), old_password);
   } else if (update) {
-    std::vector<PasswordForm> credentials_to_update =
-        FindOtherCredentialsToUpdate();
-    form_saver_->Update(pending_credentials_, best_matches_,
-                        &credentials_to_update, nullptr);
+    form_saver_->Update(pending_credentials_, GetAllMatches(), old_password);
   } else {
     form_saver_->Save(pending_credentials_, GetAllMatches(), old_password);
   }

components/password_manager/core/browser/new_password_form_manager.h

@@ -250,13 +250,6 @@ class NewPasswordFormManager : public PasswordFormManagerInterface,
   // Sends fill data to the http auth popup.
   void FillHttpAuth();
 
-  // Goes through |not_best_matches_|, updates the password of those which share
-  // the old password and username with |pending_credentials_| to the new
-  // password of |pending_credentials_|, and returns copies of all such modified
-  // credentials.
-  // TODO(crbug/831123): remove. FormSaver should do the job.
-  std::vector<autofill::PasswordForm> FindOtherCredentialsToUpdate() const;
-
   // Helper function for calling form parsing and logging results if logging is
   // active.
   std::unique_ptr<autofill::PasswordForm> ParseFormAndMakeLogging(
@@ -276,9 +269,8 @@ class NewPasswordFormManager : public PasswordFormManagerInterface,
   // and |not_best_matches_|).
   std::vector<const autofill::PasswordForm*> GetAllMatches() const;
 
-  // Save/update |pending_credentials_| to the password store. If |old_password|
-  // is provided, then the corresponding credentials are updated too.
-  void SavePendingToStore(bool update, const base::string16& old_password);
+  // Save/update |pending_credentials_| to the password store.
+  void SavePendingToStore(bool update);
 
   // The client which implements embedder-specific PasswordManager operations.
   PasswordManagerClient* client_;

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -18,6 +18,7 @@
 #include "components/autofill/core/common/password_form_generation_data.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "components/password_manager/core/browser/fake_form_fetcher.h"
+#include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/stub_form_saver.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/browser/stub_password_manager_driver.h"
@@ -41,12 +42,15 @@ using base::TestMockTimeTaskRunner;
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
+using testing::ElementsAre;
 using testing::IsEmpty;
 using testing::Mock;
 using testing::NiceMock;
+using testing::Pointee;
 using testing::Return;
 using testing::SaveArg;
 using testing::SaveArgPointee;
+using testing::UnorderedElementsAre;
 
 namespace password_manager {
 
@@ -56,6 +60,10 @@ namespace {
 const int kUsernameFieldIndex = 1;
 const int kPasswordFieldIndex = 2;
 
+MATCHER_P(FormHasUniqueKey, key, "") {
+  return ArePasswordFormUniqueKeyEqual(arg, key);
+}
+
 class MockPasswordManagerDriver : public StubPasswordManagerDriver {
  public:
   MockPasswordManagerDriver() {}
@@ -189,18 +197,21 @@ class MockFormSaver : public StubFormSaver {
   ~MockFormSaver() override = default;
 
   // FormSaver:
-  MOCK_METHOD1(PermanentlyBlacklist, void(autofill::PasswordForm* observed));
+  MOCK_METHOD1(PermanentlyBlacklist, PasswordForm(PasswordStore::FormDigest));
   MOCK_METHOD3(Save,
-               void(const autofill::PasswordForm& pending,
-                    const std::vector<const autofill::PasswordForm*>& matches,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
                     const base::string16& old_password));
-  MOCK_METHOD4(
-      Update,
-      void(const autofill::PasswordForm& pending,
-           const std::map<base::string16, const PasswordForm*>& best_matches,
-           const std::vector<autofill::PasswordForm>* credentials_to_update,
-           const autofill::PasswordForm* old_primary_key));
-  MOCK_METHOD1(Remove, void(const autofill::PasswordForm&));
+  MOCK_METHOD3(Update,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
+                    const base::string16& old_password));
+  MOCK_METHOD4(UpdateReplace,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
+                    const base::string16& old_password,
+                    const PasswordForm& old_unique_key));
+  MOCK_METHOD1(Remove, void(const PasswordForm&));
 
   std::unique_ptr<FormSaver> Clone() override {
     return std::make_unique<MockFormSaver>();
@@ -929,21 +940,15 @@ TEST_F(NewPasswordFormManagerTest, OverridePassword) {
 
   MockFormSaver& form_saver = MockFormSaver::Get(form_manager_.get());
   PasswordForm updated_form;
-  std::map<base::string16, const PasswordForm*> best_matches;
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(form_saver, Update(_, _, _, nullptr))
-      .WillOnce(DoAll(SaveArg<0>(&updated_form), SaveArg<1>(&best_matches),
-                      SaveArgPointee<2>(&credentials_to_update)));
+  EXPECT_CALL(form_saver, Update(_, ElementsAre(Pointee(saved_match_)),
+                                 saved_match_.password_value))
+      .WillOnce(SaveArg<0>(&updated_form));
 
   form_manager_->Save();
 
   EXPECT_TRUE(ArePasswordFormUniqueKeyEqual(saved_match_, updated_form));
   EXPECT_TRUE(updated_form.preferred);
   EXPECT_EQ(new_password, updated_form.password_value);
-  EXPECT_EQ(1u, best_matches.size());
-  ASSERT_TRUE(best_matches.find(username) != best_matches.end());
-  EXPECT_EQ(saved_match_, *best_matches[username]);
-  EXPECT_TRUE(credentials_to_update.empty());
 }
 
 // Tests that when the user changes password on a change password form then the
@@ -971,29 +976,19 @@ TEST_F(NewPasswordFormManagerTest, UpdatePasswordOnChangePasswordForm) {
 
   MockFormSaver& form_saver = MockFormSaver::Get(form_manager_.get());
   PasswordForm updated_form;
-  std::map<base::string16, const PasswordForm*> best_matches;
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(form_saver, Update(_, _, _, nullptr))
-      .WillOnce(DoAll(SaveArg<0>(&updated_form), SaveArg<1>(&best_matches),
-                      SaveArgPointee<2>(&credentials_to_update)));
+  EXPECT_CALL(form_saver,
+              Update(_,
+                     UnorderedElementsAre(
+                         Pointee(saved_match_), Pointee(not_best_saved_match),
+                         Pointee(saved_match_another_username)),
+                     saved_match_.password_value))
+      .WillOnce(SaveArg<0>(&updated_form));
 
   form_manager_->Save();
 
   EXPECT_TRUE(ArePasswordFormUniqueKeyEqual(saved_match_, updated_form));
   EXPECT_TRUE(updated_form.preferred);
   EXPECT_EQ(new_password, updated_form.password_value);
-
-  EXPECT_EQ(2u, best_matches.size());
-  base::string16 username = saved_match_.username_value;
-  ASSERT_TRUE(best_matches.find(username) != best_matches.end());
-  EXPECT_EQ(saved_match_, *best_matches[username]);
-  base::string16 another_username = saved_match_another_username.username_value;
-  ASSERT_TRUE(best_matches.find(another_username) != best_matches.end());
-  EXPECT_EQ(saved_match_another_username, *best_matches[another_username]);
-
-  ASSERT_EQ(1u, credentials_to_update.size());
-  not_best_saved_match.password_value = new_password;
-  EXPECT_EQ(not_best_saved_match, credentials_to_update[0]);
 }
 
 TEST_F(NewPasswordFormManagerTest, VotesUploadingOnPasswordUpdate) {
@@ -1200,15 +1195,16 @@ TEST_F(NewPasswordFormManagerTest, PermanentlyBlacklist) {
 
   MockFormSaver& form_saver = MockFormSaver::Get(form_manager_.get());
 
-  PasswordForm* new_blacklisted_form = nullptr;
-  EXPECT_CALL(form_saver, PermanentlyBlacklist(_))
-      .WillOnce(SaveArg<0>(&new_blacklisted_form));
-
+  PasswordForm actual_blacklisted_form =
+      password_manager_util::MakeNormalizedBlacklistedForm(
+          PasswordStore::FormDigest(observed_form_));
+  EXPECT_CALL(form_saver,
+              PermanentlyBlacklist(PasswordStore::FormDigest(observed_form_)))
+      .WillOnce(Return(actual_blacklisted_form));
+  ;
   form_manager_->PermanentlyBlacklist();
-  ASSERT_TRUE(new_blacklisted_form);
-  EXPECT_EQ(observed_form_.url, new_blacklisted_form->origin);
-  EXPECT_EQ(GetSignonRealm(observed_form_.url),
-            new_blacklisted_form->signon_realm);
+  EXPECT_THAT(form_manager_->GetBlacklistedMatches(),
+              ElementsAre(Pointee(actual_blacklisted_form)));
 }
 
 TEST_F(NewPasswordFormManagerTest, Clone) {
@@ -1382,7 +1378,9 @@ TEST_F(NewPasswordFormManagerTest, PresaveGeneratedPasswordEmptyStore) {
   form_with_generated_password.password_value += ASCIIToUTF16("1");
   form_data.fields[kPasswordFieldIndex].value =
       form_with_generated_password.password_value;
-  EXPECT_CALL(form_saver, Update(_, IsEmpty(), nullptr, _))
+  EXPECT_CALL(form_saver,
+              UpdateReplace(_, IsEmpty(), ASCIIToUTF16(""),
+                            FormHasUniqueKey(form_with_generated_password)))
       .WillOnce(SaveArg<0>(&saved_form));
 
   form_manager_->PresaveGeneratedPassword(form_with_generated_password);
@@ -1416,7 +1414,8 @@ TEST_F(NewPasswordFormManagerTest, PresaveGenerated_ModifiedUsername) {
       true /* generation_popup_was_shown */, false /* is_manual_generation */);
 
   // Check that the generated password is presaved.
-  EXPECT_CALL(form_saver, Save(_, _, _));
+  PasswordForm saved_form;
+  EXPECT_CALL(form_saver, Save(_, _, _)).WillOnce(SaveArg<0>(&saved_form));
   PasswordForm form_with_generated_password = parsed_submitted_form_;
   FormData& form_data = form_with_generated_password.form_data;
   form_manager_->PresaveGeneratedPassword(form_with_generated_password);
@@ -1426,10 +1425,10 @@ TEST_F(NewPasswordFormManagerTest, PresaveGenerated_ModifiedUsername) {
   form_with_generated_password.username_value += ASCIIToUTF16("1");
   form_data.fields[kUsernameFieldIndex].value =
       form_with_generated_password.username_value;
-  PasswordForm saved_form;
-  EXPECT_CALL(form_saver, Update(_, _, nullptr, _))
-      .WillOnce(SaveArg<0>(&saved_form));
 
+  EXPECT_CALL(form_saver, UpdateReplace(_, IsEmpty(), ASCIIToUTF16(""),
+                                        FormHasUniqueKey(saved_form)))
+      .WillOnce(SaveArg<0>(&saved_form));
   form_manager_->PresaveGeneratedPassword(form_with_generated_password);
 
   EXPECT_TRUE(form_manager_->HasGeneratedPassword());
@@ -1472,7 +1471,9 @@ TEST_F(NewPasswordFormManagerTest, GeneratedPasswordWhichIsNotInFormData) {
   EXPECT_TRUE(form_manager_->HasGeneratedPassword());
 
   // Check that the generated password is saved.
-  EXPECT_CALL(form_saver, Update(_, _, _, _)).WillOnce(SaveArg<0>(&saved_form));
+  EXPECT_CALL(form_saver, UpdateReplace(_, IsEmpty(), ASCIIToUTF16(""),
+                                        FormHasUniqueKey(saved_form)))
+      .WillOnce(SaveArg<0>(&saved_form));
   EXPECT_CALL(client_, UpdateFormManagers());
 
   EXPECT_TRUE(form_manager_->ProvisionallySave(submitted_form_, &driver_));
@@ -1712,11 +1713,12 @@ TEST_F(NewPasswordFormManagerTest, Update) {
 
   MockFormSaver& form_saver = MockFormSaver::Get(form_manager_.get());
   PasswordForm updated_form;
-  std::map<base::string16, const PasswordForm*> best_matches;
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(form_saver, Update(_, _, _, nullptr))
-      .WillOnce(DoAll(SaveArg<0>(&updated_form), SaveArg<1>(&best_matches),
-                      SaveArgPointee<2>(&credentials_to_update)));
+  EXPECT_CALL(form_saver, Update(_,
+                                 UnorderedElementsAre(
+                                     Pointee(saved_match_),
+                                     Pointee(saved_match_another_username)),
+                                 saved_match_.password_value))
+      .WillOnce(SaveArg<0>(&updated_form));
   EXPECT_CALL(client_, UpdateFormManagers());
 
   form_manager_->Update(saved_match_);
@@ -1724,10 +1726,6 @@ TEST_F(NewPasswordFormManagerTest, Update) {
   EXPECT_TRUE(ArePasswordFormUniqueKeyEqual(saved_match_, updated_form));
   EXPECT_TRUE(updated_form.preferred);
   EXPECT_EQ(new_password, updated_form.password_value);
-  EXPECT_EQ(2u, best_matches.size());
-  ASSERT_TRUE(best_matches.find(username) != best_matches.end());
-  EXPECT_EQ(saved_match_, *best_matches[username]);
-  EXPECT_TRUE(credentials_to_update.empty());
 }
 
 // TODO(https://crbug.com/918846): implement FillingAssistance metric on iOS.
@@ -1923,20 +1921,15 @@ TEST_F(NewPasswordFormManagerTest, HTTPAuthPasswordOverridden) {
 
   // Check that the password is updated in the stored credential.
   PasswordForm updated_form;
-  std::map<base::string16, const PasswordForm*> best_matches;
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(form_saver, Update(_, _, _, nullptr))
-      .WillOnce(DoAll(SaveArg<0>(&updated_form), SaveArg<1>(&best_matches),
-                      SaveArgPointee<2>(&credentials_to_update)));
+  EXPECT_CALL(form_saver,
+              Update(_, ElementsAre(Pointee(saved_http_auth_form)), password))
+      .WillOnce(SaveArg<0>(&updated_form));
 
   form_manager_->Save();
 
   EXPECT_TRUE(
       ArePasswordFormUniqueKeyEqual(saved_http_auth_form, updated_form));
   EXPECT_EQ(new_password, updated_form.password_value);
-  ASSERT_TRUE(best_matches.find(username) != best_matches.end());
-  EXPECT_EQ(saved_http_auth_form, *best_matches[username]);
-  EXPECT_TRUE(credentials_to_update.empty());
 }
 
 TEST_F(NewPasswordFormManagerTest, BlacklistHttpAuthCredentials) {
@@ -1955,13 +1948,9 @@ TEST_F(NewPasswordFormManagerTest, BlacklistHttpAuthCredentials) {
 
   // Simulate that the user clicks never.
   PasswordForm blacklisted_form;
-  EXPECT_CALL(form_saver, PermanentlyBlacklist(_))
-      .WillOnce(SaveArgPointee<0>(&blacklisted_form));
+  EXPECT_CALL(form_saver,
+              PermanentlyBlacklist(PasswordStore::FormDigest(http_auth_form)));
   form_manager_->OnNeverClicked();
-
-  EXPECT_EQ(http_auth_form.signon_realm, blacklisted_form.signon_realm);
-  EXPECT_EQ(http_auth_form.origin, blacklisted_form.origin);
-  EXPECT_EQ(http_auth_form.scheme, blacklisted_form.scheme);
 }
 
 }  // namespace

components/password_manager/core/browser/password_form_manager.cc

@@ -243,10 +243,11 @@ void PasswordFormManager::PermanentlyBlacklist() {
   DCHECK(!client_->IsIncognito());
 
   if (!new_blacklisted_) {
-    new_blacklisted_ = std::make_unique<PasswordForm>(observed_form_);
+    new_blacklisted_ = std::make_unique<PasswordForm>();
     blacklisted_matches_.push_back(new_blacklisted_.get());
   }
-  form_saver_->PermanentlyBlacklist(new_blacklisted_.get());
+  *new_blacklisted_ = form_saver_->PermanentlyBlacklist(
+      PasswordStore::FormDigest(observed_form_));
 }
 
 bool PasswordFormManager::IsNewLogin() const {
@@ -1035,19 +1036,26 @@ std::vector<PasswordForm> PasswordFormManager::FindOtherCredentialsToUpdate()
 }
 
 void PasswordFormManager::SavePendingToStore(bool update) {
+  std::vector<const autofill::PasswordForm*> matches;
+  for (const auto& match : best_matches_)
+    matches.push_back(match.second);
+  matches.insert(matches.end(), not_best_matches_.begin(),
+                 not_best_matches_.end());
   if (HasGeneratedPassword()) {
-    generation_state_->CommitGeneratedPassword(pending_credentials_,
-                                               best_matches_, nullptr);
+    generation_state_->CommitGeneratedPassword(pending_credentials_, matches,
+                                               base::string16());
   } else if (update) {
-    std::vector<PasswordForm> credentials_to_update =
-        FindOtherCredentialsToUpdate();
-    form_saver_->Update(pending_credentials_, best_matches_,
-                        &credentials_to_update, nullptr);
+    base::string16 old_password;
+    if (!pending_credentials_.IsFederatedCredential()) {
+      auto updated_password_it =
+          best_matches_.find(pending_credentials_.username_value);
+      DCHECK(best_matches_.end() != updated_password_it);
+      old_password = updated_password_it->second->password_value;
+    }
+
+    form_saver_->Update(pending_credentials_, matches, old_password);
   } else {
-    std::vector<const autofill::PasswordForm*> best_matches;
-    for (const auto& match : best_matches_)
-      best_matches.push_back(match.second);
-    form_saver_->Save(pending_credentials_, best_matches, base::string16());
+    form_saver_->Save(pending_credentials_, matches, base::string16());
   }
 }
 

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -34,6 +34,7 @@
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/statistics_table.h"
 #include "components/password_manager/core/browser/stub_credentials_filter.h"
 #include "components/password_manager/core/browser/stub_form_saver.h"
@@ -116,18 +117,21 @@ class MockFormSaver : public StubFormSaver {
   ~MockFormSaver() override = default;
 
   // FormSaver:
-  MOCK_METHOD1(PermanentlyBlacklist, void(autofill::PasswordForm* observed));
+  MOCK_METHOD1(PermanentlyBlacklist, PasswordForm(PasswordStore::FormDigest));
   MOCK_METHOD3(Save,
-               void(const autofill::PasswordForm& pending,
-                    const std::vector<const autofill::PasswordForm*>& matches,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
                     const base::string16& old_password));
-  MOCK_METHOD4(
-      Update,
-      void(const autofill::PasswordForm& pending,
-           const std::map<base::string16, const PasswordForm*>& best_matches,
-           const std::vector<autofill::PasswordForm>* credentials_to_update,
-           const autofill::PasswordForm* old_primary_key));
-  MOCK_METHOD1(Remove, void(const autofill::PasswordForm&));
+  MOCK_METHOD3(Update,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
+                    const base::string16& old_password));
+  MOCK_METHOD4(UpdateReplace,
+               void(PasswordForm pending,
+                    const std::vector<const PasswordForm*>& matches,
+                    const base::string16& old_password,
+                    const PasswordForm& old_unique_key));
+  MOCK_METHOD1(Remove, void(const PasswordForm&));
 
   std::unique_ptr<FormSaver> Clone() override {
     return std::make_unique<MockFormSaver>();
@@ -1124,18 +1128,21 @@ TEST_F(PasswordFormManagerTest, TestBlacklist) {
             form_manager()->GetPendingCredentials().signon_realm);
 
   const PasswordForm pending_form = form_manager()->GetPendingCredentials();
-  PasswordForm actual_add_form;
+  PasswordForm actual_blacklisted_form =
+      password_manager_util::MakeNormalizedBlacklistedForm(
+          PasswordStore::FormDigest(*observed_form()));
   // Now pretend the user wants to never save passwords on this origin. Chrome
   // is supposed to only request blacklisting of a single form.
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), PermanentlyBlacklist(_))
-      .WillOnce(SaveArgPointee<0>(&actual_add_form));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()),
+              PermanentlyBlacklist(PasswordStore::FormDigest(*observed_form())))
+      .WillOnce(Return(actual_blacklisted_form));
   form_manager()->PermanentlyBlacklist();
   EXPECT_EQ(pending_form, form_manager()->GetPendingCredentials());
   // The PasswordFormManager should have updated its knowledge of blacklisting
   // without waiting for PasswordStore updates.
   EXPECT_TRUE(form_manager()->IsBlacklisted());
   EXPECT_THAT(form_manager()->GetBlacklistedMatches(),
-              UnorderedElementsAre(Pointee(actual_add_form)));
+              ElementsAre(Pointee(actual_blacklisted_form)));
 }
 
 // Test that stored blacklisted forms are correctly evaluated for whether they
@@ -1351,7 +1358,7 @@ TEST_F(PasswordFormManagerTest, TestUpdatePasswordFromNewPasswordElement) {
 
   // Trigger saving to exercise some special case handling for updating.
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
 
   form_manager.Save();
@@ -1463,7 +1470,7 @@ TEST_F(PasswordFormManagerTest, TestAlternateUsername_NoChange) {
   EXPECT_FALSE(form_manager()->IsNewLogin());
 
   PasswordForm saved_result;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
       .WillOnce(SaveArg<0>(&saved_result));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
@@ -1704,7 +1711,7 @@ TEST_F(PasswordFormManagerTest, AndroidCredentialsAreAutofilled) {
   EXPECT_FALSE(form_manager()->IsNewLogin());
 
   PasswordForm updated_credential;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
       .WillOnce(testing::SaveArg<0>(&updated_credential));
   form_manager()->Save();
 
@@ -1954,10 +1961,10 @@ TEST_F(PasswordFormManagerTest, CorrectlyUpdatePasswordsWithSameUsername) {
   EXPECT_FALSE(form_manager()->IsNewLogin());
 
   PasswordForm saved_result;
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
-      .WillOnce(testing::DoAll(SaveArg<0>(&saved_result),
-                               SaveArgPointee<2>(&credentials_to_update)));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()),
+              Update(_, UnorderedElementsAre(&first, &second, &third),
+                     first.password_value))
+      .WillOnce(SaveArg<0>(&saved_result));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
 
@@ -1967,12 +1974,6 @@ TEST_F(PasswordFormManagerTest, CorrectlyUpdatePasswordsWithSameUsername) {
   EXPECT_EQ(ASCIIToUTF16("third"), saved_result.password_value);
   EXPECT_FALSE(saved_result.password_element.empty());
   EXPECT_FALSE(saved_result.username_element.empty());
-
-  // What was |second| above should be another credential updated.
-  ASSERT_EQ(1u, credentials_to_update.size());
-  EXPECT_EQ(ASCIIToUTF16("third"), credentials_to_update[0].password_value);
-  EXPECT_FALSE(credentials_to_update[0].password_element.empty());
-  EXPECT_TRUE(credentials_to_update[0].username_element.empty());
 }
 
 TEST_F(PasswordFormManagerTest, UploadFormData_NewPassword) {
@@ -2240,7 +2241,7 @@ TEST_F(PasswordFormManagerTest, TestUpdateMethod) {
 
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _))
       .WillOnce(SaveArg<0>(&new_credentials));
 
   form_manager.Update(*saved_match());
@@ -2297,7 +2298,7 @@ TEST_F(PasswordFormManagerTest, TestUpdateNoUsernameTextfieldPresent) {
 
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _))
       .WillOnce(SaveArg<0>(&new_credentials));
 
   form_manager.Update(form_manager.GetPendingCredentials());
@@ -2435,10 +2436,8 @@ TEST_F(PasswordFormManagerTest, UpdateUsername_ValueSavedInStore) {
 
     // User clicks save, edited username is saved, password updated.
     EXPECT_CALL(MockFormSaver::Get(form_manager()),
-                Update(expected_pending,
-                       ElementsAre(Pair(saved_match()->username_value,
-                                        Pointee(*saved_match()))),
-                       Pointee(IsEmpty()), nullptr));
+                Update(expected_pending, ElementsAre(Pointee(*saved_match())),
+                       saved_match()->password_value));
     // Expect a username reusing vote.
     FieldTypeMap expected_types;
     expected_types[ASCIIToUTF16("full_name")] = autofill::UNKNOWN_TYPE;
@@ -2710,7 +2709,7 @@ TEST_F(PasswordFormManagerTest, GenerationStatusChangedWithPassword) {
   form_manager()->ProvisionallySave(submitted_form);
 
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
   form_manager()->Save();
 
@@ -2733,7 +2732,7 @@ TEST_F(PasswordFormManagerTest, GenerationStatusNotUpdatedIfPasswordUnchanged) {
   form_manager()->ProvisionallySave(submitted_form);
 
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
   form_manager()->Save();
 
@@ -2745,7 +2744,7 @@ TEST_F(PasswordFormManagerTest, GenerationStatusNotUpdatedIfPasswordUnchanged) {
   generated_form.times_used = 1;
   SetNonFederatedAndNotifyFetchCompleted({&generated_form});
   form_manager()->ProvisionallySave(submitted_form);
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, _));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _));
   form_manager()->Save();
   histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
                                      metrics_util::PASSWORD_USED, 1);
@@ -2768,7 +2767,7 @@ TEST_F(PasswordFormManagerTest, GeneratedPasswordIsOverridden) {
   form_manager()->ProvisionallySave(submitted_form);
 
   PasswordForm new_credentials;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
   form_manager()->Save();
 
@@ -2780,7 +2779,7 @@ TEST_F(PasswordFormManagerTest, GeneratedPasswordIsOverridden) {
   // On the reuse of the overriden password, the metric is not reported.
   SetNonFederatedAndNotifyFetchCompleted({&new_credentials});
   form_manager()->ProvisionallySave(new_credentials);
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, _));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _));
   form_manager()->Save();
   histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
                                      metrics_util::PASSWORD_OVERRIDDEN, 1);
@@ -2894,10 +2893,11 @@ TEST_F(PasswordFormManagerTest, TestUpdatePSLMatchedCredentials) {
 
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
-  std::vector<autofill::PasswordForm> credentials_to_update;
-  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _, nullptr))
-      .WillOnce(testing::DoAll(SaveArg<0>(&new_credentials),
-                               SaveArgPointee<2>(&credentials_to_update)));
+  std::vector<const autofill::PasswordForm*> matches;
+  EXPECT_CALL(MockFormSaver::Get(&form_manager),
+              Update(_, _, saved_match()->password_value))
+      .WillOnce(
+          testing::DoAll(SaveArg<0>(&new_credentials), SaveArg<1>(&matches)));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
 
@@ -2910,16 +2910,8 @@ TEST_F(PasswordFormManagerTest, TestUpdatePSLMatchedCredentials) {
   EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
   EXPECT_EQ(saved_match()->origin, new_credentials.origin);
 
-  ASSERT_EQ(1u, credentials_to_update.size());
-  EXPECT_EQ(credentials.password_value,
-            credentials_to_update[0].password_value);
-  EXPECT_EQ(psl_saved_match()->username_element,
-            credentials_to_update[0].username_element);
-  EXPECT_EQ(psl_saved_match()->username_element,
-            credentials_to_update[0].username_element);
-  EXPECT_EQ(psl_saved_match()->password_element,
-            credentials_to_update[0].password_element);
-  EXPECT_EQ(psl_saved_match()->origin, credentials_to_update[0].origin);
+  EXPECT_THAT(matches, UnorderedElementsAre(Pointee(*saved_match()),
+                                            Pointee(*psl_saved_match())));
 }
 
 TEST_F(PasswordFormManagerTest,
@@ -2949,7 +2941,10 @@ TEST_F(PasswordFormManagerTest,
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
   EXPECT_CALL(MockFormSaver::Get(&form_manager),
-              Update(_, _, Pointee(IsEmpty()), nullptr))
+              Update(_,
+                     UnorderedElementsAre(Pointee(*saved_match()),
+                                          Pointee(*psl_saved_match())),
+                     saved_match()->password_value))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
@@ -2991,7 +2986,10 @@ TEST_F(PasswordFormManagerTest,
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
   EXPECT_CALL(MockFormSaver::Get(&form_manager),
-              Update(_, _, Pointee(IsEmpty()), nullptr))
+              Update(_,
+                     UnorderedElementsAre(Pointee(*saved_match()),
+                                          Pointee(*psl_saved_match())),
+                     saved_match()->password_value))
       .WillOnce(testing::SaveArg<0>(&new_credentials));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
@@ -3217,7 +3215,7 @@ TEST_F(PasswordFormManagerTest,
               StartUploadRequest(SignatureIsSameAs(submitted_form), _, _, _, _,
                                  nullptr));
 
-  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _, _));
+  EXPECT_CALL(MockFormSaver::Get(&form_manager), Update(_, _, _));
   form_manager.Save();
 }
 
@@ -3275,17 +3273,21 @@ TEST_F(PasswordFormManagerTest, PresaveGeneratedPasswordAndRemoveIt) {
   EXPECT_FALSE(form_manager()->generated_password_changed());
 
   // Simulate the user changed the presaved username.
+  PasswordForm saved_form = credentials;
   credentials.username_value = ASCIIToUTF16("new_username");
   EXPECT_CALL(MockFormSaver::Get(form_manager()),
-              Update(FormIgnoreDate(credentials), _, _, _));
+              UpdateReplace(FormIgnoreDate(credentials), _, _,
+                            FormIgnoreDate(saved_form)));
   form_manager()->PresaveGeneratedPassword(credentials);
   EXPECT_TRUE(form_manager()->HasGeneratedPassword());
   EXPECT_FALSE(form_manager()->generated_password_changed());
 
   // Simulate the user changed the presaved password.
+  saved_form = credentials;
   credentials.password_value = ASCIIToUTF16("changed_password");
   EXPECT_CALL(MockFormSaver::Get(form_manager()),
-              Update(FormIgnoreDate(credentials), _, _, _));
+              UpdateReplace(FormIgnoreDate(credentials), _, _,
+                            FormIgnoreDate(saved_form)));
   form_manager()->PresaveGeneratedPassword(credentials);
   EXPECT_TRUE(form_manager()->HasGeneratedPassword());
   EXPECT_TRUE(form_manager()->generated_password_changed());
@@ -3389,17 +3391,18 @@ TEST_F(PasswordFormManagerTest, SkipZeroClickIntact) {
 
   // Trigger saving to exercise some special case handling during updating.
   PasswordForm new_credentials;
-  std::vector<autofill::PasswordForm> credentials_to_update;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
-      .WillOnce(testing::DoAll(SaveArg<0>(&new_credentials),
-                               SaveArgPointee<2>(&credentials_to_update)));
+  std::vector<const autofill::PasswordForm*> matches;
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _))
+      .WillOnce(
+          testing::DoAll(SaveArg<0>(&new_credentials), SaveArg<1>(&matches)));
   EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
               StartUploadRequest(_, false, _, _, true, nullptr));
   form_manager()->Save();
 
   EXPECT_TRUE(new_credentials.skip_zero_click);
-  ASSERT_EQ(1u, credentials_to_update.size());
-  EXPECT_TRUE(credentials_to_update[0].skip_zero_click);
+  ASSERT_EQ(2u, matches.size());
+  EXPECT_TRUE(matches[0]->skip_zero_click);
+  EXPECT_TRUE(matches[1]->skip_zero_click);
 }
 
 TEST_F(PasswordFormManagerFillOnAccountSelectTest, ProcessFrame) {
@@ -3654,10 +3657,8 @@ TEST_F(PasswordFormManagerTest,
     expected_pending.other_possible_usernames.clear();
 
     EXPECT_CALL(MockFormSaver::Get(form_manager()),
-                Update(expected_pending,
-                       ElementsAre(Pair(saved_match()->username_value,
-                                        Pointee(*saved_match()))),
-                       Pointee(IsEmpty()), nullptr));
+                Update(expected_pending, ElementsAre(Pointee(*saved_match())),
+                       saved_match()->password_value));
     EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
                 StartUploadRequest(_, _, _, _, _, nullptr));
     EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(),
@@ -3699,15 +3700,9 @@ TEST_F(PasswordFormManagerTest, ResetStoredMatches) {
   PasswordForm updated(best_match1);
   updated.password_value = ASCIIToUTF16("updated password");
   form_manager()->ProvisionallySave(updated);
-  std::vector<PasswordForm> credentials_to_update;
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
-      .WillOnce(SaveArgPointee<2>(&credentials_to_update));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _));
   form_manager()->Save();
 
-  PasswordForm updated_non_best = non_best_match;
-  updated_non_best.password_value = updated.password_value;
-  EXPECT_THAT(credentials_to_update, UnorderedElementsAre(updated_non_best));
-
   form_manager()->ResetStoredMatches();
 
   EXPECT_THAT(form_manager()->GetBestMatches(), IsEmpty());
@@ -3720,12 +3715,8 @@ TEST_F(PasswordFormManagerTest, ResetStoredMatches) {
   SetNonFederatedAndNotifyFetchCompleted({&best_match1});
 
   form_manager()->ProvisionallySave(updated);
-  credentials_to_update.clear();
-  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
-      .WillOnce(SaveArgPointee<2>(&credentials_to_update));
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _));
   form_manager()->Save();
-
-  EXPECT_THAT(credentials_to_update, IsEmpty());
 }
 
 // Check that on changing FormFetcher, the PasswordFormManager removes itself
@@ -3849,8 +3840,9 @@ TEST_F(PasswordFormManagerTest, Clone_OnNeverClicked) {
 
   std::unique_ptr<PasswordFormManager> clone = form_manager->Clone();
 
-  EXPECT_CALL(MockFormSaver::Get(clone.get()),
-              PermanentlyBlacklist(Pointee(*observed_form())));
+  EXPECT_CALL(
+      MockFormSaver::Get(clone.get()),
+      PermanentlyBlacklist(PasswordStore::FormDigest(*observed_form())));
   clone->OnNeverClicked();
 }
 

components/password_manager/core/browser/password_generation_state.cc

@@ -29,9 +29,9 @@ void PasswordGenerationState::PresaveGeneratedPassword(PasswordForm generated) {
   DCHECK(!generated.password_value.empty());
   generated.date_created = clock_->Now();
   if (presaved_) {
-    form_saver_->Update(generated, {} /* best_matches */,
-                        nullptr /* credentials_to_update */,
-                        &presaved_.value() /* old_primary_key */);
+    form_saver_->UpdateReplace(generated, {} /* matches */,
+                               base::string16() /* old_password */,
+                               presaved_.value() /* old_primary_key */);
   } else {
     form_saver_->Save(generated, {} /* matches */,
                       base::string16() /* old_password */);
@@ -47,13 +47,13 @@ void PasswordGenerationState::PasswordNoLongerGenerated() {
 
 void PasswordGenerationState::CommitGeneratedPassword(
     PasswordForm generated,
-    const std::map<base::string16, const PasswordForm*>& best_matches,
-    const std::vector<PasswordForm>* credentials_to_update) {
+    const std::vector<const autofill::PasswordForm*>& matches,
+    const base::string16& old_password) {
   DCHECK(presaved_);
   generated.preferred = true;
   generated.date_created = clock_->Now();
-  form_saver_->Update(generated, best_matches, credentials_to_update,
-                      &presaved_.value() /* old_primary_key */);
+  form_saver_->UpdateReplace(generated, matches, old_password,
+                             presaved_.value() /* old_primary_key */);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_generation_state.h

@@ -38,17 +38,12 @@ class PasswordGenerationState {
   // Signals that the user cancels password generation.
   void PasswordNoLongerGenerated();
 
-  // Finish the generation flow by saving the final credential |generated| and
-  // leaving the generation state.
-  // |best_matches| constains possible passwords for the current site. They will
-  // be update according to the new preferred state.
-  // |credentials_to_update| are credentials for probably related domain that
-  // should be also updated.
+  // Finish the generation flow by saving the final credential |generated|.
+  // |matches| and |old_password| have the same meaning as in FormSaver.
   void CommitGeneratedPassword(
       autofill::PasswordForm generated,
-      const std::map<base::string16, const autofill::PasswordForm*>&
-          best_matches,
-      const std::vector<autofill::PasswordForm>* credentials_to_update);
+      const std::vector<const autofill::PasswordForm*>& matches,
+      const base::string16& old_password);
 
 #if defined(UNIT_TEST)
   void set_clock(std::unique_ptr<base::Clock> clock) {

components/password_manager/core/browser/password_generation_state_unittest.cc

@@ -44,6 +44,7 @@ PasswordForm CreateSavedPSL() {
   form.action = GURL("https://login.example.org");
   form.username_value = ASCIIToUTF16("old_username2");
   form.password_value = ASCIIToUTF16("passw0rd");
+  form.is_public_suffix_match = true;
   return form;
 }
 
@@ -157,7 +158,7 @@ TEST_F(PasswordGenerationStateTest, PresaveGeneratedPassword_ReplaceTwice) {
 // credential (as new) results in replacing the presaved password with the
 // pending one.
 TEST_F(PasswordGenerationStateTest, PresaveGeneratedPassword_ThenSaveAsNew) {
-  PasswordForm generated = CreateGenerated();
+  const PasswordForm generated = CreateGenerated();
 
   EXPECT_CALL(store(), AddLogin(_));
   state().PresaveGeneratedPassword(generated);
@@ -170,8 +171,8 @@ TEST_F(PasswordGenerationStateTest, PresaveGeneratedPassword_ThenSaveAsNew) {
   generated_with_date.date_created = base::Time::FromTimeT(kTime);
   EXPECT_CALL(store(), UpdateLoginWithPrimaryKey(generated_with_date,
                                                  FormHasUniqueKey(generated)));
-  state().CommitGeneratedPassword(pending, {} /* best_matches */,
-                                  nullptr /* credentials_to_update */);
+  state().CommitGeneratedPassword(pending, {} /* matches */,
+                                  base::string16() /* old_password */);
   EXPECT_TRUE(state().HasGeneratedPassword());
 }
 
@@ -184,26 +185,50 @@ TEST_F(PasswordGenerationStateTest, PresaveGeneratedPassword_ThenUpdate) {
   EXPECT_CALL(store(), AddLogin(_));
   state().PresaveGeneratedPassword(generated);
 
-  PasswordForm pending = generated;
-  pending.username_value = ASCIIToUTF16("edited_username");
+  PasswordForm related_password = CreateSaved();
+  related_password.username_value = ASCIIToUTF16("username");
+  related_password.username_element = ASCIIToUTF16("username_field");
+  related_password.password_value = ASCIIToUTF16("old password");
 
-  PasswordForm old_saved = CreateSaved();
-  old_saved.preferred = false;
-  PasswordForm old_psl_saved = CreateSavedPSL();
-  old_psl_saved.password_value = pending.password_value;
-  std::vector<autofill::PasswordForm> credentials_to_update = {old_psl_saved};
-  PasswordForm generated_with_date = pending;
+  PasswordForm related_psl_password = CreateSavedPSL();
+  related_psl_password.username_value = ASCIIToUTF16("username");
+  related_psl_password.password_value = ASCIIToUTF16("old password");
+
+  PasswordForm unrelated_password = CreateSaved();
+  unrelated_password.preferred = true;
+  unrelated_password.username_value = ASCIIToUTF16("another username");
+  unrelated_password.password_value = ASCIIToUTF16("some password");
+
+  PasswordForm unrelated_psl_password = CreateSavedPSL();
+  unrelated_psl_password.preferred = true;
+  unrelated_psl_password.username_value = ASCIIToUTF16("another username");
+  unrelated_psl_password.password_value = ASCIIToUTF16("some password");
+
+  generated.username_value = ASCIIToUTF16("username");
+  PasswordForm generated_with_date = generated;
   generated_with_date.date_created = base::Time::FromTimeT(kTime);
 
-  EXPECT_CALL(store(), UpdateLoginWithPrimaryKey(generated_with_date,
-                                                 FormHasUniqueKey(generated)));
-  EXPECT_CALL(store(), UpdateLogin(old_saved));
-  EXPECT_CALL(store(), UpdateLogin(old_psl_saved));
+  EXPECT_CALL(store(),
+              UpdateLoginWithPrimaryKey(generated_with_date,
+                                        FormHasUniqueKey(CreateGenerated())));
+
+  PasswordForm related_password_expected = related_password;
+  related_password_expected.password_value = generated.password_value;
+  EXPECT_CALL(store(), UpdateLogin(related_password_expected));
+
+  PasswordForm related_psl_password_expected = related_psl_password;
+  related_psl_password_expected.password_value = generated.password_value;
+  EXPECT_CALL(store(), UpdateLogin(related_psl_password_expected));
+
+  PasswordForm unrelated_password_expected = unrelated_password;
+  unrelated_password_expected.preferred = false;
+  EXPECT_CALL(store(), UpdateLogin(unrelated_password_expected));
 
-  old_saved.preferred = true;
   state().CommitGeneratedPassword(
-      pending, {{old_saved.username_value, &old_saved}} /* best_matches */,
-      &credentials_to_update);
+      generated,
+      {&related_password, &related_psl_password, &unrelated_password,
+       &unrelated_psl_password} /* matches */,
+      ASCIIToUTF16("old password"));
   EXPECT_TRUE(state().HasGeneratedPassword());
 }
 

components/password_manager/core/browser/stub_form_saver.cc

@@ -6,6 +6,11 @@
 
 namespace password_manager {
 
+autofill::PasswordForm StubFormSaver::PermanentlyBlacklist(
+    PasswordStore::FormDigest digest) {
+  return autofill::PasswordForm();
+}
+
 std::unique_ptr<FormSaver> StubFormSaver::Clone() {
   return nullptr;
 }

components/password_manager/core/browser/stub_form_saver.h

@@ -18,15 +18,18 @@ class StubFormSaver : public FormSaver {
   ~StubFormSaver() override = default;
 
   // FormSaver:
-  void PermanentlyBlacklist(autofill::PasswordForm* observed) override {}
-  void Save(const autofill::PasswordForm& pending,
+  autofill::PasswordForm PermanentlyBlacklist(
+      PasswordStore::FormDigest digest) override;
+  void Save(autofill::PasswordForm pending,
             const std::vector<const autofill::PasswordForm*>& matches,
             const base::string16& old_password) override {}
-  void Update(const autofill::PasswordForm& pending,
-              const std::map<base::string16, const autofill::PasswordForm*>&
-                  best_matches,
-              const std::vector<autofill::PasswordForm>* credentials_to_update,
-              const autofill::PasswordForm* old_primary_key) override {}
+  void Update(autofill::PasswordForm pending,
+              const std::vector<const autofill::PasswordForm*>& matches,
+              const base::string16& old_password) override {}
+  void UpdateReplace(autofill::PasswordForm pending,
+                     const std::vector<const autofill::PasswordForm*>& matches,
+                     const base::string16& old_password,
+                     const autofill::PasswordForm& old_unique_key) override {}
   void Remove(const autofill::PasswordForm& form) override {}
   std::unique_ptr<FormSaver> Clone() override;