Upgrading Crostini via settings, don't always end by showing a Terminal.

The original implementation ended the Crostini Upgrade by displaying a
Terminal connected to the upgraded Crostini container. This is not
wanted when the user has canceled the upgrade.

Bug: 1060770
Change-Id: If27c8dd90657eac76c3478b1c57351fadfba00e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2109571Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752658}

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

@@ -24,21 +24,26 @@ GURL GetUrl() {
 
 namespace chromeos {
 
-void CrostiniUpgraderDialog::Show(base::OnceClosure launch_closure) {
+void CrostiniUpgraderDialog::Show(base::OnceClosure launch_closure,
+                                  bool only_run_launch_closure_on_restart) {
   auto* instance = SystemWebDialogDelegate::FindInstance(GetUrl().spec());
   if (instance) {
     instance->Focus();
     return;
   }
 
-  instance = new CrostiniUpgraderDialog(std::move(launch_closure));
+  instance = new CrostiniUpgraderDialog(std::move(launch_closure),
+                                        only_run_launch_closure_on_restart);
   instance->ShowSystemDialog();
   base::UmaHistogramEnumeration(crostini::kUpgradeDialogEventHistogram,
                                 crostini::UpgradeDialogEvent::kDialogShown);
 }
 
-CrostiniUpgraderDialog::CrostiniUpgraderDialog(base::OnceClosure launch_closure)
+CrostiniUpgraderDialog::CrostiniUpgraderDialog(
+    base::OnceClosure launch_closure,
+    bool only_run_launch_closure_on_restart)
     : SystemWebDialogDelegate{GetUrl(), /*title=*/{}},
+      only_run_launch_closure_on_restart_(only_run_launch_closure_on_restart),
       launch_closure_{std::move(launch_closure)} {}
 
 CrostiniUpgraderDialog::~CrostiniUpgraderDialog() = default;
@@ -71,12 +76,15 @@ bool CrostiniUpgraderDialog::CanCloseDialog() const {
 namespace {
 void RunLaunchClosure(base::WeakPtr<crostini::CrostiniManager> crostini_manager,
                       base::OnceClosure launch_closure,
+                      bool only_run_launch_closure_on_restart,
                       bool restart_required) {
   if (!crostini_manager) {
     return;
   }
   if (!restart_required) {
+    if (!only_run_launch_closure_on_restart) {
       std::move(launch_closure).Run();
+    }
     return;
   }
   crostini_manager->RestartCrostini(
@@ -106,9 +114,9 @@ void CrostiniUpgraderDialog::OnDialogShown(content::WebUI* webui) {
                             crostini::kCrostiniDefaultContainerName));
 
   upgrader_ui_ = static_cast<CrostiniUpgraderUI*>(webui->GetController());
-  upgrader_ui_->set_launch_callback(
-      base::BindOnce(&RunLaunchClosure, crostini_manager->GetWeakPtr(),
-                     std::move(launch_closure_)));
+  upgrader_ui_->set_launch_callback(base::BindOnce(
+      &RunLaunchClosure, crostini_manager->GetWeakPtr(),
+      std::move(launch_closure_), only_run_launch_closure_on_restart_));
   return SystemWebDialogDelegate::OnDialogShown(webui);
 }
 

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

@@ -13,10 +13,18 @@ class CrostiniUpgraderUI;
 
 class CrostiniUpgraderDialog : public SystemWebDialogDelegate {
  public:
-  static void Show(base::OnceClosure launch_closure);
+  // If a restart of Crostini is not required, the launch closure can be
+  // optionally dropped. e.g. when running the upgrader from Settings, if the
+  // user cancels before starting the upgrade, the launching of a Terminal is
+  // not desired. This contrasts to being propmpted for container upgrade from
+  // The app launcher, in which case the user could opt not to upgrade and the
+  // app should still be launched.
+  static void Show(base::OnceClosure launch_closure,
+                   bool only_run_launch_closure_on_restart = false);
 
  private:
-  explicit CrostiniUpgraderDialog(base::OnceClosure launch_closure);
+  explicit CrostiniUpgraderDialog(base::OnceClosure launch_closure,
+                                  bool only_run_launch_closure_on_restart);
   ~CrostiniUpgraderDialog() override;
 
   // SystemWebDialogDelegate:
@@ -30,8 +38,8 @@ class CrostiniUpgraderDialog : public SystemWebDialogDelegate {
                        bool* out_close_dialog) override;
 
  private:
-  // Not owned.
-  CrostiniUpgraderUI* upgrader_ui_ = nullptr;
+  CrostiniUpgraderUI* upgrader_ui_ = nullptr;  // Not owned.
+  const bool only_run_launch_closure_on_restart_;
   base::OnceClosure launch_closure_;
 };
 

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

@@ -87,22 +87,23 @@ void CrostiniUpgraderPageHandler::Cancel() {
 }
 
 void CrostiniUpgraderPageHandler::Launch() {
-  std::move(launch_callback_).Run(true);
+  std::move(launch_callback_).Run(restart_required_);
 }
 
 void CrostiniUpgraderPageHandler::CancelBeforeStart() {
   base::UmaHistogramEnumeration(crostini::kUpgradeDialogEventHistogram,
                                 crostini::UpgradeDialogEvent::kNotStarted);
+  restart_required_ = false;
   upgrader_ui_delegate_->CancelBeforeStart();
   if (launch_callback_) {
     // Running launch closure - no upgrade wanted, no need to restart crostini.
-    std::move(launch_callback_).Run(false);
+    Launch();
   }
 }
 
 void CrostiniUpgraderPageHandler::Close() {
   if (launch_callback_) {
-    std::move(launch_callback_).Run(true);
+    Launch();
   }
   std::move(close_dialog_callback_).Run();
 }

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

@@ -70,6 +70,10 @@ class CrostiniUpgraderPageHandler
   mojo::Remote<chromeos::crostini_upgrader::mojom::Page> page_;
   base::OnceClosure close_dialog_callback_;
   base::OnceCallback<void(bool)> launch_callback_;
+  // Will we need to restart the container as part of launch_callback?
+  // |restart_required_| is true unless the user cancels before starting the
+  // upgrade.
+  bool restart_required_ = true;
 
   base::WeakPtrFactory<CrostiniUpgraderPageHandler> weak_ptr_factory_{this};
 

chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc

@@ -445,8 +445,12 @@ void CrostiniHandler::LaunchTerminal() {
 void CrostiniHandler::HandleRequestContainerUpgradeView(
     const base::ListValue* args) {
   CHECK_EQ(0U, args->GetList().size());
-  chromeos::CrostiniUpgraderDialog::Show(base::BindOnce(
-      &CrostiniHandler::LaunchTerminal, weak_ptr_factory_.GetWeakPtr()));
+  chromeos::CrostiniUpgraderDialog::Show(
+      base::BindOnce(&CrostiniHandler::LaunchTerminal,
+                     weak_ptr_factory_.GetWeakPtr()),
+      // If the user cancels the upgrade, we won't need to restart Crostini and
+      // we don't want to run the launch closure which would launch Terminal.
+      /*only_run_launch_closure_on_restart=*/true);
 }
 
 void CrostiniHandler::OnCrostiniExportImportOperationStatusChanged(