Skip to content

Commit

Permalink
Revert "cbui crostini: refactor away CrostiniAppRestartView"
Browse files Browse the repository at this point in the history
This reverts commit b6deb46.

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 <ellyjones@chromium.org>
> Reviewed-by: Joel Hockey <joelhockey@chromium.org>
> 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 <hollingum@google.com>
Auto-Submit: Nic Hollingum <hollingum@google.com>
Commit-Queue: Nic Hollingum <hollingum@google.com>
Cr-Commit-Position: refs/heads/master@{#819687}
  • Loading branch information
Nic Hollingum authored and Commit Bot committed Oct 22, 2020
1 parent dfd6d85 commit fc9e551
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 113 deletions.
4 changes: 2 additions & 2 deletions chrome/browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
}

Expand Down
17 changes: 0 additions & 17 deletions chrome/browser/ui/views/crostini/crostini_app_restart_dialog.h

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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 {

Expand All @@ -27,11 +27,34 @@ gfx::NativeWindow GetNativeWindowFromDisplayId(int64_t display_id) {
return screen->GetWindowAtScreenPoint(display.bounds().origin());
}

std::unique_ptr<views::View> MakeCrostiniAppRestartView() {
auto view = std::make_unique<views::View>();
} // 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<views::BoxLayout>(
SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kVertical,
provider->GetInsetsMetric(views::InsetsMetric::INSETS_DIALOG),
provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL)));
Expand All @@ -42,42 +65,9 @@ std::unique_ptr<views::View> 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<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);
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
29 changes: 29 additions & 0 deletions chrome/browser/ui/views/crostini/crostini_app_restart_view.h
Original file line number Diff line number Diff line change
@@ -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_
1 change: 0 additions & 1 deletion chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]
}

Expand Down

0 comments on commit fc9e551

Please sign in to comment.