Skip to content

Commit

Permalink
float: Move nudge controller from ash/ to chromeos/
Browse files Browse the repository at this point in the history
The first step for making the nudge work in lacros. This CL just moves
the controller into chromeos, and removes all ash/ dependencies.
Lacros support to be added in the future.

Old (ash)  ->  New (chromeos)
TabletModeController -> TabletState
SessionController -> Delegate
SystemToastStyle -> Custom Label

TODO:
Remove controller and have frame size button own the nudge.
Make calls to controller handle async operations, for lacros.

Test: existing, manual
Bug: b/267787811
Change-Id: I8c5c325a6edfacf0f513679ce761b9356e010156
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4234910
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1104715}
  • Loading branch information
Sammie Quon authored and Chromium LUCI CQ committed Feb 13, 2023
1 parent a705013 commit 2bb7e39
Show file tree
Hide file tree
Showing 20 changed files with 290 additions and 136 deletions.
4 changes: 2 additions & 2 deletions ash/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,8 @@ component("ash") {
"wm/multi_display/persistent_window_controller.h",
"wm/multi_display/persistent_window_info.cc",
"wm/multi_display/persistent_window_info.h",
"wm/multitask_menu_nudge_delegate_ash.cc",
"wm/multitask_menu_nudge_delegate_ash.h",

# These headers declare functions that are implemented inside of //ash, so
# they cannot live in //ash/public/cpp/BUILD.gn. The "test api" files are
Expand Down Expand Up @@ -2224,8 +2226,6 @@ component("ash") {
"wm/mru_window_tracker.h",
"wm/multi_display/multi_display_metrics_controller.cc",
"wm/multi_display/multi_display_metrics_controller.h",
"wm/multitask_menu_nudge_controller.cc",
"wm/multitask_menu_nudge_controller.h",
"wm/native_cursor_manager_ash.cc",
"wm/native_cursor_manager_ash.h",
"wm/overlay_event_filter.cc",
Expand Down
4 changes: 2 additions & 2 deletions ash/ash_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@
#include "ash/wm/desks/persistent_desks_bar/persistent_desks_bar_controller.h"
#include "ash/wm/desks/templates/saved_desk_util.h"
#include "ash/wm/lock_state_controller.h"
#include "ash/wm/multitask_menu_nudge_controller.h"
#include "ash/wm/window_cycle/window_cycle_controller.h"
#include "chromeos/ash/services/assistant/public/cpp/assistant_prefs.h"
#include "chromeos/components/quick_answers/public/cpp/quick_answers_prefs.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"
#include "chromeos/ui/wm/fullscreen/pref_names.h"
#include "components/language/core/browser/pref_names.h"
#include "components/live_caption/pref_names.h"
Expand Down Expand Up @@ -127,7 +127,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry, bool for_test) {
VPNListView::RegisterProfilePrefs(registry);
WallpaperPrefManager::RegisterProfilePrefs(registry);
WindowCycleController::RegisterProfilePrefs(registry);
MultitaskMenuNudgeController::RegisterProfilePrefs(registry);
chromeos::MultitaskMenuNudgeController::RegisterProfilePrefs(registry);

// Provide prefs registered in the browser for ash_unittests.
if (for_test) {
Expand Down
8 changes: 0 additions & 8 deletions ash/ash_strings.grd
Original file line number Diff line number Diff line change
Expand Up @@ -6275,14 +6275,6 @@ New install
Update and shut down
</message>

<!-- Multitask menu -->
<message name="IDS_MULTITASK_MENU_NUDGE_TEXT" desc="Text that is shown when the clamshell multitask menu nudge is displayed.">
Keep hovering for more layout options
</message>
<message name="IDS_TABLET_MULTITASK_MENU_NUDGE_TEXT" desc="Text that is shown when the tablet multitask menu nudge is displayed.">
Swipe down for more layout options
</message>

<!-- Glanceables -->
<message name="IDS_GLANCEABLES_WELCOME_LABEL" desc="Personalized greeting / welcome message shown on glanceables surfaces (welcome screen and overview mode).">
Welcome back, <ph name="GIVEN_NAME">$1<ex>John</ex></ph>
Expand Down
1 change: 0 additions & 1 deletion ash/ash_strings_grd/IDS_MULTITASK_MENU_NUDGE_TEXT.png.sha1

This file was deleted.

This file was deleted.

19 changes: 12 additions & 7 deletions ash/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@
#include "ash/wm/mru_window_tracker.h"
#include "ash/wm/multi_display/multi_display_metrics_controller.h"
#include "ash/wm/multi_display/persistent_window_controller.h"
#include "ash/wm/multitask_menu_nudge_controller.h"
#include "ash/wm/multitask_menu_nudge_delegate_ash.h"
#include "ash/wm/native_cursor_manager_ash.h"
#include "ash/wm/overlay_event_filter.h"
#include "ash/wm/overview/overview_controller.h"
Expand Down Expand Up @@ -221,6 +221,7 @@
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/dbus/init/initialize_dbus_client.h"
#include "chromeos/dbus/power/power_policy_controller.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"
#include "chromeos/ui/wm/features.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
Expand Down Expand Up @@ -1260,11 +1261,6 @@ void Shell::Init(
focus_controller_ = std::make_unique<::wm::FocusController>(focus_rules_);
focus_controller_->AddObserver(this);

// Needs to be constructed after `focus_controller_` as it adds itself as an
// observer on construction.
multitask_menu_nudge_controller_ =
std::make_unique<MultitaskMenuNudgeController>();

overview_controller_ = std::make_unique<OverviewController>();

screen_position_controller_ = std::make_unique<ScreenPositionController>();
Expand Down Expand Up @@ -1534,10 +1530,19 @@ void Shell::Init(
// WindowTreeHostManager to host the keyboard window.
keyboard_controller_->CreateVirtualKeyboard(std::move(keyboard_ui_factory));

// Create window restore controller after WindowTreeHostManager::InitHosts()
// Create window restore controller after `WindowTreeHostManager::InitHosts()`
// since it may need to add observers to root windows.
window_restore_controller_ = std::make_unique<WindowRestoreController>();

// Needs to be constructed after `WindowTreeHostManager::InitHosts()` as it
// needs to get the activation client from chromeos/.
if (chromeos::wm::features::IsWindowLayoutMenuEnabled()) {
multitask_menu_nudge_controller_ =
std::make_unique<chromeos::MultitaskMenuNudgeController>(
GetPrimaryRootWindow(),
std::make_unique<MultitaskMenuNudgeDelegateAsh>());
}

static_cast<CursorManager*>(cursor_manager_.get())->Init();

mojo::PendingRemote<device::mojom::Fingerprint> fingerprint;
Expand Down
6 changes: 3 additions & 3 deletions ash/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class Window;

namespace chromeos {
class ImmersiveContext;
class MultitaskMenuNudgeController;
class SnapController;
} // namespace chromeos

Expand Down Expand Up @@ -171,7 +172,6 @@ class MouseCursorEventFilter;
class MruWindowTracker;
class MultiDeviceNotificationPresenter;
class MultiDisplayMetricsController;
class MultitaskMenuNudgeController;
class NearbyShareControllerImpl;
class NearbyShareDelegate;
class NightLightControllerImpl;
Expand Down Expand Up @@ -577,7 +577,7 @@ class ASH_EXPORT Shell : public SessionObserver,
MultiDisplayMetricsController* multi_display_metrics_controller() {
return multi_display_metrics_controller_.get();
}
MultitaskMenuNudgeController* multitask_menu_nudge_controller() {
chromeos::MultitaskMenuNudgeController* multitask_menu_nudge_controller() {
return multitask_menu_nudge_controller_.get();
}
NearbyShareControllerImpl* nearby_share_controller() {
Expand Down Expand Up @@ -944,7 +944,7 @@ class ASH_EXPORT Shell : public SessionObserver,
multi_display_metrics_controller_;
std::unique_ptr<MultiDeviceNotificationPresenter>
multidevice_notification_presenter_;
std::unique_ptr<MultitaskMenuNudgeController>
std::unique_ptr<chromeos::MultitaskMenuNudgeController>
multitask_menu_nudge_controller_;
std::unique_ptr<NearbyShareControllerImpl> nearby_share_controller_;
std::unique_ptr<NearbyShareDelegate> nearby_share_delegate_;
Expand Down
4 changes: 2 additions & 2 deletions ash/test/ash_test_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
#include "ash/wallpaper/test_wallpaper_controller_client.h"
#include "ash/wallpaper/wallpaper_controller_impl.h"
#include "ash/wm/desks/templates/saved_desk_test_helper.h"
#include "ash/wm/multitask_menu_nudge_controller.h"
#include "ash/wm/overview/overview_controller.h"
#include "ash/wm/tablet_mode/tablet_mode_controller.h"
#include "base/run_loop.h"
Expand All @@ -45,6 +44,7 @@
#include "chromeos/ash/components/dbus/rgbkbd/rgbkbd_client.h"
#include "chromeos/ash/components/login/login_state/login_state.h"
#include "chromeos/dbus/power/power_policy_controller.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"
#include "device/bluetooth/bluetooth_adapter_factory.h"
#include "device/bluetooth/dbus/bluez_dbus_manager.h"
#include "device/bluetooth/floss/floss_dbus_manager.h"
Expand Down Expand Up @@ -328,7 +328,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
// the nudge first before dismissing the launcher.
shell->dark_light_mode_controller()->SetShowNudgeForTesting(false);

MultitaskMenuNudgeController::SetSuppressNudgeForTesting(true);
chromeos::MultitaskMenuNudgeController::SetSuppressNudgeForTesting(true);

// Set up a test wallpaper controller client before signing in any users. At
// the time a user logs in, Wallpaper controller relies on
Expand Down
11 changes: 6 additions & 5 deletions ash/wm/multitask_menu_nudge_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "ash/wm/multitask_menu_nudge_controller.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"

#include "ash/constants/ash_features.h"
#include "ash/display/display_move_window_util.h"
Expand Down Expand Up @@ -50,7 +50,7 @@ class MultitaskMenuNudgeControllerTest : public AshTestBase {

AshTestBase::SetUp();

MultitaskMenuNudgeController::SetSuppressNudgeForTesting(false);
chromeos::MultitaskMenuNudgeController::SetSuppressNudgeForTesting(false);
controller_ = Shell::Get()->multitask_menu_nudge_controller();
controller_->SetOverrideClockForTesting(&test_clock_);

Expand All @@ -74,14 +74,15 @@ class MultitaskMenuNudgeControllerTest : public AshTestBase {
const gfx::Rect expected_bounds(
(window_screen_bounds.width() - size.width()) / 2 +
window_screen_bounds.x(),
MultitaskMenuNudgeController::kTabletNudgeYOffset +
chromeos::MultitaskMenuNudgeController::kTabletNudgeYOffset +
window_screen_bounds.y(),
size.width(), size.height());
EXPECT_EQ(expected_bounds, GetWidget()->GetWindowBoundsInScreen());
}

private:
MultitaskMenuNudgeController* controller_;
chromeos::MultitaskMenuNudgeController* controller_;

base::test::ScopedFeatureList scoped_feature_list_;
};

Expand Down Expand Up @@ -263,4 +264,4 @@ TEST_F(MultitaskMenuNudgeControllerTest, TabletNudgeBounds) {
ExpectCorrectTabletNudgeBounds(window.get());
}

} // namespace ash
} // namespace ash
64 changes: 64 additions & 0 deletions ash/wm/multitask_menu_nudge_delegate_ash.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "ash/wm/multitask_menu_nudge_delegate_ash.h"

#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "components/prefs/pref_service.h"

namespace ash {

namespace {

PrefService* GetPrefService() {
return Shell::Get()->session_controller()->GetActivePrefService();
}

std::string GetShowCountPrefName(bool tablet_mode) {
return tablet_mode
? chromeos::MultitaskMenuNudgeController::kTabletShownCountPrefName
: chromeos::MultitaskMenuNudgeController::
kClamshellShownCountPrefName;
}

std::string GetLastShownPrefName(bool tablet_mode) {
return tablet_mode
? chromeos::MultitaskMenuNudgeController::kTabletLastShownPrefName
: chromeos::MultitaskMenuNudgeController::
kClamshellLastShownPrefName;
}

} // namespace

MultitaskMenuNudgeDelegateAsh::MultitaskMenuNudgeDelegateAsh() = default;

MultitaskMenuNudgeDelegateAsh::~MultitaskMenuNudgeDelegateAsh() = default;

bool MultitaskMenuNudgeDelegateAsh::IsRegularUser() const {
auto* session_controller = Shell::Get()->session_controller();
const absl::optional<user_manager::UserType> user_type =
session_controller->GetUserType();
return user_type == user_manager::USER_TYPE_REGULAR;
}

int MultitaskMenuNudgeDelegateAsh::GetShowCount(bool tablet_mode) const {
return GetPrefService()->GetInteger(GetShowCountPrefName(tablet_mode));
}

void MultitaskMenuNudgeDelegateAsh::SetShowCount(int count, bool tablet_mode) {
GetPrefService()->SetInteger(GetShowCountPrefName(tablet_mode), count);
}

base::Time MultitaskMenuNudgeDelegateAsh::GetLastShownTime(
bool tablet_mode) const {
return GetPrefService()->GetTime(GetLastShownPrefName(tablet_mode));
}

void MultitaskMenuNudgeDelegateAsh::SetLastShownTime(base::Time time,
bool tablet_mode) {
GetPrefService()->SetTime(GetLastShownPrefName(tablet_mode), time);
}

} // namespace ash
33 changes: 33 additions & 0 deletions ash/wm/multitask_menu_nudge_delegate_ash.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef ASH_WM_MULTITASK_MENU_NUDGE_DELEGATE_ASH_
#define ASH_WM_MULTITASK_MENU_NUDGE_DELEGATE_ASH_

#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"

namespace ash {

// Ash implementation of nudge controller delegate that lets us get and set pref
// values from the ash active profile.
class MultitaskMenuNudgeDelegateAsh
: public chromeos::MultitaskMenuNudgeController::Delegate {
public:
MultitaskMenuNudgeDelegateAsh();
MultitaskMenuNudgeDelegateAsh(const MultitaskMenuNudgeDelegateAsh&) = delete;
MultitaskMenuNudgeDelegateAsh& operator=(
const MultitaskMenuNudgeDelegateAsh&) = delete;
~MultitaskMenuNudgeDelegateAsh() override;

// chromeos::MultitaskMenuNudgeController::Delegate:
bool IsRegularUser() const override;
int GetShowCount(bool tablet_mode) const override;
void SetShowCount(int count, bool tablet_mode) override;
base::Time GetLastShownTime(bool tablet_mode) const override;
void SetLastShownTime(base::Time time, bool tablet_mode) override;
};

} // namespace ash

#endif // ASH_WM_MULTITASK_MENU_NUDGE_DELEGATE_ASH_
6 changes: 4 additions & 2 deletions ash/wm/tablet_mode/tablet_mode_multitask_cue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

#include "ash/constants/app_types.h"
#include "ash/shell.h"
#include "ash/wm/multitask_menu_nudge_controller.h"
#include "ash/wm/window_state.h"
#include "ash/wm/window_util.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"
#include "chromeos/ui/wm/features.h"
#include "ui/aura/client/aura_constants.h"
#include "ui/views/animation/animation_builder.h"
Expand Down Expand Up @@ -101,6 +101,7 @@ void TabletModeMultitaskCue::MaybeShowCue(aura::Window* active_window) {
&TabletModeMultitaskCue::OnTimerFinished);

// Show the education nudge a maximum of three times with 24h in between.
DCHECK(Shell::Get()->multitask_menu_nudge_controller());
Shell::Get()->multitask_menu_nudge_controller()->MaybeShowNudge(window_);
}

Expand All @@ -116,6 +117,7 @@ void TabletModeMultitaskCue::DismissCue() {
cue_layer_.reset();

// The education nudge should not appear without the cue.
DCHECK(Shell::Get()->multitask_menu_nudge_controller());
Shell::Get()->multitask_menu_nudge_controller()->DismissNudge();
}

Expand Down Expand Up @@ -175,4 +177,4 @@ void TabletModeMultitaskCue::OnTimerFinished() {
.SetOpacity(cue_layer_.get(), 0.0f, gfx::Tween::LINEAR);
}

} // namespace ash
} // namespace ash
4 changes: 2 additions & 2 deletions chrome/browser/ui/ash/desks/desks_client_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include "ash/wm/desks/templates/saved_desk_test_util.h"
#include "ash/wm/desks/templates/saved_desk_util.h"
#include "ash/wm/float/float_controller.h"
#include "ash/wm/multitask_menu_nudge_controller.h"
#include "ash/wm/overview/overview_controller.h"
#include "ash/wm/overview/overview_grid.h"
#include "ash/wm/overview/overview_session.h"
Expand Down Expand Up @@ -82,6 +81,7 @@
#include "chrome/test/base/ui_test_utils.h"
#include "chromeos/crosapi/mojom/desk.mojom-shared.h"
#include "chromeos/ui/base/window_state_type.h"
#include "chromeos/ui/frame/multitask_menu/multitask_menu_nudge_controller.h"
#include "chromeos/ui/wm/features.h"
#include "components/account_id/account_id.h"
#include "components/app_constants/constants.h"
Expand Down Expand Up @@ -470,7 +470,7 @@ class DesksClientTest : public extensions::PlatformAppBrowserTest {

// Suppress the multitask menu nudge as we'll be checking the stacking order
// and the count of the active desk children.
ash::MultitaskMenuNudgeController::SetSuppressNudgeForTesting(true);
chromeos::MultitaskMenuNudgeController::SetSuppressNudgeForTesting(true);
}
DesksClientTest(const DesksClientTest&) = delete;
DesksClientTest& operator=(const DesksClientTest&) = delete;
Expand Down
6 changes: 6 additions & 0 deletions chromeos/chromeos_strings.grd
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,12 @@
<message name="IDS_MULTITASK_MENU_EXIT_FLOAT_BUTTON_NAME" desc="Title and and accessible name of the float button on the multitask menu, when the window is in float state.">
Exit float
</message>
<message name="IDS_MULTITASK_MENU_NUDGE_TEXT" desc="Text that is shown when the clamshell multitask menu nudge is displayed.">
Keep hovering for more layout options
</message>
<message name="IDS_TABLET_MULTITASK_MENU_NUDGE_TEXT" desc="Text that is shown when the tablet multitask menu nudge is displayed.">
Swipe down for more layout options
</message>

<if expr="chromeos_ash">
<!-- The following strings are located here for accessibility from both //ash and //chrome -->
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
a5bdc20bd716abf26cfb91196d67c456dd053c03
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
99d2d3f1b4ad3485a0099968ab49372653482e10

0 comments on commit 2bb7e39

Please sign in to comment.