Skip to content

Commit

Permalink
Revert "wm: Add display settings shortcut in context menu."
Browse files Browse the repository at this point in the history
This reverts commit a84e8b6.

Reason for revert: UI review thumbs down this change. We
sent a rebuttal, but i will first revert before branch. If
we get permission then its ok to punt to M86.

Original change's description:
> wm: Add display settings shortcut in context menu.
> 
> Details/mocks are linked in the linked bug.
> 
> Test: manual
> Bug: 1095140
> Change-Id: Ic8dc25d686355b80dc5f537eda67836e6c97f172
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225978
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#778590}

TBR=xiyuan@chromium.org,sammiequon@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1095140
Change-Id: I1d065a7d798e73a6151282d58d0843101d019ef4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2268120
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782717}
  • Loading branch information
Sammie Quon authored and Commit Bot committed Jun 25, 2020
1 parent 4d1393f commit 7d72f89
Show file tree
Hide file tree
Showing 11 changed files with 14 additions and 74 deletions.
3 changes: 0 additions & 3 deletions ash/ash_strings.grd
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,6 @@ This file contains the strings for ash.
<message name="IDS_ASH_SHELF_CONTEXT_MENU_ALIGN_RIGHT" desc="Title of the menu item in the context menu for aligning the shelf to the right of the screen">
Right
</message>
<message name="IDS_ASH_SHELF_DISPLAY_SETTINGS" desc="Title of the menu item in the context menu for opening the display settings">
Display settings
</message>
<message name="IDS_ASH_SHELF_ACCESSIBLE_NAME" desc="The accessible name of the shelf.">
Shelf
</message>
Expand Down

This file was deleted.

