cbui crostini: refactor away CrostiniAppRestartView

This change removes CrostiniAppRestartView, replacing it with:
* Construction and configuration of a bare View + DialogDelegate pair
* A pair of public functions that create and show this dialog

This change also adds some unit test coverage for this dialog to
ensure that I didn't break it when doing this refactor.

This change also fixes a bug in the dialog found by the new tests: the
message used for the body was being given a parameter that was never
used, which would have caused a crash if it had ever happened in a debug
build.

Bug: 1075649
Change-Id: Ia29640e1e47bd96c2996eba6c39722c477ff4aab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490345
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#819545}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490518
Cr-Commit-Position: refs/heads/master@{#819979}

chrome/browser/ui/BUILD.gn

@@ -2051,8 +2051,8 @@ static_library("ui") {
       "views/chrome_views_delegate_chromeos.cc",
       "views/crostini/crostini_ansible_software_config_view.cc",
       "views/crostini/crostini_ansible_software_config_view.h",
-      "views/crostini/crostini_app_restart_view.cc",
-      "views/crostini/crostini_app_restart_view.h",
+      "views/crostini/crostini_app_restart_dialog.cc",
+      "views/crostini/crostini_app_restart_dialog.h",
       "views/crostini/crostini_force_close_view.cc",
       "views/crostini/crostini_force_close_view.h",
       "views/crostini/crostini_package_install_failure_view.cc",

chrome/browser/ui/ash/launcher/app_service/app_service_shelf_context_menu.cc

@@ -37,7 +37,7 @@
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/chrome_pages.h"
-#include "chrome/browser/ui/views/crostini/crostini_app_restart_view.h"
+#include "chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h"
 #include "chrome/browser/ui/webui/settings/chromeos/app_management/app_management_uma.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/app_registry_controller.h"
@@ -182,7 +182,7 @@ void AppServiceShelfContextMenu::ExecuteCommand(int command_id,
       const bool scaled = command_id == ash::CROSTINI_USE_LOW_DENSITY;
       registry_service->SetAppScaled(item().id.app_id, scaled);
       if (controller()->IsOpen(item().id))
-        CrostiniAppRestartView::Show(display_id());
+        crostini::ShowAppRestartDialog(display_id());
       return;
     }
 

chrome/browser/ui/views/crostini/crostini_app_restart_view.cc → chrome/browser/ui/views/crostini/crostini_app_restart_dialog.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
+// Copyright 2020 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_app_restart_view.h"
+#include "chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h"
 
-#include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_crostini_tracker.h"
-#include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.h"
-#include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_types.h"
-#include "ui/chromeos/devicetype_utils.h"
 #include "ui/display/screen.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/layout_provider.h"
+#include "ui/views/window/dialog_delegate.h"
+
+namespace crostini {
 
 namespace {
 
@@ -27,47 +26,56 @@ gfx::NativeWindow GetNativeWindowFromDisplayId(int64_t display_id) {
   return screen->GetWindowAtScreenPoint(display.bounds().origin());
 }
 
-}  // namespace
-
-// static
-void CrostiniAppRestartView::Show(int64_t display_id) {
-  CrostiniAppRestartView* view = new CrostiniAppRestartView();
-  views::DialogDelegate::CreateDialogWidget(
-      view, GetNativeWindowFromDisplayId(display_id), nullptr);
-  view->GetWidget()->Show();
-  chrome::RecordDialogCreation(chrome::DialogIdentifier::CROSTINI_APP_RESTART);
-}
-
-bool CrostiniAppRestartView::ShouldShowCloseButton() const {
-  return false;
-}
-
-gfx::Size CrostiniAppRestartView::CalculatePreferredSize() const {
-  const int dialog_width = ChromeLayoutProvider::Get()->GetDistanceMetric(
-                               views::DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH) -
-                           margins().width();
-  return gfx::Size(dialog_width, GetHeightForWidth(dialog_width));
-}
-
-CrostiniAppRestartView::CrostiniAppRestartView() {
-  // This dialog just has a generic "ok".
-  SetButtons(ui::DIALOG_BUTTON_OK);
+std::unique_ptr<views::View> MakeCrostiniAppRestartView() {
+  auto view = std::make_unique<views::View>();
 
   views::LayoutProvider* provider = views::LayoutProvider::Get();
-  SetLayoutManager(std::make_unique<views::BoxLayout>(
+  view->SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical,
       provider->GetInsetsMetric(views::InsetsMetric::INSETS_DIALOG),
       provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL)));
 
-  const base::string16 device_type = ui::GetChromeOSDeviceName();
   const base::string16 message =
-      l10n_util::GetStringFUTF16(IDS_CROSTINI_APP_RESTART_BODY, device_type);
+      l10n_util::GetStringUTF16(IDS_CROSTINI_APP_RESTART_BODY);
   views::Label* message_label = new views::Label(message);
   message_label->SetMultiLine(true);
   message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  AddChildView(message_label);
+  view->AddChildView(message_label);
+
+  return view;
 }
 
-ui::ModalType CrostiniAppRestartView::GetModalType() const {
-  return ui::MODAL_TYPE_SYSTEM;
+std::unique_ptr<views::DialogDelegate> MakeCrostiniAppRestartDelegate(
+    std::unique_ptr<views::View> contents) {
+  auto delegate = std::make_unique<views::DialogDelegate>();
+  delegate->SetButtons(ui::DIALOG_BUTTON_OK);
+  delegate->SetContentsView(std::move(contents));
+  delegate->SetModalType(ui::MODAL_TYPE_SYSTEM);
+  delegate->SetOwnedByWidget(true);
+  delegate->SetShowCloseButton(false);
+  delegate->set_fixed_width(ChromeLayoutProvider::Get()->GetDistanceMetric(
+      views::DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH));
+
+  return delegate;
+}
+
+void ShowInternal(gfx::NativeWindow context) {
+  auto contents = MakeCrostiniAppRestartView();
+  auto delegate = MakeCrostiniAppRestartDelegate(std::move(contents));
+  views::DialogDelegate::CreateDialogWidget(std::move(delegate), context,
+                                            nullptr)
+      ->Show();
+  chrome::RecordDialogCreation(chrome::DialogIdentifier::CROSTINI_APP_RESTART);
 }
+
+}  // namespace
+
+void ShowAppRestartDialog(int64_t display_id) {
+  ShowInternal(GetNativeWindowFromDisplayId(display_id));
+}
+
+void ShowAppRestartDialogForTesting(gfx::NativeWindow context) {
+  ShowInternal(context);
+}
+
+}  // namespace crostini

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

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_DIALOG_H_
+#define CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_DIALOG_H_
+
+#include "ui/gfx/native_widget_types.h"
+
+namespace crostini {
+
+void ShowAppRestartDialog(int64_t display_id);
+void ShowAppRestartDialogForTesting(gfx::NativeWindow context);
+
+}  // namespace crostini
+
+#endif  // CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_DIALOG_H_

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

+// Copyright 2020 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_app_restart_dialog.h"
+
+#include "chrome/browser/ui/views/chrome_layout_provider.h"
+#include "chrome/test/views/chrome_views_test_base.h"
+#include "ui/views/test/widget_test.h"
+#include "ui/views/widget/any_widget_observer.h"
+#include "ui/views/window/dialog_delegate.h"
+
+class CrostiniAppRestartDialogTest : public ChromeViewsTestBase {
+ public:
+  CrostiniAppRestartDialogTest() = default;
+  ~CrostiniAppRestartDialogTest() override = default;
+
+  views::test::WidgetTest::WidgetAutoclosePtr ShowDialog() {
+    // TODO(ellyjones): the class name being "View" is kind of awkward. It
+    // should be possible to set the class name even when there is no actual
+    // class...
+    views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey{},
+                                         "View");
+    crostini::ShowAppRestartDialogForTesting(GetContext());
+    return views::test::WidgetTest::WidgetAutoclosePtr(
+        waiter.WaitIfNeededAndGet());
+  }
+};
+
+TEST_F(CrostiniAppRestartDialogTest, OnlyHasOkButton) {
+  auto widget = ShowDialog();
+  EXPECT_EQ(widget->widget_delegate()->AsDialogDelegate()->GetDialogButtons(),
+            ui::DIALOG_BUTTON_OK);
+}
+
+TEST_F(CrostiniAppRestartDialogTest, IsSystemModal) {
+  auto widget = ShowDialog();
+  EXPECT_EQ(widget->widget_delegate()->AsDialogDelegate()->GetModalType(),
+            ui::MODAL_TYPE_SYSTEM);
+}
+
+TEST_F(CrostiniAppRestartDialogTest, ContentsViewHasModalPreferredWidth) {
+  auto widget = ShowDialog();
+  EXPECT_EQ(widget->widget_delegate()->GetContentsView()->width(),
+            ChromeLayoutProvider::Get()->GetDistanceMetric(
+                views::DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH));
+}

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

-// 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.
-
-#ifndef CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_VIEW_H_
-#define CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_VIEW_H_
-
-#include "ash/public/cpp/shelf_types.h"
-#include "ui/views/window/dialog_delegate.h"
-
-// Provide user a choice to restart the app after display density change.
-class CrostiniAppRestartView : public views::DialogDelegateView {
- public:
-  // Create and show a new dialog.
-  static void Show(int64_t display_id);
-
-  // views::DialogDelegateView:
-  bool ShouldShowCloseButton() const override;
-  gfx::Size CalculatePreferredSize() const override;
-  ui::ModalType GetModalType() const override;
-
- private:
-  CrostiniAppRestartView();
-  ~CrostiniAppRestartView() override = default;
-
-  DISALLOW_COPY_AND_ASSIGN(CrostiniAppRestartView);
-};
-
-#endif  // CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_APP_RESTART_VIEW_H_

chrome/test/BUILD.gn

@@ -3890,6 +3890,7 @@ test("unit_tests") {
       "../browser/performance_manager/policies/userspace_swap_policy_chromeos_unittest.cc",
       "../browser/performance_manager/policies/working_set_trimmer_policy_chromeos_unittest.cc",
       "../browser/signin/signin_status_metrics_provider_chromeos_unittest.cc",
+      "../browser/ui/views/crostini/crostini_app_restart_dialog_unittest.cc",
     ]
   }