[Passwords] Make observed_form optional

This change makes the observed_form parameter in the APIs of
VotesUploader and PasswordSaveManager optional, since it doesn't exist
when saving or updating HTTP and proxy auth credentials.

TBR=arbesser

Bug: 943045
Change-Id: I9794c65f8950a2cb20bb4d9882cc1a2730508159
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2341637
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798596}

components/autofill_assistant/browser/website_login_manager_impl.cc

@@ -196,7 +196,7 @@ class WebsiteLoginManagerImpl::UpdatePasswordRequest
   }
 
   void CommitGeneratedPassword() {
-    password_save_manager_->Save(form_data_ /* observed_form */,
+    password_save_manager_->Save(&form_data_ /* observed_form */,
                                  password_form_);
   }
 
@@ -206,7 +206,7 @@ class WebsiteLoginManagerImpl::UpdatePasswordRequest
                                  metrics_recorder_, &votes_uploader_);
     password_save_manager_->PresaveGeneratedPassword(password_form_);
     password_save_manager_->CreatePendingCredentials(
-        password_form_, form_data_ /* observed_form */,
+        password_form_, &form_data_ /* observed_form */,
         form_data_ /* submitted_form */, false /* is_http_auth */,
         false /* is_credential_api_save */);
 

components/password_manager/core/browser/password_form_manager.cc

@@ -312,7 +312,7 @@ void PasswordFormManager::Save() {
     newly_blacklisted_ = false;
   }
 
-  password_save_manager_->Save(observed_form_, *parsed_submitted_form_);
+  password_save_manager_->Save(&observed_form_, *parsed_submitted_form_);
 
   client_->UpdateFormManagers();
 }