1 change: 0 additions & 1 deletion ash/resources/vector_icons/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ aggregate_vector_icons("ash_vector_icons") {
"dictation_off_newui.icon",
"dictation_on.icon",
"dictation_on_newui.icon",
"display_settings.icon",
"dogfood.icon",
"hollow_check_circle.icon",
"ime_menu_emoticon.icon",
Expand Down
25 changes: 0 additions & 25 deletions ash/resources/vector_icons/display_settings.icon

This file was deleted.

8 changes: 0 additions & 8 deletions ash/shelf/shelf_context_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "ash/root_window_controller.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "ash/shell_delegate.h"
#include "ash/strings/grit/ash_strings.h"
#include "ash/wallpaper/wallpaper_controller_impl.h"
#include "ash/wm/tablet_mode/tablet_mode_controller.h"
Expand Down Expand Up @@ -116,9 +115,6 @@ void ShelfContextMenuModel::ExecuteCommand(int command_id, int event_flags) {
case MENU_CHANGE_WALLPAPER:
shell->wallpaper_controller()->OpenWallpaperPickerIfAllowed();
break;
case MENU_DISPLAY_SETTINGS:
shell->shell_delegate()->OpenDisplaySettings();
break;
default:
if (delegate_) {
if (IsCommandIdAnAppLaunch(command_id)) {
Expand Down Expand Up @@ -175,10 +171,6 @@ void ShelfContextMenuModel::AddShelfAndWallpaperItems() {
ui::ImageModel::FromVectorIcon(kShelfPositionIcon));
}

AddItemWithStringIdAndIcon(
MENU_DISPLAY_SETTINGS, IDS_ASH_SHELF_DISPLAY_SETTINGS,
ui::ImageModel::FromVectorIcon(kDisplaySettingsIcon));

if (Shell::Get()->wallpaper_controller()->CanOpenWallpaperPicker()) {
AddItemWithStringIdAndIcon(MENU_CHANGE_WALLPAPER,
IDS_AURA_SET_DESKTOP_WALLPAPER,
Expand Down
1 change: 0 additions & 1 deletion ash/shelf/shelf_context_menu_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ class ASH_EXPORT ShelfContextMenuModel : public ui::SimpleMenuModel,
MENU_ALIGNMENT_RIGHT = 503,
MENU_ALIGNMENT_BOTTOM = 504,
MENU_CHANGE_WALLPAPER = 505,
MENU_DISPLAY_SETTINGS = 506,
MENU_ASH_END
};

Expand Down
37 changes: 14 additions & 23 deletions ash/shelf/shelf_context_menu_model_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,10 @@ class TestShelfItemDelegate : public ShelfItemDelegate {
TEST_F(ShelfContextMenuModelTest, Basic) {
ShelfContextMenuModel menu(nullptr, GetPrimaryDisplay().id());

ASSERT_EQ(4, menu.GetItemCount());
ASSERT_EQ(3, menu.GetItemCount());
EXPECT_EQ(CommandId::MENU_AUTO_HIDE, menu.GetCommandIdAt(0));
EXPECT_EQ(CommandId::MENU_ALIGNMENT_MENU, menu.GetCommandIdAt(1));
EXPECT_EQ(CommandId::MENU_DISPLAY_SETTINGS, menu.GetCommandIdAt(2));
EXPECT_EQ(CommandId::MENU_CHANGE_WALLPAPER, menu.GetCommandIdAt(3));
EXPECT_EQ(CommandId::MENU_CHANGE_WALLPAPER, menu.GetCommandIdAt(2));
for (int i = 0; i < menu.GetItemCount(); ++i) {
EXPECT_TRUE(menu.IsEnabledAt(i));
EXPECT_TRUE(menu.IsVisibleAt(i));
Expand Down Expand Up @@ -144,7 +143,7 @@ TEST_F(ShelfContextMenuModelTest, Invocation) {
EXPECT_EQ(0u, client.open_count());

// Click the third option, wallpaper picker. It should open.
menu3.ActivatedAt(3);
menu3.ActivatedAt(2);
EXPECT_EQ(1u, client.open_count());
}

Expand Down Expand Up @@ -201,22 +200,20 @@ TEST_F(ShelfContextMenuModelTest, ExcludeClamshellOptionsOnTabletMode) {
Shell::Get()->tablet_mode_controller();
int64_t primary_id = GetPrimaryDisplay().id();

// In tablet mode, shelf alignment is disabled.
// In tablet mode, the wallpaper picker and auto-hide should be the only two
// options because other options are disabled.
tablet_mode_controller->SetEnabledForTest(true);
ShelfContextMenuModel menu1(nullptr, primary_id);
EXPECT_EQ(3, menu1.GetItemCount());
EXPECT_EQ(2, menu1.GetItemCount());
EXPECT_EQ(ShelfContextMenuModel::MENU_AUTO_HIDE, menu1.GetCommandIdAt(0));
EXPECT_EQ(ShelfContextMenuModel::MENU_DISPLAY_SETTINGS,
menu1.GetCommandIdAt(1));
EXPECT_EQ(ShelfContextMenuModel::MENU_CHANGE_WALLPAPER,
menu1.GetCommandIdAt(2));
menu1.GetCommandIdAt(1));

// Test that a menu shown out of tablet mode includes all four options.
// MENU_AUTO_HIDE, MENU_ALIGNMENT_MENU, MENU_DISPLAY_SETTINGS and
// MENU_CHANGE_WALLPAPER.
// Test that a menu shown out of tablet mode includes all three options:
// MENU_AUTO_HIDE, MENU_ALIGNMENT_MENU, and MENU_CHANGE_WALLPAPER.
tablet_mode_controller->SetEnabledForTest(false);
ShelfContextMenuModel menu2(nullptr, primary_id);
EXPECT_EQ(4, menu2.GetItemCount());
EXPECT_EQ(3, menu2.GetItemCount());

// Test the auto hide option.
EXPECT_EQ(ShelfContextMenuModel::MENU_AUTO_HIDE, menu2.GetCommandIdAt(0));
Expand All @@ -241,15 +238,10 @@ TEST_F(ShelfContextMenuModelTest, ExcludeClamshellOptionsOnTabletMode) {
submenu->GetCommandIdAt(2));
EXPECT_TRUE(submenu->IsEnabledAt(2));

// Test the open display settings option.
EXPECT_EQ(ShelfContextMenuModel::MENU_DISPLAY_SETTINGS,
menu2.GetCommandIdAt(2));
EXPECT_TRUE(menu2.IsEnabledAt(2));

// Test the wallpaper picker option.
EXPECT_EQ(ShelfContextMenuModel::MENU_CHANGE_WALLPAPER,
menu2.GetCommandIdAt(3));
EXPECT_TRUE(menu2.IsEnabledAt(3));
menu2.GetCommandIdAt(2));
EXPECT_TRUE(menu2.IsEnabledAt(2));
}

TEST_F(ShelfContextMenuModelTest, CommandIdsMatchEnumsForHistograms) {
Expand All @@ -261,16 +253,15 @@ TEST_F(ShelfContextMenuModelTest, CommandIdsMatchEnumsForHistograms) {
EXPECT_EQ(503, ShelfContextMenuModel::MENU_ALIGNMENT_RIGHT);
EXPECT_EQ(504, ShelfContextMenuModel::MENU_ALIGNMENT_BOTTOM);
EXPECT_EQ(505, ShelfContextMenuModel::MENU_CHANGE_WALLPAPER);
EXPECT_EQ(506, ShelfContextMenuModel::MENU_DISPLAY_SETTINGS);
}

TEST_F(ShelfContextMenuModelTest, ShelfContextMenuOptions) {
// Tests that there are exactly 4 shelf context menu options. If you're adding
// Tests that there are exactly 3 shelf context menu options. If you're adding
// a context menu option ensure that you have added the enum to
// tools/metrics/enums.xml and that you haven't modified the order of the
// existing enums.
ShelfContextMenuModel menu(nullptr, GetPrimaryDisplay().id());
EXPECT_EQ(4, menu.GetItemCount());
EXPECT_EQ(3, menu.GetItemCount());
}

TEST_F(ShelfContextMenuModelTest, NotificationContainerEnabled) {
Expand Down
2 changes: 0 additions & 2 deletions ash/shell_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ class ASH_EXPORT ShellDelegate {
virtual media_session::mojom::MediaSessionService* GetMediaSessionService();

virtual void OpenKeyboardShortcutHelpPage() const {}

virtual void OpenDisplaySettings() const {}
};

} // namespace ash
Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/ui/ash/chrome_shell_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
#include "chrome/browser/ui/settings_window_manager_chromeos.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom.h"
#include "chrome/browser/ui/webui/tab_strip/tab_strip_ui_util.h"
#include "chromeos/services/multidevice_setup/multidevice_setup_service.h"
#include "content/public/browser/device_service.h"
Expand Down Expand Up @@ -195,9 +193,3 @@ ChromeShellDelegate::CreateBackGestureContextualNudgeDelegate(
ash::BackGestureContextualNudgeController* controller) {
return std::make_unique<BackGestureContextualNudgeDelegate>(controller);
}

void ChromeShellDelegate::OpenDisplaySettings() const {
chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
ProfileManager::GetActiveUserProfile(),
chromeos::settings::mojom::kDisplaySubpagePath);
}
1 change: 0 additions & 1 deletion chrome/browser/ui/ash/chrome_shell_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ class ChromeShellDelegate : public ash::ShellDelegate {
chromeos::multidevice_setup::mojom::MultiDeviceSetup> receiver)
override;
media_session::mojom::MediaSessionService* GetMediaSessionService() override;
void OpenDisplaySettings() const override;

private:
DISALLOW_COPY_AND_ASSIGN(ChromeShellDelegate);
Expand Down
1 change: 0 additions & 1 deletion tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9376,7 +9376,6 @@ histogram as enum -->
<int value="503" label="Shelf Alignment Right"/>
<int value="504" label="Shelf Alignment Bottom"/>
<int value="505" label="Change Wallpaper"/>
<int value="506" label="Display Settings"/>
</enum>

<enum name="ChromeOSUsbEventTiming">
Expand Down

0 comments on commit 7d72f89

Please sign in to comment.