PasswordFormManager clean-up.

There are 2 problems with function PasswordFormManager::UpdatePendingAndGetOldKey
1.It has pretty complicated and confusing logic in updating password_element,
username_element and submit_element. Now these variables have no meaning for
filling or saving. So it doesn't make sense to update password store records
because of them (moreover username and password elements are part of the key).
So this code may be removed.

2.This function is doing 2 independent actions, update *elements and finds which
other elements needed to be updated.

Now the time to throw away garbage, since this code will be copied to
NewPasswordFormManager. This CL removes item 1 and because of 2, the function is
renamed to FindOtherCredentialsToUpdate.

Bug: 831123
Change-Id: Ided9f7fe3156fc5c7384cc4278d7d61b416a77a6
Reviewed-on: https://chromium-review.googlesource.com/1169164
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582168}

components/password_manager/core/browser/password_form_manager.cc

@@ -316,12 +316,10 @@ void PasswordFormManager::Save() {
     form_saver_->Save(pending_credentials_, best_matches_);
   } else {
     ProcessUpdate();
-    std::vector<PasswordForm> credentials_to_update;
-    base::Optional<PasswordForm> old_primary_key =
-        UpdatePendingAndGetOldKey(&credentials_to_update);
+    std::vector<PasswordForm> credentials_to_update =
+        FindOtherCredentialsToUpdate();
     form_saver_->Update(pending_credentials_, best_matches_,
-                        &credentials_to_update,
-                        old_primary_key ? &old_primary_key.value() : nullptr);
+                        &credentials_to_update, nullptr);
   }
 
   // This is not in ProcessUpdate() to catch PSL matched credentials.
@@ -354,12 +352,10 @@ void PasswordFormManager::Update(
   pending_credentials_.preferred = true;
   is_new_login_ = false;
   ProcessUpdate();
-  std::vector<PasswordForm> more_credentials_to_update;
-  base::Optional<PasswordForm> old_primary_key =
-      UpdatePendingAndGetOldKey(&more_credentials_to_update);
+  std::vector<PasswordForm> more_credentials_to_update =
+      FindOtherCredentialsToUpdate();
   form_saver_->Update(pending_credentials_, best_matches_,
-                      &more_credentials_to_update,
-                      old_primary_key ? &old_primary_key.value() : nullptr);
+                      &more_credentials_to_update, nullptr);
 
   password_manager_->UpdateFormManagers();
 }
@@ -1018,56 +1014,26 @@ void PasswordFormManager::SetUserAction(UserAction user_action) {
   metrics_recorder_->SetUserAction(user_action);
 }
 
