Fixes bugs in CrostiniRemover and CrostiniUninstallerView.

Also adds a browsertest for CrostiniUninstallerView.

Bug: 849438
Change-Id: I13ff94ec9324be0c01757f3094d76403c1d3b30f
Reviewed-on: https://chromium-review.googlesource.com/1092397
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566758}

chrome/browser/chromeos/crostini/crostini_remover.cc

@@ -54,10 +54,17 @@ void CrostiniRemover::OnConciergeStarted(ConciergeClientResult result) {
 }
 
 void CrostiniRemover::OnRestartCrostini(ConciergeClientResult result) {
+  // If we got here, it must be due to a failure before Concierge was started.
+  // |result| should never be SUCCESS.
   DCHECK_NE(result, ConciergeClientResult::SUCCESS)
       << "RestartCrostini should have been aborted after starting the "
          "Concierge service.";
-  LOG(ERROR) << "Failed to start concierge service";
+
+  // We still need to signal failure via |callback_|
+  if (result != ConciergeClientResult::SUCCESS) {
+    LOG(ERROR) << "Failed to start concierge service";
+    std::move(callback_).Run(result);
+  }
 }
 
 void CrostiniRemover::StopVmFinished(ConciergeClientResult result) {

chrome/browser/ui/views/crostini/crostini_uninstaller_view.cc

@@ -31,15 +31,6 @@ constexpr char kCrostiniUninstallSourceHistogram[] = "Crostini.UninstallSource";
 
 }  // namespace
 
-// These values are persisted to logs. Entries should not be renumbered and
-// numeric values should never be reused.
-enum class CrostiniUninstallerView::UninstallResult {
-  kCancelled = 0,
-  kError = 1,
-  kSuccess = 2,
-  kCount
-};
-
 void ShowCrostiniUninstallerView(Profile* profile,
                                  CrostiniUISurface ui_surface) {
   base::UmaHistogramEnumeration(kCrostiniUninstallSourceHistogram, ui_surface,
@@ -104,9 +95,9 @@ bool CrostiniUninstallerView::Accept() {
   // Setting value to -1 makes the progress bar play the
   // "indeterminate animation".
   progress_bar_->SetValue(-1);
+  DialogModelChanged();
   GetWidget()->UpdateWindowTitle();
   GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());
-  DialogModelChanged();
   return false;  // Should not close the dialog
 }
 
@@ -122,6 +113,11 @@ gfx::Size CrostiniUninstallerView::CalculatePreferredSize() const {
   return gfx::Size(dialog_width, GetHeightForWidth(dialog_width));
 }
 
+// static
+CrostiniUninstallerView* CrostiniUninstallerView::GetActiveViewForTesting() {
+  return g_crostini_uninstaller_view;
+}
+
 CrostiniUninstallerView::CrostiniUninstallerView(Profile* profile)
     : app_name_(base::ASCIIToUTF16(kCrostiniTerminalAppName)),
       profile_(profile),
@@ -154,8 +150,9 @@ void CrostiniUninstallerView::HandleError(const base::string16& error_message) {
   message_label_->SetVisible(true);
   message_label_->SetText(error_message);
   progress_bar_->SetVisible(false);
-  GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());
+  DialogModelChanged();
   GetWidget()->UpdateWindowTitle();
+  GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());
   RecordUninstallResultHistogram(UninstallResult::kError);
 }
 

chrome/browser/ui/views/crostini/crostini_uninstaller_view.h

