From fc9e5516a215ec178c97c6ba2af8f10f80f87f24 Mon Sep 17 00:00:00 2001 From: Nic Hollingum Date: Thu, 22 Oct 2020 03:22:23 +0000 Subject: [PATCH] Revert "cbui crostini: refactor away CrostiniAppRestartView" This reverts commit b6deb4697d0883997795a1a54e7052453847d8d9. Reason for revert: Deterministically breaks linux-chromeos-dbg, e.g. https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/20956 Original change's description: > 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. > > Bug: 1075649 > Change-Id: I7de0e2386de14aa6aec8917c630f64a69df26832 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490345 > Commit-Queue: Elly Fong-Jones > Reviewed-by: Joel Hockey > Cr-Commit-Position: refs/heads/master@{#819545} TBR=ellyjones@chromium.org,joelhockey@chromium.org Change-Id: I230664fa24b78125632310bf05edd6a3d219f18b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1075649 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490971 Reviewed-by: Nic Hollingum Auto-Submit: Nic Hollingum Commit-Queue: Nic Hollingum Cr-Commit-Position: refs/heads/master@{#819687} --- chrome/browser/ui/BUILD.gn | 4 +- .../app_service_shelf_context_menu.cc | 4 +- .../crostini/crostini_app_restart_dialog.h | 17 ---- .../crostini_app_restart_dialog_unittest.cc | 47 ----------- ...dialog.cc => crostini_app_restart_view.cc} | 78 ++++++++----------- .../crostini/crostini_app_restart_view.h | 29 +++++++ chrome/test/BUILD.gn | 1 - 7 files changed, 67 insertions(+), 113 deletions(-) delete mode 100644 chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h delete mode 100644 chrome/browser/ui/views/crostini/crostini_app_restart_dialog_unittest.cc rename chrome/browser/ui/views/crostini/{crostini_app_restart_dialog.cc => crostini_app_restart_view.cc} (52%) create mode 100644 chrome/browser/ui/views/crostini/crostini_app_restart_view.h diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn index e6897b25af9ab5..8ec60090ced58e 100644 --- a/chrome/browser/ui/BUILD.gn +++ b/chrome/browser/ui/BUILD.gn @@ -2053,8 +2053,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_dialog.cc", - "views/crostini/crostini_app_restart_dialog.h", + "views/crostini/crostini_app_restart_view.cc", + "views/crostini/crostini_app_restart_view.h", "views/crostini/crostini_force_close_view.cc", "views/crostini/crostini_force_close_view.h", "views/crostini/crostini_package_install_failure_view.cc", diff --git a/chrome/browser/ui/ash/launcher/app_service/app_service_shelf_context_menu.cc b/chrome/browser/ui/ash/launcher/app_service/app_service_shelf_context_menu.cc index 3c7043fad5eb63..f79c4acce7c2b3 100644 --- a/chrome/browser/ui/ash/launcher/app_service/app_service_shelf_context_menu.cc +++ b/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_dialog.h" +#include "chrome/browser/ui/views/crostini/crostini_app_restart_view.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)) - crostini::ShowAppRestartDialog(display_id()); + CrostiniAppRestartView::Show(display_id()); return; } diff --git a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h b/chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h deleted file mode 100644 index 3352f078eb708f..00000000000000 --- a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h +++ /dev/null @@ -1,17 +0,0 @@ -// 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_ diff --git a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog_unittest.cc b/chrome/browser/ui/views/crostini/crostini_app_restart_dialog_unittest.cc deleted file mode 100644 index 6d17a39aac1d2e..00000000000000 --- a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog_unittest.cc +++ /dev/null @@ -1,47 +0,0 @@ -// 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)); -} diff --git a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog.cc b/chrome/browser/ui/views/crostini/crostini_app_restart_view.cc similarity index 52% rename from chrome/browser/ui/views/crostini/crostini_app_restart_dialog.cc rename to chrome/browser/ui/views/crostini/crostini_app_restart_view.cc index f4ff31c0c190ae..edc02a1ea483d3 100644 --- a/chrome/browser/ui/views/crostini/crostini_app_restart_dialog.cc +++ b/chrome/browser/ui/views/crostini/crostini_app_restart_view.cc @@ -1,9 +1,12 @@ -// Copyright 2020 The Chromium Authors. All rights reserved. +// 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_app_restart_dialog.h" +#include "chrome/browser/ui/views/crostini/crostini_app_restart_view.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" @@ -14,9 +17,6 @@ #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,11 +27,34 @@ gfx::NativeWindow GetNativeWindowFromDisplayId(int64_t display_id) { return screen->GetWindowAtScreenPoint(display.bounds().origin()); } -std::unique_ptr MakeCrostiniAppRestartView() { - auto view = std::make_unique(); +} // 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); views::LayoutProvider* provider = views::LayoutProvider::Get(); - view->SetLayoutManager(std::make_unique( + SetLayoutManager(std::make_unique( views::BoxLayout::Orientation::kVertical, provider->GetInsetsMetric(views::InsetsMetric::INSETS_DIALOG), provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL))); @@ -42,42 +65,9 @@ std::unique_ptr MakeCrostiniAppRestartView() { views::Label* message_label = new views::Label(message); message_label->SetMultiLine(true); message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT); - view->AddChildView(message_label); - - return view; + AddChildView(message_label); } -std::unique_ptr MakeCrostiniAppRestartDelegate( - std::unique_ptr contents) { - auto delegate = std::make_unique(); - 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); +ui::ModalType CrostiniAppRestartView::GetModalType() const { + return ui::MODAL_TYPE_SYSTEM; } - -} // namespace - -void ShowAppRestartDialog(int64_t display_id) { - ShowInternal(GetNativeWindowFromDisplayId(display_id)); -} - -void ShowAppRestartDialogForTesting(gfx::NativeWindow context) { - ShowInternal(context); -} - -} // namespace crostini diff --git a/chrome/browser/ui/views/crostini/crostini_app_restart_view.h b/chrome/browser/ui/views/crostini/crostini_app_restart_view.h new file mode 100644 index 00000000000000..5cf54631f63ace --- /dev/null +++ b/chrome/browser/ui/views/crostini/crostini_app_restart_view.h @@ -0,0 +1,29 @@ +// 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_ diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 89125c9bdee9b6..6ee6b4a9e407ff 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -3892,7 +3892,6 @@ 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", ] }