Allow smarter choosing/re-using backup locations during crostini upgrade

One feature of the mocks that is yet to be implemented is the handling
of backup-location choice/re-use. In this CL we implement those
features. Specifically:
 - Backup will use the default location without bringing up a
   file-chooser (unless it is taken, in which case the file chooser is
   brought up to confirm the overwrite/rename)
 - If the user selects "change location" the file chooser will be shown.
 - If backup completed successfully and upgrade fails, restore will
   immediately begin from the known backup location, rather than
   prompting the user to find their backup.

Bug: 1024693
Change-Id: Iaa57f7162e7c12b483ba30b205c720dd88ea0c31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068400
Commit-Queue: Nic Hollingum <hollingum@google.com>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745370}

chrome/browser/chromeos/crostini/crostini_export_import.cc

@@ -152,6 +152,14 @@ void CrostiniExportImport::ImportContainer(ContainerId container_id,
       web_contents);
 }
 
+base::FilePath CrostiniExportImport::GetDefaultBackupPath() const {
+  base::Time::Exploded exploded;
+  base::Time::Now().LocalExplode(&exploded);
+  return file_manager::util::GetMyFilesFolderForProfile(profile_).Append(
+      base::StringPrintf("chromeos-linux-%04d-%02d-%02d.tini", exploded.year,
+                         exploded.month, exploded.day_of_month));
+}
+
 void CrostiniExportImport::OpenFileDialog(OperationData* operation_data,
                                           content::WebContents* web_contents) {
   if (!crostini::CrostiniFeatures::Get()->IsExportImportUIAllowed(profile_)) {
@@ -169,13 +177,7 @@ void CrostiniExportImport::OpenFileDialog(OperationData* operation_data,
     case ExportImportType::EXPORT:
       file_selector_mode = ui::SelectFileDialog::SELECT_SAVEAS_FILE;
       title = IDS_SETTINGS_CROSTINI_EXPORT;
-      base::Time::Exploded exploded;
-      base::Time::Now().LocalExplode(&exploded);
-      default_path =
-          file_manager::util::GetMyFilesFolderForProfile(profile_).Append(
-              base::StringPrintf("chromeos-linux-%04d-%02d-%02d.tini",
-                                 exploded.year, exploded.month,
-                                 exploded.day_of_month));
+      default_path = GetDefaultBackupPath();
       break;
     case ExportImportType::IMPORT:
       file_selector_mode = ui::SelectFileDialog::SELECT_OPEN_FILE,
@@ -227,6 +229,22 @@ void CrostiniExportImport::ImportContainer(
         path, std::move(callback));
 }
 
+void CrostiniExportImport::ExportContainer(ContainerId container_id,
+                                           base::FilePath path,
+                                           OnceTrackerFactory tracker_factory) {
+  Start(NewOperationData(ExportImportType::EXPORT, std::move(container_id),
+                         std::move(tracker_factory)),
+        path, base::DoNothing());
+}
+
+void CrostiniExportImport::ImportContainer(ContainerId container_id,
+                                           base::FilePath path,
+                                           OnceTrackerFactory tracker_factory) {
+  Start(NewOperationData(ExportImportType::IMPORT, std::move(container_id),
+                         std::move(tracker_factory)),
+        path, base::DoNothing());
+}
+
 void CrostiniExportImport::Start(
     OperationData* operation_data,
     base::FilePath path,

chrome/browser/chromeos/crostini/crostini_export_import.h

@@ -122,12 +122,26 @@ class CrostiniExportImport : public KeyedService,
                        content::WebContents* web_contents,
                        OnceTrackerFactory tracker_factory);
 
+  // Export |container| to |path| and invoke |tracker_factory| to create a
+  // tracker for this operation.
+  void ExportContainer(ContainerId container_id,
+                       base::FilePath path,
+                       OnceTrackerFactory tracker_factory);
+  // Import |container| from |path| and invoke |tracker_factory| to create a
+  // tracker for this operation.
+  void ImportContainer(ContainerId container_id,
+                       base::FilePath path,
+                       OnceTrackerFactory tracker_factory);
+
   // Cancel currently running export/import.
   void CancelOperation(ExportImportType type, ContainerId id);
 
   // Whether an export or import is currently in progress.
   bool GetExportImportOperationStatus() const;
 
+  // Returns the default location to export the container to.
+  base::FilePath GetDefaultBackupPath() const;
+
   base::WeakPtr<CrostiniExportImportNotificationController>
   GetNotificationControllerForTesting(ContainerId container_id);
 

chrome/browser/chromeos/crostini/crostini_upgrader.cc

@@ -5,9 +5,13 @@
 #include "chrome/browser/chromeos/crostini/crostini_upgrader.h"
 
 #include "base/barrier_closure.h"
+#include "base/files/file_util.h"
+#include "base/location.h"
+#include "base/optional.h"
 #include "base/system/sys_info.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
+#include "chrome/browser/chromeos/crostini/crostini_export_import.h"
 #include "chrome/browser/chromeos/crostini/crostini_export_import_status_tracker.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager_factory.h"
@@ -63,7 +67,10 @@ CrostiniUpgrader* CrostiniUpgrader::GetForProfile(Profile* profile) {
 }
 
 CrostiniUpgrader::CrostiniUpgrader(Profile* profile)
-    : profile_(profile), container_id_("", ""), pmc_observer_(this) {
+    : profile_(profile),
+      container_id_("", ""),
+      pmc_observer_(this),
+      backup_path_(base::nullopt) {
   CrostiniManager::GetForProfile(profile_)->AddUpgradeContainerProgressObserver(
       this);
 }
@@ -103,16 +110,17 @@ void CrostiniUpgrader::StatusTracker::SetStatusRunningUI(int progress_percent) {
 
 void CrostiniUpgrader::StatusTracker::SetStatusDoneUI() {
   if (type() == ExportImportType::EXPORT) {
-    upgrader_->OnBackup(CrostiniResult::SUCCESS);
+    upgrader_->OnBackup(CrostiniResult::SUCCESS, path());
   } else {
     upgrader_->OnRestore(CrostiniResult::SUCCESS);
   }
 }
 
 void CrostiniUpgrader::StatusTracker::SetStatusCancelledUI() {
-  // Successfully canceled backup/restore. Upgrade can continue.
+  // Successfully canceled backup/restore. Upgrade can continue but we will not
+  // use that backup to restore from.
   if (type() == ExportImportType::EXPORT) {
-    upgrader_->OnBackup(CrostiniResult::SUCCESS);
+    upgrader_->OnBackup(CrostiniResult::SUCCESS, base::nullopt);
   } else {
     upgrader_->OnRestore(CrostiniResult::SUCCESS);
   }
@@ -126,25 +134,52 @@ void CrostiniUpgrader::StatusTracker::SetStatusFailedWithMessageUI(
     result = CrostiniResult::CONTAINER_EXPORT_IMPORT_FAILED_SPACE;
   }
   if (type() == ExportImportType::EXPORT) {
-    upgrader_->OnBackup(result);
+    upgrader_->OnBackup(result, base::nullopt);
   } else {
     upgrader_->OnRestore(result);
   }
 }
 
 void CrostiniUpgrader::Backup(const ContainerId& container_id,
+                              bool show_file_chooser,
                               content::WebContents* web_contents) {
-  CrostiniExportImport::GetForProfile(profile_)->ExportContainer(
-      container_id, web_contents, MakeFactory());
+  if (show_file_chooser) {
+    CrostiniExportImport::GetForProfile(profile_)->ExportContainer(
+        container_id, web_contents, MakeFactory());
+    return;
+  }
+  base::FilePath default_path =
+      CrostiniExportImport::GetForProfile(profile_)->GetDefaultBackupPath();
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::ThreadPool(), base::MayBlock()},
+      base::BindOnce(&base::PathExists, default_path),
+      base::BindOnce(&CrostiniUpgrader::OnBackupPathChecked,
+                     weak_ptr_factory_.GetWeakPtr(), container_id, web_contents,
+                     default_path));
+}
+
+void CrostiniUpgrader::OnBackupPathChecked(const ContainerId& container_id,
+                                           content::WebContents* web_contents,
+                                           base::FilePath path,
+                                           bool path_exists) {
+  if (path_exists) {
+    CrostiniExportImport::GetForProfile(profile_)->ExportContainer(
+        container_id, web_contents, MakeFactory());
+  } else {
+    CrostiniExportImport::GetForProfile(profile_)->ExportContainer(
+        container_id, path, MakeFactory());
+  }
 }
 
-void CrostiniUpgrader::OnBackup(CrostiniResult result) {
+void CrostiniUpgrader::OnBackup(CrostiniResult result,
+                                base::Optional<base::FilePath> backup_path) {
   if (result != CrostiniResult::SUCCESS) {
     for (auto& observer : upgrader_observers_) {
       observer.OnBackupFailed();
     }
     return;
   }
+  backup_path_ = backup_path;
   for (auto& observer : upgrader_observers_) {
     observer.OnBackupSucceeded();
   }
@@ -242,8 +277,30 @@ void CrostiniUpgrader::OnUpgrade(CrostiniResult result) {
 
 void CrostiniUpgrader::Restore(const ContainerId& container_id,
                                content::WebContents* web_contents) {
-  CrostiniExportImport::GetForProfile(profile_)->ImportContainer(
-      container_id, web_contents, MakeFactory());
+  if (!backup_path_.has_value()) {
+    CrostiniExportImport::GetForProfile(profile_)->ImportContainer(
+        container_id, web_contents, MakeFactory());
+    return;
+  }
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::ThreadPool(), base::MayBlock()},
+      base::BindOnce(&base::PathExists, *backup_path_),
+      base::BindOnce(&CrostiniUpgrader::OnRestorePathChecked,
+                     weak_ptr_factory_.GetWeakPtr(), container_id, web_contents,
+                     *backup_path_));
+}
+
+void CrostiniUpgrader::OnRestorePathChecked(const ContainerId& container_id,
+                                            content::WebContents* web_contents,
+                                            base::FilePath path,
+                                            bool path_exists) {
+  if (!path_exists) {
+    CrostiniExportImport::GetForProfile(profile_)->ImportContainer(
+        container_id, web_contents, MakeFactory());
+  } else {
+    CrostiniExportImport::GetForProfile(profile_)->ImportContainer(
+        container_id, path, MakeFactory());
+  }
 }
 
 void CrostiniUpgrader::OnRestore(CrostiniResult result) {

chrome/browser/chromeos/crostini/crostini_upgrader.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_UPGRADER_H_
 
 #include "base/callback_forward.h"
+#include "base/optional.h"
 #include "base/scoped_observer.h"
 #include "chrome/browser/chromeos/crostini/crostini_export_import.h"
 #include "chrome/browser/chromeos/crostini/crostini_export_import_status_tracker.h"
@@ -38,6 +39,7 @@ class CrostiniUpgrader : public KeyedService,
   void AddObserver(CrostiniUpgraderUIObserver* observer) override;
   void RemoveObserver(CrostiniUpgraderUIObserver* observer) override;
   void Backup(const ContainerId& container_id,
+              bool show_file_chooser,
               content::WebContents* web_contents) override;
   void StartPrechecks() override;
   void Upgrade(const ContainerId& container_id) override;
@@ -64,12 +66,24 @@ class CrostiniUpgrader : public KeyedService,
   static constexpr int64_t kDiskRequired = 1 << 30;
 
  private:
+  void OnBackupPathChecked(const ContainerId& container_id,
+                           content::WebContents* web_contents,
+                           base::FilePath path,
+                           bool path_exists);
+  // Called when backup completes. If backup was completed successfully (which
+  // is different from if |result|==SUCCESS) the |backup_path| will contain a
+  // path to the backup tarball.
+  void OnBackup(CrostiniResult result,
+                base::Optional<base::FilePath> backup_path);
   void OnCancel(CrostiniResult result);
-  void OnBackup(CrostiniResult result);
   void OnBackupProgress(int progress_percent);
   void OnUpgrade(CrostiniResult result);
   void OnAvailableDiskSpace(int64_t bytes);
   void DoPrechecks();
+  void OnRestorePathChecked(const ContainerId& container_id,
+                            content::WebContents* web_contents,
+                            base::FilePath path,
+                            bool path_exists);
   void OnRestore(CrostiniResult result);
   void OnRestoreProgress(int progress_percent);
   CrostiniExportImport::OnceTrackerFactory MakeFactory();
@@ -106,6 +120,11 @@ class CrostiniUpgrader : public KeyedService,
                  chromeos::PowerManagerClient::Observer>
       pmc_observer_;
 
+  // When restoring after a failed upgrade, if the user successfully completed a
+  // backup, we will auto-restore from that (if the file still exists),
+  // otherwise |backup_path_|==nullopt and restore will bring up a file-chooser.
+  base::Optional<base::FilePath> backup_path_;
+
   base::WeakPtrFactory<CrostiniUpgrader> weak_ptr_factory_{this};
 };
 

chrome/browser/chromeos/crostini/crostini_upgrader_ui_delegate.h

@@ -40,8 +40,11 @@ class CrostiniUpgraderUIDelegate {
   virtual void AddObserver(CrostiniUpgraderUIObserver* observer) = 0;
   virtual void RemoveObserver(CrostiniUpgraderUIObserver* observer) = 0;
 
-  // Back up the current container before upgrading
+  // Back up the current container before upgrading. If |show_file_chooser|
+  // is true, the user will be able to select the backup location via a file
+  // chooser.
   virtual void Backup(const ContainerId& container_id,
+                      bool show_file_chooser,
                       content::WebContents* web_contents) = 0;
 
   virtual void StartPrechecks() = 0;

chrome/browser/resources/chromeos/crostini_upgrader/app.html

@@ -127,7 +127,7 @@
     <cr-checkbox checked="{{backupCheckboxChecked_}}" >
       <p class="checkbox-text">
         $i18n{backupCheckboxMessage}
-        <a href="$i18n{learnMoreUrl}" target="_blank">
+        <a href="#" on-click="onChangeLocationButtonClick_">
         $i18n{backupChangeLocation}</a>
       </p>
     </cr-checkbox>

chrome/browser/resources/chromeos/crostini_upgrader/app.js

@@ -210,7 +210,7 @@ Polymer({
         }, () => {});
       case State.PROMPT:
         if (this.backupCheckboxChecked_) {
-          this.startBackup_();
+          this.startBackup_(/*showFileChooser=*/ false);
         } else {
           this.startPrechecks_(() => {
             this.startUpgrade_();
@@ -247,9 +247,17 @@ Polymer({
   },
 
   /** @private */
-  startBackup_() {
+  onChangeLocationButtonClick_() {
+    this.startBackup_(/*showFileChooser=*/ true);
+  },
+
+  /**
+   * @param {boolean} showFileChooser
+   * @private
+   */
+  startBackup_(showFileChooser) {
     this.state_ = State.BACKUP;
-    BrowserProxy.getInstance().handler.backup();
+    BrowserProxy.getInstance().handler.backup(showFileChooser);
   },
 
   /** @private */

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader.mojom

@@ -27,8 +27,9 @@ interface PageHandlerFactory {
 // Lives in the browser process. A renderer uses this to control Crostini
 // upgrade.
 interface PageHandler {
-  // Backup the existing container.
-  Backup();
+  // Backup the existing container. If |show_file_chooser| is true, the
+  // user will be shown a dialog to decide where to store the backup.
+  Backup(bool show_file_chooser);
   // Start running upgrade prechecks. Result is asynchronously
   // returned via Page::PrecheckStatus.
   StartPrechecks();

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_page_handler.cc

@@ -42,12 +42,12 @@ void Redisplay() {
 
 }  // namespace
 
-void CrostiniUpgraderPageHandler::Backup() {
+void CrostiniUpgraderPageHandler::Backup(bool show_file_chooser) {
   Redisplay();
   upgrader_ui_delegate_->Backup(
       crostini::ContainerId(crostini::kCrostiniDefaultVmName,
                             crostini::kCrostiniDefaultContainerName),
-      web_contents_);
+      show_file_chooser, web_contents_);
 }
 
 void CrostiniUpgraderPageHandler::StartPrechecks() {

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_page_handler.h

@@ -37,7 +37,7 @@ class CrostiniUpgraderPageHandler
   ~CrostiniUpgraderPageHandler() override;
 
   // chromeos::crostini_upgrader::mojom::PageHandler:
-  void Backup() override;
+  void Backup(bool show_file_chooser) override;
   void StartPrechecks() override;
   void Upgrade() override;
   void Restore() override;