@@ -22,6 +22,15 @@ class Profile;
 // uninstalls Crostinin if the user chooses to do so.
 class CrostiniUninstallerView : public views::DialogDelegateView {
  public:
+  // These values are persisted to logs. Entries should not be renumbered and
+  // numeric values should never be reused.
+  enum class UninstallResult {
+    kCancelled = 0,
+    kError = 1,
+    kSuccess = 2,
+    kCount
+  };
+
   static void Show(Profile* profile);
 
   // views::DialogDelegateView:
@@ -33,8 +42,9 @@ class CrostiniUninstallerView : public views::DialogDelegateView {
   bool Cancel() override;
   gfx::Size CalculatePreferredSize() const override;
 
+  static CrostiniUninstallerView* GetActiveViewForTesting();
+
  private:
-  enum class UninstallResult;
   enum class State {
     PROMPT,  // Prompting the user to allow uninstallation.
     ERROR,   // Something unexpected happened.

chrome/browser/ui/views/crostini/crostini_uninstaller_view_browsertest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/crostini/crostini_uninstaller_view.h"
+
+#include "base/metrics/histogram_base.h"
+#include "base/run_loop.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/chromeos/crostini/crostini_manager.h"
+#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/app_list_client_impl.h"
+#include "chrome/browser/ui/app_list/crostini/crostini_app_model_builder.h"
+#include "chrome/browser/ui/app_list/test/chrome_app_list_test_support.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/test/test_browser_dialog.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_concierge_client.h"
+#include "components/crx_file/id_util.h"
+#include "components/prefs/pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/window/dialog_client_view.h"
+
+class CrostiniUninstallerViewBrowserTest : public DialogBrowserTest {
+ public:
+  class WaitingFakeConciergeClient : public chromeos::FakeConciergeClient {
+   public:
+    void StopVm(
+        const vm_tools::concierge::StopVmRequest& request,
+        chromeos::DBusMethodCallback<vm_tools::concierge::StopVmResponse>
+            callback) override {
+      chromeos::FakeConciergeClient::StopVm(request, std::move(callback));
+      if (closure_) {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                      std::move(closure_));
+      }
+    }
+
+    void WaitForStopVmCalled() {
+      base::RunLoop loop;
+      closure_ = loop.QuitClosure();
+      loop.Run();
+      EXPECT_TRUE(stop_vm_called());
+    }
+
+   private:
+    base::OnceClosure closure_;
+  };
+
+  CrostiniUninstallerViewBrowserTest()
+      : waiting_fake_concierge_client_(new WaitingFakeConciergeClient()) {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetConciergeClient(
+        base::WrapUnique(waiting_fake_concierge_client_));
+  }
+
+  // DialogBrowserTest:
+  void ShowUi(const std::string& name) override {
+    ShowCrostiniUninstallerView(browser()->profile(),
+                                CrostiniUISurface::kSettings);
+  }
+
+  void SetUp() override {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kExperimentalCrostiniUI);
+    DialogBrowserTest::SetUp();
+  }
+
+  void SetUpOnMainThread() override {
+    browser()->profile()->GetPrefs()->SetBoolean(
+        crostini::prefs::kCrostiniEnabled, true);
+  }
+
+  CrostiniUninstallerView* ActiveView() {
+    return CrostiniUninstallerView::GetActiveViewForTesting();
+  }
+
+  bool HasAcceptButton() {
+    return ActiveView()->GetDialogClientView()->ok_button() != nullptr;
+  }
+
+  bool HasCancelButton() {
+    return ActiveView()->GetDialogClientView()->cancel_button() != nullptr;
+  }
+
+  void WaitForViewDestroyed() {
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(nullptr, ActiveView());
+  }
+
+ protected:
+  // Owned by chromeos::DBusThreadManager
+  WaitingFakeConciergeClient* waiting_fake_concierge_client_;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrostiniUninstallerViewBrowserTest);
+};
+
+// Test the dialog is actually launched from the app launcher.
+IN_PROC_BROWSER_TEST_F(CrostiniUninstallerViewBrowserTest, InvokeUi_default) {
+  ShowAndVerifyUi();
+}
+
+IN_PROC_BROWSER_TEST_F(CrostiniUninstallerViewBrowserTest, UninstallFlow) {
+  base::HistogramTester histogram_tester;
+
+  ShowUi("default");
+  EXPECT_NE(nullptr, ActiveView());
+  EXPECT_EQ(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL,
+            ActiveView()->GetDialogButtons());
+
+  EXPECT_TRUE(HasAcceptButton());
+  EXPECT_TRUE(HasCancelButton());
+
+  ActiveView()->GetDialogClientView()->AcceptWindow();
+  EXPECT_FALSE(ActiveView()->GetWidget()->IsClosed());
+  EXPECT_FALSE(HasAcceptButton());
+  EXPECT_FALSE(HasCancelButton());
+
+  WaitForViewDestroyed();
+
+  histogram_tester.ExpectBucketCount(
+      "Crostini.UninstallResult",
+      static_cast<base::HistogramBase::Sample>(
+          CrostiniUninstallerView::UninstallResult::kSuccess),
+      1);
+}
+
+IN_PROC_BROWSER_TEST_F(CrostiniUninstallerViewBrowserTest, Cancel) {
+  base::HistogramTester histogram_tester;
+
+  ShowUi("default");
+  EXPECT_NE(nullptr, ActiveView());
+  ActiveView()->GetDialogClientView()->CancelWindow();
+  EXPECT_TRUE(ActiveView()->GetWidget()->IsClosed());
+  WaitForViewDestroyed();
+
+  histogram_tester.ExpectBucketCount(
+      "Crostini.UninstallResult",
+      static_cast<base::HistogramBase::Sample>(
+          CrostiniUninstallerView::UninstallResult::kCancelled),
+      1);
+}
+
+IN_PROC_BROWSER_TEST_F(CrostiniUninstallerViewBrowserTest, ErrorThenCancel) {
+  base::HistogramTester histogram_tester;
+  ShowUi("default");
+  EXPECT_NE(nullptr, ActiveView());
+  vm_tools::concierge::StopVmResponse response;
+  response.set_success(false);
+  waiting_fake_concierge_client_->set_stop_vm_response(std::move(response));
+
+  ActiveView()->GetDialogClientView()->AcceptWindow();
+  EXPECT_FALSE(ActiveView()->GetWidget()->IsClosed());
+  waiting_fake_concierge_client_->WaitForStopVmCalled();
+  EXPECT_TRUE(HasCancelButton());
+  ActiveView()->GetDialogClientView()->CancelWindow();
+  WaitForViewDestroyed();
+
+  histogram_tester.ExpectBucketCount(
+      "Crostini.UninstallResult",
+      static_cast<base::HistogramBase::Sample>(
+          CrostiniUninstallerView::UninstallResult::kError),
+      1);
+}

chrome/test/BUILD.gn

@@ -1714,6 +1714,7 @@ test("browser_tests") {
         "../browser/ui/views/apps/chrome_native_app_window_views_aura_ash_browsertest.cc",
         "../browser/ui/views/arc_app_dialog_view_browsertest.cc",
         "../browser/ui/views/crostini/crostini_installer_view_browsertest.cc",
+        "../browser/ui/views/crostini/crostini_uninstaller_view_browsertest.cc",
         "../browser/ui/views/frame/browser_frame_ash_browsertest.cc",
         "../browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc",
         "../browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc",