[Password Manager] Preparing the passwords for export also serialises them

I also changed the timing on the IN_PROGRESS update to occur as soon as
the destination is known. If we happen to still be serialising, that is an
implementation detail that should not be affect the UI.

Bug: 785237
Change-Id: Ie9466840b1e4bf11ddfaa4c69045326ab3b11fe3
Reviewed-on: https://chromium-review.googlesource.com/789330
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533264}

components/password_manager/core/browser/export/password_manager_exporter.cc

@@ -17,13 +17,16 @@
 
 namespace {
 
-std::vector<std::unique_ptr<autofill::PasswordForm>> CopyOf(
-    const std::vector<std::unique_ptr<autofill::PasswordForm>>& password_list) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> ret_val;
-  for (const auto& form : password_list) {
-    ret_val.push_back(std::make_unique<autofill::PasswordForm>(*form));
-  }
-  return ret_val;
+// A wrapper for |write_function|, which can be bound and keep a copy of its
+// data on the closure.
+bool Write(int (*write_function)(const base::FilePath& filename,
+                                 const char* data,
+                                 int size),
+           const base::FilePath& destination,
+           const std::string& serialised) {
+  int written =
+      write_function(destination, serialised.c_str(), serialised.size());
+  return written == static_cast<int>(serialised.size());
 }
 
 }  // namespace
@@ -45,16 +48,34 @@ PasswordManagerExporter::PasswordManagerExporter(
 PasswordManagerExporter::~PasswordManagerExporter() {}
 
 void PasswordManagerExporter::PreparePasswordsForExport() {
-  password_list_ = credential_provider_interface_->GetAllPasswords();
+  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
+      credential_provider_interface_->GetAllPasswords();
+  size_t password_list_size = password_list.size();
 
-  if (IsReadyForExport())
-    Export();
+  base::PostTaskAndReplyWithResult(
+      task_runner_.get(), FROM_HERE,
+      base::BindOnce(&password_manager::PasswordCSVWriter::SerializePasswords,
+                     base::Passed(std::move(password_list))),
+      base::BindOnce(&PasswordManagerExporter::SetSerialisedPasswordList,
+                     weak_factory_.GetWeakPtr(), password_list_size));
 }
 
 void PasswordManagerExporter::SetDestination(
     const base::FilePath& destination) {
   destination_ = destination;
 
+  if (IsReadyForExport())
+    Export();
+
+  OnProgress(ExportProgressStatus::IN_PROGRESS, std::string());
+}
+
+void PasswordManagerExporter::SetSerialisedPasswordList(
+    size_t count,
+    const std::string& serialised) {
+  serialised_password_list_ = serialised;
+  password_count_ = count;
+
   if (IsReadyForExport())
     Export();
 }
@@ -64,7 +85,8 @@ void PasswordManagerExporter::Cancel() {
   weak_factory_.InvalidateWeakPtrs();
 
   destination_.clear();
-  password_list_.clear();
+  serialised_password_list_.clear();
+  password_count_ = 0;
 
   OnProgress(ExportProgressStatus::FAILED_CANCELLED, std::string());
 }
@@ -82,33 +104,31 @@ void PasswordManagerExporter::SetWriteForTesting(
 }
 
 bool PasswordManagerExporter::IsReadyForExport() {
-  return !destination_.empty() && !password_list_.empty();
+  return !destination_.empty() && !serialised_password_list_.empty();
 }
 
 void PasswordManagerExporter::Export() {
-  UMA_HISTOGRAM_COUNTS("PasswordManager.ExportedPasswordsPerUserInCSV",
-                       password_list_.size());
-
-  OnProgress(ExportProgressStatus::IN_PROGRESS, std::string());
-
+  base::FilePath destination_copy_(destination_);
   base::PostTaskAndReplyWithResult(
       task_runner_.get(), FROM_HERE,
-      base::BindOnce(&password_manager::PasswordCSVWriter::SerializePasswords,
-                     base::Passed(CopyOf(password_list_))),
-      base::BindOnce(&PasswordManagerExporter::OnPasswordsSerialised,
-                     weak_factory_.GetWeakPtr(),
-                     base::Passed(std::move(destination_))));
+      base::BindOnce(::Write, write_function_, std::move(destination_copy_),
+                     std::move(serialised_password_list_)),
+      base::BindOnce(&PasswordManagerExporter::OnPasswordsExported,
+                     weak_factory_.GetWeakPtr(), std::move(destination_),
+                     password_count_));
 
-  password_list_.clear();
   destination_.clear();
+  serialised_password_list_.clear();
+  password_count_ = 0;
 }
 
