Add metric recorder into constructor parameters of NewPasswordForManager.

Before this CL, during cloning the new metric recorder was created and
then it was overwritten with the old one. That leads to many extra
metric sending (each recorder sends metrics separately). This CL fixes
this to adding metric recorder to the constructor, so it's possible to
reuse existing metric recorder. It's very similar to PasswordFormManager
implementation, but then it's done with a separate Init() function, but
it's easier to add this in constructor in order to have the consistent
object after constructor call, since anyway the recorder must
be created.

Bug: 831123
Change-Id: Id46dee6aee4ae30686f1190884db05e25f26113b
Reviewed-on: https://chromium-review.googlesource.com/c/1288437
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600855}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -101,10 +101,12 @@ NewPasswordFormManager::NewPasswordFormManager(
     const base::WeakPtr<PasswordManagerDriver>& driver,
     const FormData& observed_form,
     FormFetcher* form_fetcher,
-    std::unique_ptr<FormSaver> form_saver)
+    std::unique_ptr<FormSaver> form_saver,
+    scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder)
     : client_(client),
       driver_(driver),
       observed_form_(observed_form),
+      metrics_recorder_(metrics_recorder),
       owned_form_fetcher_(
           form_fetcher ? nullptr
                        : std::make_unique<FormFetcherImpl>(
@@ -118,8 +120,10 @@ NewPasswordFormManager::NewPasswordFormManager(
       // |is_possible_change_password_form| in |votes_uploader_| constructor
       votes_uploader_(client, false /* is_possible_change_password_form */),
       weak_ptr_factory_(this) {
-  metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-      client_->IsMainFrameSecure(), client_->GetUkmSourceId());
+  if (!metrics_recorder_) {
+    metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(
+        client_->IsMainFrameSecure(), client_->GetUkmSourceId());
+  }
   metrics_recorder_->RecordFormSignature(CalculateFormSignature(observed_form));
 
   if (owned_form_fetcher_)
@@ -379,9 +383,7 @@ std::unique_ptr<NewPasswordFormManager> NewPasswordFormManager::Clone() {
   // blacklisting entry if needed.
   auto result = std::make_unique<NewPasswordFormManager>(
       client_, base::WeakPtr<PasswordManagerDriver>(), observed_form_,
-      fetcher.get(), form_saver_->Clone());
-
-  result->metrics_recorder_ = metrics_recorder_;
+      fetcher.get(), form_saver_->Clone(), metrics_recorder_);
 
   // The constructor only can take a weak pointer to the fetcher, so moving the
   // owning one needs to happen explicitly.

components/password_manager/core/browser/new_password_form_manager.h

@@ -44,12 +44,15 @@ class NewPasswordFormManager : public PasswordFormManagerInterface,
   // |this| creates an instance of it itself (meant for production code). Once
   // the fetcher is shared between PasswordFormManager instances, it will be
   // required that |form_fetcher| is not null. |form_saver| is used to
-  // save/update the form.
-  NewPasswordFormManager(PasswordManagerClient* client,
-                         const base::WeakPtr<PasswordManagerDriver>& driver,
-                         const autofill::FormData& observed_form,
-                         FormFetcher* form_fetcher,
-                         std::unique_ptr<FormSaver> form_saver);
+  // save/update the form. |metrics_recorder| records metrics for |*this|. If
+  // null a new instance will be created.
+  NewPasswordFormManager(
+      PasswordManagerClient* client,
+      const base::WeakPtr<PasswordManagerDriver>& driver,
+      const autofill::FormData& observed_form,
+      FormFetcher* form_fetcher,
+      std::unique_ptr<FormSaver> form_saver,
+      scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder);
 
   ~NewPasswordFormManager() override;
 

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -253,7 +253,7 @@ class NewPasswordFormManagerTest : public testing::Test {
     fetcher_->Fetch();
     form_manager_.reset(new NewPasswordFormManager(
         &client_, driver_.AsWeakPtr(), observed_form, fetcher_.get(),
-        std::make_unique<NiceMock<MockFormSaver>>()));
+        std::make_unique<NiceMock<MockFormSaver>>(), nullptr));
   }
 };
 

components/password_manager/core/browser/password_manager.cc

@@ -883,7 +883,7 @@ void PasswordManager::CreateFormManagers(
         client_,
         driver ? driver->AsWeakPtr() : base::WeakPtr<PasswordManagerDriver>(),
         new_form->form_data, nullptr,
-        std::make_unique<FormSaverImpl>(client_->GetPasswordStore())));
+        std::make_unique<FormSaverImpl>(client_->GetPasswordStore()), nullptr));
     form_managers_.back()->set_old_parsing_result(*new_form);
   }
 }