@@ -323,7 +323,7 @@ void PasswordFormManager::Update(const PasswordForm& credentials_to_update) {
   metrics_recorder_->SetSubmissionIndicatorEvent(
       parsed_submitted_form_->submission_event);
 
-  password_save_manager_->Update(credentials_to_update, observed_form_,
+  password_save_manager_->Update(credentials_to_update, &observed_form_,
                                  *parsed_submitted_form_);
 
   client_->UpdateFormManagers();
@@ -685,7 +685,7 @@ void PasswordFormManager::CreatePendingCredentials() {
     return;
 
   password_save_manager_->CreatePendingCredentials(
-      *parsed_submitted_form_, observed_form_, submitted_form_, IsHttpAuth(),
+      *parsed_submitted_form_, &observed_form_, submitted_form_, IsHttpAuth(),
       IsCredentialAPISave());
 }
 

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -105,8 +105,8 @@ MATCHER_P(FormHasPassword, password_value, "") {
   return arg.new_password_value == password_value;
 }
 
-MATCHER_P(FormDataEqualTo, form_data, "") {
-  return autofill::FormDataEqualForTesting(arg, form_data);
+MATCHER_P(FormDataPointeeEqualTo, form_data, "") {
+  return autofill::FormDataEqualForTesting(*arg, form_data);
 }
 
 class MockPasswordManagerDriver : public StubPasswordManagerDriver {
@@ -2380,16 +2380,16 @@ class MockPasswordSaveManager : public PasswordSaveManager {
   MOCK_CONST_METHOD0(GetFormSaver, FormSaver*());
   MOCK_METHOD5(CreatePendingCredentials,
                void(const autofill::PasswordForm&,
-                    const autofill::FormData&,
+                    const autofill::FormData*,
                     const autofill::FormData&,
                     bool,
                     bool));
   MOCK_METHOD0(ResetPendingCredentials, void());
   MOCK_METHOD2(Save,
-               void(const autofill::FormData&, const autofill::PasswordForm&));
+               void(const autofill::FormData*, const autofill::PasswordForm&));
   MOCK_METHOD3(Update,
                void(const autofill::PasswordForm&,
-                    const autofill::FormData&,
+                    const autofill::FormData*,
                     const autofill::PasswordForm&));
   MOCK_METHOD1(PermanentlyBlacklist, void(const PasswordStore::FormDigest&));
   MOCK_METHOD1(Unblacklist, void(const PasswordStore::FormDigest&));
@@ -2475,7 +2475,7 @@ TEST_F(PasswordFormManagerTestWithMockedSaver, SaveCredentials) {
       form_manager_->ProvisionallySave(submitted_form, &driver_, nullptr));
   PasswordForm updated_form;
   EXPECT_CALL(*mock_password_save_manager(),
-              Save(FormDataEqualTo(observed_form_), _))
+              Save(FormDataPointeeEqualTo(observed_form_), _))
       .WillOnce(SaveArg<1>(&updated_form));
   EXPECT_CALL(client_, UpdateFormManagers());
   form_manager_->Save();
@@ -2692,7 +2692,7 @@ TEST_F(PasswordFormManagerTestWithMockedSaver,
   EXPECT_TRUE(
       form_manager_->ProvisionallySave(submitted_form_, &driver_, nullptr));
   EXPECT_CALL(*mock_password_save_manager(),
-              Save(FormDataEqualTo(submitted_form_), _))
+              Save(FormDataPointeeEqualTo(submitted_form_), _))
       .WillOnce(SaveArg<1>(&updated_form));
   form_manager_->Save();
   EXPECT_EQ(submitted_form_.fields[kUsernameFieldIndex].value,

components/password_manager/core/browser/password_save_manager.h

@@ -50,22 +50,28 @@ class PasswordSaveManager {
 
   virtual FormSaver* GetFormSaver() const = 0;
 
-  // Create pending credentials from |parsed_submitted_form| and
-  // |parsed_observed_form| and |submitted_form|.
+  // Create pending credentials from |parsed_submitted_form| and |observed_form|
+  // and |submitted_form|. In the case of HTTP or proxy auth no |observed_form|
+  // exists, so this parameter is optional.
   virtual void CreatePendingCredentials(
       const autofill::PasswordForm& parsed_submitted_form,
-      const autofill::FormData& observed_form,
+      const autofill::FormData* observed_form,
       const autofill::FormData& submitted_form,
       bool is_http_auth,
       bool is_credential_api_save) = 0;
 
   virtual void ResetPendingCredentials() = 0;
 
-  virtual void Save(const autofill::FormData& observed_form,
+  // Saves `parsed_submitted_form` to the store. An optional `observed_form` is
+  // passed along to be able to send votes. This is null for HTTP or proxy auth.
+  virtual void Save(const autofill::FormData* observed_form,
                     const autofill::PasswordForm& parsed_submitted_form) = 0;
 
+  // Replaces `credentials_to_update` with `parsed_submitted_form` in the store.
+  // An optional `observed_form` is passed along to be able to send votes. This
+  // is null for HTTP or proxy auth.
   virtual void Update(const autofill::PasswordForm& credentials_to_update,
-                      const autofill::FormData& observed_form,
+                      const autofill::FormData* observed_form,
                       const autofill::PasswordForm& parsed_submitted_form) = 0;
 
   virtual void PermanentlyBlacklist(

components/password_manager/core/browser/password_save_manager_impl.cc

@@ -41,7 +41,7 @@ ValueElementPair PasswordToSave(const PasswordForm& form) {
 
 PasswordForm PendingCredentialsForNewCredentials(
     const PasswordForm& parsed_submitted_form,
-    const FormData& observed_form,
+    const FormData* observed_form,
     const base::string16& password_element,
     bool is_http_auth,
     bool is_credential_api_save) {
@@ -49,7 +49,8 @@ PasswordForm PendingCredentialsForNewCredentials(
     return parsed_submitted_form;
 
   PasswordForm pending_credentials = parsed_submitted_form;
-  pending_credentials.form_data = observed_form;
+  if (observed_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|.
@@ -156,7 +157,7 @@ void PasswordSaveManagerImpl::Init(
 
 void PasswordSaveManagerImpl::CreatePendingCredentials(
     const PasswordForm& parsed_submitted_form,
-    const FormData& observed_form,
+    const FormData* observed_form,
     const FormData& submitted_form,
     bool is_http_auth,
     bool is_credential_api_save) {
@@ -214,7 +215,7 @@ void PasswordSaveManagerImpl::ResetPendingCredentials() {
   pending_credentials_state_ = PendingCredentialsState::NONE;
 }
 
-void PasswordSaveManagerImpl::Save(const FormData& observed_form,
+void PasswordSaveManagerImpl::Save(const FormData* observed_form,
                                    const PasswordForm& parsed_submitted_form) {
   if (IsPasswordUpdate() &&
       pending_credentials_.type == PasswordForm::Type::kGenerated &&
@@ -241,7 +242,7 @@ void PasswordSaveManagerImpl::Save(const FormData& observed_form,
 
 void PasswordSaveManagerImpl::Update(
     const PasswordForm& credentials_to_update,
-    const FormData& observed_form,
+    const FormData* observed_form,
     const PasswordForm& parsed_submitted_form) {
   base::string16 password_to_save = pending_credentials_.password_value;
   bool skip_zero_click = pending_credentials_.skip_zero_click;
@@ -375,7 +376,7 @@ PendingCredentialsState PasswordSaveManagerImpl::ComputePendingCredentialsState(
 PasswordForm PasswordSaveManagerImpl::BuildPendingCredentials(
     PendingCredentialsState pending_credentials_state,
     const PasswordForm& parsed_submitted_form,
-    const FormData& observed_form,
+    const FormData* observed_form,
     const FormData& submitted_form,
     const base::Optional<base::string16>& generated_password,
     bool is_http_auth,
@@ -458,7 +459,7 @@ PasswordSaveManagerImpl::FindSimilarSavedFormAndComputeState(
 }
 
 void PasswordSaveManagerImpl::SavePendingToStore(
-    const FormData& observed_form,
+    const FormData* observed_form,
     const PasswordForm& parsed_submitted_form) {
   UploadVotesAndMetrics(observed_form, parsed_submitted_form);
 
@@ -494,16 +495,19 @@ base::string16 PasswordSaveManagerImpl::GetOldPassword(
 }
 
 void PasswordSaveManagerImpl::UploadVotesAndMetrics(
-    const FormData& observed_form,
+    const FormData* observed_form,
     const PasswordForm& parsed_submitted_form) {
   if (IsNewLogin()) {
     metrics_util::LogNewlySavedPasswordIsGenerated(
         pending_credentials_.type == PasswordForm::Type::kGenerated,
         client_->GetPasswordFeatureManager()
             ->ComputePasswordAccountStorageUsageLevel());
-    votes_uploader_->SendVotesOnSave(observed_form, parsed_submitted_form,
-                                     form_fetcher_->GetBestMatches(),
-                                     &pending_credentials_);
+    // Don't send votes if there was no observed form.
+    if (observed_form) {
+      votes_uploader_->SendVotesOnSave(*observed_form, parsed_submitted_form,
+                                       form_fetcher_->GetBestMatches(),
+                                       &pending_credentials_);
+    }
     return;
   }
 
@@ -521,11 +525,12 @@ void PasswordSaveManagerImpl::UploadVotesAndMetrics(
       base::UserMetricsAction("PasswordManager_LoginFollowingAutofill"));
 
   // Check to see if this form is a candidate for password generation.
-  // Do not send votes on change password forms, since they were already sent
-  // in Update() method.
-  if (!parsed_submitted_form.IsPossibleChangePasswordForm()) {
+  // Do not send votes if there was no observed form. Furthermore, don't send
+  // votes on change password forms, since they were already sent in Update()
+  // method.
+  if (observed_form && !parsed_submitted_form.IsPossibleChangePasswordForm()) {
     votes_uploader_->SendVoteOnCredentialsReuse(
-        observed_form, parsed_submitted_form, &pending_credentials_);
+        *observed_form, parsed_submitted_form, &pending_credentials_);
   }
   if (IsPasswordUpdate()) {
     votes_uploader_->UploadPasswordVote(

components/password_manager/core/browser/password_save_manager_impl.h

@@ -43,18 +43,18 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   // and |submitted_form|.
   void CreatePendingCredentials(
       const autofill::PasswordForm& parsed_submitted_form,
-      const autofill::FormData& observed_form,
+      const autofill::FormData* observed_form,
       const autofill::FormData& submitted_form,
       bool is_http_auth,
       bool is_credential_api_save) override;
 
   void ResetPendingCredentials() override;
 
-  void Save(const autofill::FormData& observed_form,
+  void Save(const autofill::FormData* observed_form,
             const autofill::PasswordForm& parsed_submitted_form) override;
 
   void Update(const autofill::PasswordForm& credentials_to_update,
-              const autofill::FormData& observed_form,
+              const autofill::FormData* observed_form,
               const autofill::PasswordForm& parsed_submitted_form) override;
 
   void PermanentlyBlacklist(
@@ -95,7 +95,7 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   static autofill::PasswordForm BuildPendingCredentials(
       PendingCredentialsState pending_credentials_state,
       const autofill::PasswordForm& parsed_submitted_form,
-      const autofill::FormData& observed_form,
+      const autofill::FormData* observed_form,
       const autofill::FormData& submitted_form,
       const base::Optional<base::string16>& generated_password,
       bool is_http_auth,
@@ -149,13 +149,13 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
       const autofill::PasswordForm& parsed_submitted_form);
 
   // Save/update |pending_credentials_| to the password store.
-  void SavePendingToStore(const autofill::FormData& observed_form,
+  void SavePendingToStore(const autofill::FormData* observed_form,
                           const autofill::PasswordForm& parsed_submitted_form);
 
   // This sends needed signals to the autofill server, and also triggers some
   // UMA reporting.
   void UploadVotesAndMetrics(
-      const autofill::FormData& observed_form,
+      const autofill::FormData* observed_form,
       const autofill::PasswordForm& parsed_submitted_form);
 
   // Handles the user flows related to the generation.