-void PasswordManagerExporter::OnPasswordsSerialised(
+void PasswordManagerExporter::OnPasswordsExported(
     const base::FilePath& destination,
-    const std::string& serialised) {
-  int written =
-      write_function_(destination, serialised.c_str(), serialised.size());
-  if (written == static_cast<int>(serialised.size())) {
+    int count,
+    bool success) {
+  if (success) {
+    UMA_HISTOGRAM_COUNTS("PasswordManager.ExportedPasswordsPerUserInCSV",
+                         count);
     OnProgress(ExportProgressStatus::SUCCEEDED, std::string());
   } else {
     OnProgress(ExportProgressStatus::FAILED_WRITE_FAILED,

components/password_manager/core/browser/export/password_manager_exporter.h

@@ -15,10 +15,6 @@
 #include "base/sequenced_task_runner.h"
 #include "components/password_manager/core/browser/ui/export_progress_status.h"
 
-namespace autofill {
-struct PasswordForm;
-}
-
 namespace password_manager {
 
 class CredentialProviderInterface;
@@ -59,6 +55,11 @@ class PasswordManagerExporter {
                                                 int size));
 
  private:
+  // Caches the serialised password list. It proceeds to export, if all the
+  // parts are ready. |count| is the number of passwords which were serialised.
+  // |serialised| is the serialised list of passwords.
+  void SetSerialisedPasswordList(size_t count, const std::string& serialised);
+
   // Returns true if all the necessary data is available.
   bool IsReadyForExport();
 
@@ -66,9 +67,13 @@ class PasswordManagerExporter {
   // At the end, it clears cached fields.
   void Export();
 
-  // Callback after the passwords have been serialised.
-  void OnPasswordsSerialised(const base::FilePath& destination,
-                             const std::string& serialised);
+  // Callback after the passwords have been serialised. It reports the result to
+  // the UI and to metrics. |destination| is the folder we wrote to. |count| is
+  // the number of passwords exported. |success| is whether they were actually
+  // written.
+  void OnPasswordsExported(const base::FilePath& destination,
+                           int count,
+                           bool success);
 
   // Wrapper for the |on_progress_| callback, which caches |status|, so that
   // it can be provided by GetProgressStatus.
@@ -83,9 +88,10 @@ class PasswordManagerExporter {
   // The most recent progress status update, as was seen on |on_progress_|.
   ExportProgressStatus last_progress_status_;
 
-  // The password list that was read from the store. It will be cleared once
-  // exporting is complete.
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list_;
+  // The password list that was read from the store and serialised.
+  std::string serialised_password_list_;
+  // The number of passwords that were serialised. Useful for metrics.
+  size_t password_count_;
 
   // The destination which was provided and where the password list will be
   // sent. It will be cleared once exporting is complete.

components/password_manager/core/browser/export/password_manager_exporter_unittest.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/scoped_task_environment.h"
 #include "build/build_config.h"
@@ -117,6 +118,7 @@ class PasswordManagerExporterTest : public testing::Test {
   password_manager::PasswordManagerExporter exporter_;
   StrictMock<base::MockCallback<WriteCallback>> mock_write_file_;
   base::FilePath destination_path_;
+  base::HistogramTester histogram_tester_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerExporterTest);
@@ -143,6 +145,8 @@ TEST_F(PasswordManagerExporterTest, PasswordExportSetPasswordListFirst) {
   exporter_.SetDestination(destination_path_);
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectUniqueSample(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list.size(), 1);
 }
 
 TEST_F(PasswordManagerExporterTest, PasswordExportSetDestinationFirst) {
@@ -166,6 +170,8 @@ TEST_F(PasswordManagerExporterTest, PasswordExportSetDestinationFirst) {
   exporter_.PreparePasswordsForExport();
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectUniqueSample(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list.size(), 1);
 }
 
 TEST_F(PasswordManagerExporterTest, WriteFileFailed) {
@@ -187,6 +193,8 @@ TEST_F(PasswordManagerExporterTest, WriteFileFailed) {
   exporter_.PreparePasswordsForExport();
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectTotalCount(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", 0);
 }
 
 // Test that GetProgressStatus() returns the last ExportProgressStatus sent
@@ -222,11 +230,15 @@ TEST_F(PasswordManagerExporterTest, DontExportWithOnlyDestination) {
   fake_credential_provider_.SetPasswordList(password_list);
 
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).Times(0);
-  EXPECT_CALL(mock_on_progress_, Run(_, _)).Times(0);
+  EXPECT_CALL(
+      mock_on_progress_,
+      Run(password_manager::ExportProgressStatus::IN_PROGRESS, IsEmpty()));
 
   exporter_.SetDestination(destination_path_);
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectTotalCount(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", 0);
 }
 
 TEST_F(PasswordManagerExporterTest, CancelAfterPasswords) {
@@ -238,12 +250,17 @@ TEST_F(PasswordManagerExporterTest, CancelAfterPasswords) {
   EXPECT_CALL(
       mock_on_progress_,
       Run(password_manager::ExportProgressStatus::FAILED_CANCELLED, IsEmpty()));
+  EXPECT_CALL(
+      mock_on_progress_,
+      Run(password_manager::ExportProgressStatus::IN_PROGRESS, IsEmpty()));
 
   exporter_.PreparePasswordsForExport();
   exporter_.Cancel();
   exporter_.SetDestination(destination_path_);
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectTotalCount(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", 0);
 }
 
 TEST_F(PasswordManagerExporterTest, CancelAfterDestination) {
@@ -252,6 +269,9 @@ TEST_F(PasswordManagerExporterTest, CancelAfterDestination) {
   fake_credential_provider_.SetPasswordList(password_list);
 
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).Times(0);
+  EXPECT_CALL(
+      mock_on_progress_,
+      Run(password_manager::ExportProgressStatus::IN_PROGRESS, IsEmpty()));
   EXPECT_CALL(
       mock_on_progress_,
       Run(password_manager::ExportProgressStatus::FAILED_CANCELLED, IsEmpty()));
@@ -261,6 +281,8 @@ TEST_F(PasswordManagerExporterTest, CancelAfterDestination) {
   exporter_.PreparePasswordsForExport();
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectTotalCount(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", 0);
 }
 
 // Test that PasswordManagerExporter is reusable, after an export has been
@@ -291,6 +313,8 @@ TEST_F(PasswordManagerExporterTest, CancelAfterPasswordsThenExport) {
   exporter_.PreparePasswordsForExport();
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectUniqueSample(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list.size(), 1);
 }
 
 // Test that PasswordManagerExporter is reusable, after an export has been
@@ -313,7 +337,8 @@ TEST_F(PasswordManagerExporterTest, CancelAfterDestinationThenExport) {
       Run(password_manager::ExportProgressStatus::FAILED_CANCELLED, IsEmpty()));
   EXPECT_CALL(
       mock_on_progress_,
-      Run(password_manager::ExportProgressStatus::IN_PROGRESS, IsEmpty()));
+      Run(password_manager::ExportProgressStatus::IN_PROGRESS, IsEmpty()))
+      .Times(2);
   EXPECT_CALL(
       mock_on_progress_,
       Run(password_manager::ExportProgressStatus::SUCCEEDED, IsEmpty()));
@@ -324,6 +349,8 @@ TEST_F(PasswordManagerExporterTest, CancelAfterDestinationThenExport) {
   exporter_.SetDestination(destination_path_);
 
   scoped_task_environment_.RunUntilIdle();
+  histogram_tester_.ExpectUniqueSample(
+      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list.size(), 1);
 }
 
 }  // namespace