-base::Optional<PasswordForm> PasswordFormManager::UpdatePendingAndGetOldKey(
-    std::vector<PasswordForm>* credentials_to_update) {
-  base::Optional<PasswordForm> old_primary_key;
-  bool update_related_credentials = false;
-
-  if (pending_credentials_.federation_origin.unique() &&
-      !IsValidAndroidFacetURI(pending_credentials_.signon_realm) &&
-      (pending_credentials_.password_element.empty() ||
-       pending_credentials_.username_element.empty() ||
-       pending_credentials_.submit_element.empty())) {
-    // Given that |password_element| and |username_element| are part of Sync and
-    // PasswordStore primary key, the old primary key must be used in order to
-    // match and update the existing entry.
-    old_primary_key = pending_credentials_;
-    // TODO(crbug.com/833171) It is possible for best_matches to not contain the
-    // username being updated. Add comments and a test, when we realise why.
-    auto best_match = best_matches_.find(pending_credentials_.username_value);
-    if (best_match != best_matches_.end()) {
-      old_primary_key->username_element = best_match->second->username_element;
-      old_primary_key->password_element = best_match->second->password_element;
-    }
-    pending_credentials_.password_element = observed_form_.password_element;
-    pending_credentials_.username_element = observed_form_.username_element;
-    pending_credentials_.submit_element = observed_form_.submit_element;
-    update_related_credentials = true;
-  } else {
-    update_related_credentials =
-        pending_credentials_.federation_origin.unique();
-  }
-
-  // If this was a password update, then update all non-best matches entries
-  // with the same username and the same old password.
-  if (update_related_credentials) {
-    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;
-      }
+std::vector<PasswordForm> PasswordFormManager::FindOtherCredentialsToUpdate() {
+  std::vector<autofill::PasswordForm> credentials_to_update;
+  if (!pending_credentials_.federation_origin.unique())
+    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 old_primary_key;
+  return credentials_to_update;
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_form_manager.h

@@ -320,15 +320,11 @@ class PasswordFormManager : public PasswordFormManagerForUI,
   // Sets |user_action_| and records some metrics.
   void SetUserAction(UserAction user_action);
 
-  // Edits some fields in |pending_credentials_| before it can be used to
-  // update the password store. It also 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 adds copies of all such modified credentials to
-  // |credentials_to_update|. If needed, this also returns a PasswordForm to be
-  // used as the old primary key during the store update.
-  base::Optional<autofill::PasswordForm> UpdatePendingAndGetOldKey(
-      std::vector<autofill::PasswordForm>* credentials_to_update);
+  // 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.
+  std::vector<autofill::PasswordForm> FindOtherCredentialsToUpdate();
 
   void SetPasswordOverridden(bool password_overridden) {
     password_overridden_ = password_overridden;

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -1603,61 +1603,6 @@ TEST_F(PasswordFormManagerTest, TestAllPossiblePasswords) {
               UnorderedElementsAre(pair1, pair2, pair3));
 }
 
-// Test that if metadata stored with a form in PasswordStore are incomplete,
-// they are updated upon the next encounter.
-TEST_F(PasswordFormManagerTest, TestUpdateIncompleteCredentials) {
-  PasswordForm encountered_form;
-  encountered_form.origin = GURL("http://accounts.google.com/LoginAuth");
-  encountered_form.signon_realm = "http://accounts.google.com/";
-  encountered_form.action = GURL("http://accounts.google.com/Login");
-  encountered_form.username_element = ASCIIToUTF16("Email");
-  encountered_form.password_element = ASCIIToUTF16("Passwd");
-  encountered_form.submit_element = ASCIIToUTF16("signIn");
-
-  EXPECT_CALL(*(client()->mock_driver()), AllowPasswordGenerationForForm(_));
-
-  FakeFormFetcher fetcher;
-  fetcher.Fetch();
-  PasswordFormManager form_manager(password_manager(), client(),
-                                   client()->driver(), encountered_form,
-                                   std::make_unique<MockFormSaver>(), &fetcher);
-  form_manager.Init(nullptr);
-
-  PasswordForm incomplete_form;
-  incomplete_form.origin = GURL("http://accounts.google.com/LoginAuth");
-  incomplete_form.signon_realm = "http://accounts.google.com/";
-  incomplete_form.password_value = ASCIIToUTF16("my_password");
-  incomplete_form.username_value = ASCIIToUTF16("my_username");
-  incomplete_form.preferred = true;
-  incomplete_form.scheme = PasswordForm::SCHEME_HTML;
-
-  // We expect to see this form eventually sent to the Password store. It
-  // has password/username values from the store and 'username_element',
-  // 'password_element', 'submit_element' and 'action' fields copied from
-  // the encountered form.
-  PasswordForm complete_form(incomplete_form);
-  complete_form.action = encountered_form.action;
-  complete_form.password_element = encountered_form.password_element;
-  complete_form.username_element = encountered_form.username_element;
-  complete_form.submit_element = encountered_form.submit_element;
-
-  PasswordForm obsolete_form(incomplete_form);
-  obsolete_form.action = encountered_form.action;
-
-  // Feed the incomplete credentials to the manager.
-  fetcher.SetNonFederated({&incomplete_form}, 0u);
-
-  form_manager.ProvisionallySave(complete_form);
-  // By now that form has been used once.
-  complete_form.times_used = 1;
-  obsolete_form.times_used = 1;
-
-  // Check that PasswordStore receives an update request with the complete form.
-  EXPECT_CALL(MockFormSaver::Get(&form_manager),
-              Update(complete_form, _, _, Pointee(obsolete_form)));
-  form_manager.Save();
-}
-
 // Test that public-suffix-matched credentials score lower than same-origin
 // ones.
 TEST_F(PasswordFormManagerTest, TestScoringPublicSuffixMatch) {