PasswordSaveManagerImpl: Pull CreatePendingCredentials into a static helper

This is a first step towards making this logic reusable and
customizable for the MultiStorePasswordSaveManager.

Bug: 1012203
Change-Id: I5875c083e8b7439677ac315c0be30de61ab08c69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2116080
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753276}

components/password_manager/core/browser/password_save_manager_impl.cc

@@ -37,6 +37,27 @@ ValueElementPair PasswordToSave(const PasswordForm& form) {
   return {form.new_password_value, form.new_password_element};
 }
 
+PasswordForm PendingCredentialsForNewCredentials(
+    const PasswordForm& parsed_submitted_form,
+    const FormData& observed_form,
+    const base::string16& password_element,
+    bool is_http_auth,
+    bool is_credential_api_save) {
+  if (is_http_auth || is_credential_api_save)
+    return parsed_submitted_form;
+
+  PasswordForm pending_credentials = parsed_submitted_form;
+  pending_credentials.form_data = observed_form;
+  // The password value will be filled in later, remove any garbage for now.
+  pending_credentials.password_value.clear();
+  // The password element should be determined earlier in |PasswordToSave|.
+  pending_credentials.password_element = password_element;
+  // The new password's value and element name should be empty.
+  pending_credentials.new_password_value.clear();
+  pending_credentials.new_password_element.clear();
+  return pending_credentials;
+}
+
 // Helper to get the platform specific identifier by which autofill and password
 // manager refer to a field. See http://crbug.com/896594
 base::string16 GetPlatformSpecificIdentifier(const FormFieldData& field) {
@@ -130,96 +151,135 @@ void PasswordSaveManagerImpl::Init(
   votes_uploader_ = votes_uploader;
 }
 
-void PasswordSaveManagerImpl::CreatePendingCredentials(
+// static
+PendingCredentialsState PasswordSaveManagerImpl::FillPendingCredentials(
     const PasswordForm& parsed_submitted_form,
     const FormData& observed_form,
     const FormData& submitted_form,
+    const base::Optional<base::string16>& generated_password,
     bool is_http_auth,
-    bool is_credential_api_save) {
-  DCHECK(votes_uploader_);
+    bool is_credential_api_save,
+    const PasswordForm* similar_saved_form,
+    PasswordForm* pending_credentials) {
+  DCHECK(pending_credentials);
 
-  // This function might be called multiple times so set variables that are
-  // changed in this function to initial states.
-  pending_credentials_state_ = PendingCredentialsState::NONE;
-  votes_uploader_->set_password_overridden(false);
+  PendingCredentialsState pending_credentials_state =
+      PendingCredentialsState::NONE;
 
   ValueElementPair password_to_save(PasswordToSave(parsed_submitted_form));
-  // Look for the actually submitted credentials in the list of previously saved
-  // credentials that were available to autofilling.
-  const PasswordForm* saved_form = password_manager_util::GetMatchForUpdating(
-      parsed_submitted_form, form_fetcher_->GetBestMatches());
-  if (saved_form) {
+  // Check if there are previously saved credentials (that were available to
+  // autofilling) matching the actually submitted credentials.
+  if (similar_saved_form) {
     // A similar credential exists in the store already.
-    pending_credentials_ = *saved_form;
-    pending_credentials_state_ = PendingCredentialsState::EQUAL_TO_SAVED_MATCH;
-    if (pending_credentials_.password_value != password_to_save.first) {
-      pending_credentials_state_ = PendingCredentialsState::UPDATE;
-      votes_uploader_->set_password_overridden(true);
-    } else if (pending_credentials_.is_public_suffix_match) {
+    *pending_credentials = *similar_saved_form;
+    pending_credentials_state = PendingCredentialsState::EQUAL_TO_SAVED_MATCH;
+    if (pending_credentials->password_value != password_to_save.first) {
+      pending_credentials_state = PendingCredentialsState::UPDATE;
+    } else if (pending_credentials->is_public_suffix_match) {
       // If the autofilled credentials were a PSL match, store a copy with the
       // current origin and signon realm. This ensures that on the next visit, a
       // precise match is found.
-      pending_credentials_state_ = PendingCredentialsState::AUTOMATIC_SAVE;
+      pending_credentials_state = PendingCredentialsState::AUTOMATIC_SAVE;
       // Update credential to reflect that it has been used for submission.
       // If this isn't updated, then password generation uploads are off for
       // sites where PSL matching is required to fill the login form, as two
       // PASSWORD votes are uploaded per saved password instead of one.
-      password_manager_util::UpdateMetadataForUsage(&pending_credentials_);
+      password_manager_util::UpdateMetadataForUsage(pending_credentials);
 
-      // Update |pending_credentials_| in order to be able correctly save it.
-      pending_credentials_.origin = parsed_submitted_form.origin;
-      pending_credentials_.signon_realm = parsed_submitted_form.signon_realm;
-      pending_credentials_.action = parsed_submitted_form.action;
+      // Update |pending_credentials| in order to be able correctly save it.
+      pending_credentials->origin = parsed_submitted_form.origin;
+      pending_credentials->signon_realm = parsed_submitted_form.signon_realm;
+      pending_credentials->action = parsed_submitted_form.action;
     }
   } else {
-    pending_credentials_state_ = PendingCredentialsState::NEW_LOGIN;
+    pending_credentials_state = PendingCredentialsState::NEW_LOGIN;
     // No stored credentials can be matched to the submitted form. Offer to
     // save new credentials.
-    CreatePendingCredentialsForNewCredentials(
+    *pending_credentials = PendingCredentialsForNewCredentials(
         parsed_submitted_form, observed_form, password_to_save.second,
         is_http_auth, is_credential_api_save);
-    // Generate username correction votes.
-    bool username_correction_found =
-        votes_uploader_->FindCorrectedUsernameElement(
-            form_fetcher_->GetAllRelevantMatches(),
-            parsed_submitted_form.username_value,
-            parsed_submitted_form.password_value);
-    UMA_HISTOGRAM_BOOLEAN("PasswordManager.UsernameCorrectionFound",
-                          username_correction_found);
-    if (username_correction_found) {
-      metrics_recorder_->RecordDetailedUserAction(
-          password_manager::PasswordFormMetricsRecorder::DetailedUserAction::
-              kCorrectedUsernameInForm);
   }
-  }
-  pending_credentials_.password_value =
-      HasGeneratedPassword() ? generation_manager_->generated_password()
-                             : password_to_save.first;
-  pending_credentials_.date_last_used = base::Time::Now();
-  pending_credentials_.form_has_autofilled_value =
+  pending_credentials->password_value =
+      generated_password.value_or(password_to_save.first);
+  pending_credentials->date_last_used = base::Time::Now();
+  pending_credentials->form_has_autofilled_value =
       parsed_submitted_form.form_has_autofilled_value;
-  pending_credentials_.all_possible_passwords =
+  pending_credentials->all_possible_passwords =
       parsed_submitted_form.all_possible_passwords;
-  CopyFieldPropertiesMasks(submitted_form, &pending_credentials_.form_data);
+  CopyFieldPropertiesMasks(submitted_form, &pending_credentials->form_data);
 
   // If we're dealing with an API-driven provisionally saved form, then take
   // the server provided values. We don't do this for non-API forms, as
   // those will never have those members set.
   if (parsed_submitted_form.type == PasswordForm::Type::kApi) {
-    pending_credentials_.skip_zero_click =
+    pending_credentials->skip_zero_click =
         parsed_submitted_form.skip_zero_click;
-    pending_credentials_.display_name = parsed_submitted_form.display_name;
-    pending_credentials_.federation_origin =
+    pending_credentials->display_name = parsed_submitted_form.display_name;
+    pending_credentials->federation_origin =
         parsed_submitted_form.federation_origin;
-    pending_credentials_.icon_url = parsed_submitted_form.icon_url;
+    pending_credentials->icon_url = parsed_submitted_form.icon_url;
     // It's important to override |signon_realm| for federated credentials
     // because it has format "federation://" + origin_host + "/" +
     // federation_host
-    pending_credentials_.signon_realm = parsed_submitted_form.signon_realm;
+    pending_credentials->signon_realm = parsed_submitted_form.signon_realm;
   }
 
+  if (generated_password.has_value())
+    pending_credentials->type = PasswordForm::Type::kGenerated;
+
+  return pending_credentials_state;
+}
+
+void PasswordSaveManagerImpl::CreatePendingCredentials(
+    const PasswordForm& parsed_submitted_form,
+    const FormData& observed_form,
+    const FormData& submitted_form,
+    bool is_http_auth,
+    bool is_credential_api_save) {
+  DCHECK(votes_uploader_);
+
+  // This function might be called multiple times so set variables that are
+  // changed in this function to initial states.
+  pending_credentials_ = PasswordForm();
+  votes_uploader_->set_password_overridden(false);
+
+  base::Optional<base::string16> generated_password;
   if (HasGeneratedPassword())
-    pending_credentials_.type = PasswordForm::Type::kGenerated;
+    generated_password = generation_manager_->generated_password();
+
+  pending_credentials_state_ = FillPendingCredentials(
+      parsed_submitted_form, observed_form, submitted_form, generated_password,
+      is_http_auth, is_credential_api_save,
+      password_manager_util::GetMatchForUpdating(
+          parsed_submitted_form, form_fetcher_->GetBestMatches()),
+      &pending_credentials_);
+
+  switch (pending_credentials_state_) {
+    case PendingCredentialsState::NEW_LOGIN: {
+      // Generate username correction votes.
+      bool username_correction_found =
+          votes_uploader_->FindCorrectedUsernameElement(
+              form_fetcher_->GetAllRelevantMatches(),
+              parsed_submitted_form.username_value,
+              parsed_submitted_form.password_value);
+      UMA_HISTOGRAM_BOOLEAN("PasswordManager.UsernameCorrectionFound",
+                            username_correction_found);
+      if (username_correction_found) {
+        metrics_recorder_->RecordDetailedUserAction(
+            password_manager::PasswordFormMetricsRecorder::DetailedUserAction::
+                kCorrectedUsernameInForm);
+      }
+      break;
+    }
+    case PendingCredentialsState::UPDATE:
+      votes_uploader_->set_password_overridden(true);
+      break;
+    case PendingCredentialsState::NONE:
+    case PendingCredentialsState::AUTOMATIC_SAVE:
+    case PendingCredentialsState::EQUAL_TO_SAVED_MATCH:
+      // Nothing to be done in these cases.
+      break;
+  }
 }
 
 void PasswordSaveManagerImpl::ResetPendingCrednetials() {
@@ -361,28 +421,6 @@ std::unique_ptr<PasswordSaveManager> PasswordSaveManagerImpl::Clone() {
   return result;
 }
 
-void PasswordSaveManagerImpl::CreatePendingCredentialsForNewCredentials(
-    const PasswordForm& parsed_submitted_form,
-    const FormData& observed_form,
-    const base::string16& password_element,
-    bool is_http_auth,
-    bool is_credential_api_save) {
-  if (is_http_auth || is_credential_api_save) {
-    pending_credentials_ = parsed_submitted_form;
-    return;
-  }
-
-  pending_credentials_ = parsed_submitted_form;
-  pending_credentials_.form_data = observed_form;
-  // The password value will be filled in later, remove any garbage for now.
-  pending_credentials_.password_value.clear();
-  // The password element should be determined earlier in |PasswordToSave|.
-  pending_credentials_.password_element = password_element;
-  // The new password's value and element name should be empty.
-  pending_credentials_.new_password_value.clear();
-  pending_credentials_.new_password_element.clear();
-}
-
 void PasswordSaveManagerImpl::SavePendingToStore(
     const PasswordForm& parsed_submitted_form,
     bool update) {

components/password_manager/core/browser/password_save_manager_impl.h

@@ -38,8 +38,8 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
             scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder,
             VotesUploader* votes_uploader) override;
 
-  // Create pending credentials from |parsed_submitted_form| and
-  // |parsed_observed_form| and |submitted_form|.
+  // Create pending credentials from |parsed_submitted_form|, |observed_form|
+  // and |submitted_form|.
   void CreatePendingCredentials(
       const autofill::PasswordForm& parsed_submitted_form,
       const autofill::FormData& observed_form,
@@ -84,6 +84,16 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
 #endif
 
  protected:
+  static PendingCredentialsState FillPendingCredentials(
+      const autofill::PasswordForm& parsed_submitted_form,
+      const autofill::FormData& observed_form,
+      const autofill::FormData& submitted_form,
+      const base::Optional<base::string16>& generated_password,
+      bool is_http_auth,
+      bool is_credential_api_save,
+      const autofill::PasswordForm* similar_saved_form,
+      autofill::PasswordForm* pending_credentials);
+
   // Returns the form_saver to be used for generated passwords. Subclasses will
   // override this method to provide different logic for get the form saver.
   virtual FormSaver* GetFormSaverForGeneration();
@@ -119,15 +129,6 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   const FormFetcher* form_fetcher_;
 
  private:
-  // Create pending credentials from provisionally saved form when this form
-  // represents credentials that were not previously saved.
-  void CreatePendingCredentialsForNewCredentials(
-      const autofill::PasswordForm& parsed_submitted_form,
-      const autofill::FormData& observed_form,
-      const base::string16& password_element,
-      bool is_http_auth,
-      bool is_credential_api_save);
-
   // Save/update |pending_credentials_| to the password store.
   void SavePendingToStore(const autofill::PasswordForm& parsed_submitted_form,
                           bool update);