Skip to content

Commit

Permalink
Converts ui::Accelerator::type to an enum
Browse files Browse the repository at this point in the history 
Accelerator::type_ was an EventType, but this is overkill as the type
can only be pressed or released. So, this converts the type to an enum
with those two values.

I'm doing this as I'm going to write a mojom for Accelerator and it's
better to restrict the mojom to the actual values.

BUG=701815
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2751323002
Cr-Commit-Position: refs/heads/master@{#457492}
  • Loading branch information
sky authored and Commit bot committed Mar 16, 2017
1 parent 363735b commit d56e56c
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 97 deletions.
53 changes: 25 additions & 28 deletions ash/accelerators/accelerator_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,12 @@ class TestTarget : public ui::AcceleratorTarget {
DISALLOW_COPY_AND_ASSIGN(TestTarget);
};

class ReleaseAccelerator : public ui::Accelerator {
public:
ReleaseAccelerator(ui::KeyboardCode keycode, int modifiers)
: ui::Accelerator(keycode, modifiers) {
set_type(ui::ET_KEY_RELEASED);
}
};
ui::Accelerator CreateReleaseAccelerator(ui::KeyboardCode key_code,
int modifiers) {
ui::Accelerator accelerator(key_code, modifiers);
accelerator.set_key_state(ui::Accelerator::KeyState::RELEASED);
return accelerator;
}

class DummyBrightnessControlDelegate : public BrightnessControlDelegate {
public:
Expand Down Expand Up @@ -216,12 +215,12 @@ class AcceleratorControllerTest : public test::AshTestBase {
static AcceleratorController* GetController();

static bool ProcessInController(const ui::Accelerator& accelerator) {
if (accelerator.type() == ui::ET_KEY_RELEASED) {
if (accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
// If the |accelerator| should trigger on release, then we store the
// pressed version of it first in history then the released one to
// simulate what happens in reality.
ui::Accelerator pressed_accelerator = accelerator;
pressed_accelerator.set_type(ui::ET_KEY_PRESSED);
pressed_accelerator.set_key_state(ui::Accelerator::KeyState::PRESSED);
GetController()->accelerator_history()->StoreCurrentAccelerator(
pressed_accelerator);
}
Expand Down Expand Up @@ -286,7 +285,7 @@ AcceleratorController* AcceleratorControllerTest::GetController() {
TEST_F(AcceleratorControllerTest, ExitWarningHandlerTestDoublePress) {
ui::Accelerator press(ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN);
ui::Accelerator release(press);
release.set_type(ui::ET_KEY_RELEASED);
release.set_key_state(ui::Accelerator::KeyState::RELEASED);
ExitWarningHandler* ewh = GetController()->GetExitWarningHandlerForTest();
ASSERT_TRUE(ewh);
StubForTest(ewh);
Expand All @@ -308,7 +307,7 @@ TEST_F(AcceleratorControllerTest, ExitWarningHandlerTestDoublePress) {
TEST_F(AcceleratorControllerTest, ExitWarningHandlerTestSinglePress) {
ui::Accelerator press(ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN);
ui::Accelerator release(press);
release.set_type(ui::ET_KEY_RELEASED);
release.set_key_state(ui::Accelerator::KeyState::RELEASED);
ExitWarningHandler* ewh = GetController()->GetExitWarningHandlerForTest();
ASSERT_TRUE(ewh);
StubForTest(ewh);
Expand Down Expand Up @@ -747,11 +746,9 @@ TEST_F(EnabledDockedWindowsAcceleratorControllerTest, CenterWindowAccelerator) {

TEST_F(AcceleratorControllerTest, AutoRepeat) {
ui::Accelerator accelerator_a(ui::VKEY_A, ui::EF_CONTROL_DOWN);
accelerator_a.set_type(ui::ET_KEY_PRESSED);
TestTarget target_a;
GetController()->Register({accelerator_a}, &target_a);
ui::Accelerator accelerator_b(ui::VKEY_B, ui::EF_CONTROL_DOWN);
accelerator_b.set_type(ui::ET_KEY_PRESSED);
TestTarget target_b;
GetController()->Register({accelerator_b}, &target_b);

Expand Down Expand Up @@ -1032,8 +1029,8 @@ TEST_F(AcceleratorControllerTest, GlobalAcceleratorsToggleAppList) {
EXPECT_EQ(ui::VKEY_LWIN, GetCurrentAccelerator().key_code());
EXPECT_EQ(0u, test_app_list_presenter.toggle_count());

EXPECT_TRUE(
ProcessInController(ReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
EXPECT_TRUE(ProcessInController(
CreateReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
RunAllPendingInMessageLoop();
EXPECT_EQ(1u, test_app_list_presenter.toggle_count());
EXPECT_EQ(ui::VKEY_LWIN, GetPreviousAccelerator().key_code());
Expand All @@ -1042,17 +1039,17 @@ TEST_F(AcceleratorControllerTest, GlobalAcceleratorsToggleAppList) {
delegate->ToggleSpokenFeedback(A11Y_NOTIFICATION_NONE);
EXPECT_FALSE(
ProcessInController(ui::Accelerator(ui::VKEY_LWIN, ui::EF_NONE)));
EXPECT_FALSE(
ProcessInController(ReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
EXPECT_FALSE(ProcessInController(
CreateReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
delegate->ToggleSpokenFeedback(A11Y_NOTIFICATION_NONE);
RunAllPendingInMessageLoop();
EXPECT_EQ(1u, test_app_list_presenter.toggle_count());

// Turning off spoken feedback should allow the AppList to toggle again.
EXPECT_FALSE(
ProcessInController(ui::Accelerator(ui::VKEY_LWIN, ui::EF_NONE)));
EXPECT_TRUE(
ProcessInController(ReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
EXPECT_TRUE(ProcessInController(
CreateReleaseAccelerator(ui::VKEY_LWIN, ui::EF_NONE)));
RunAllPendingInMessageLoop();
EXPECT_EQ(2u, test_app_list_presenter.toggle_count());

Expand All @@ -1062,17 +1059,16 @@ TEST_F(AcceleratorControllerTest, GlobalAcceleratorsToggleAppList) {
RunAllPendingInMessageLoop();
EXPECT_EQ(3u, test_app_list_presenter.toggle_count());
EXPECT_FALSE(ProcessInController(
ReleaseAccelerator(ui::VKEY_BROWSER_SEARCH, ui::EF_NONE)));
CreateReleaseAccelerator(ui::VKEY_BROWSER_SEARCH, ui::EF_NONE)));
RunAllPendingInMessageLoop();
EXPECT_EQ(3u, test_app_list_presenter.toggle_count());
}

TEST_F(AcceleratorControllerTest, ImeGlobalAccelerators) {
// Test IME shortcuts.
ui::Accelerator control_space_down(ui::VKEY_SPACE, ui::EF_CONTROL_DOWN);
control_space_down.set_type(ui::ET_KEY_PRESSED);
ui::Accelerator control_space_up(ui::VKEY_SPACE, ui::EF_CONTROL_DOWN);
control_space_up.set_type(ui::ET_KEY_RELEASED);
control_space_up.set_key_state(ui::Accelerator::KeyState::RELEASED);
const ui::Accelerator convert(ui::VKEY_CONVERT, ui::EF_NONE);
const ui::Accelerator non_convert(ui::VKEY_NONCONVERT, ui::EF_NONE);
const ui::Accelerator wide_half_1(ui::VKEY_DBE_SBCSCHAR, ui::EF_NONE);
Expand Down Expand Up @@ -1203,8 +1199,8 @@ TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
EXPECT_FALSE(ProcessInController(press_alt_then_search));
// When you release Search before Alt, the key_code is still VKEY_LWIN and
// Alt is still the modifier.
const ReleaseAccelerator release_search_before_alt(ui::VKEY_LWIN,
ui::EF_ALT_DOWN);
const ui::Accelerator release_search_before_alt(
CreateReleaseAccelerator(ui::VKEY_LWIN, ui::EF_ALT_DOWN));
EXPECT_TRUE(ProcessInController(release_search_before_alt));
EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
Expand All @@ -1219,8 +1215,8 @@ TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {

// 3. Press Alt, Press Search, Release Alt, Release Search.
EXPECT_FALSE(ProcessInController(press_alt_then_search));
const ReleaseAccelerator release_alt_before_search(ui::VKEY_MENU,
ui::EF_COMMAND_DOWN);
const ui::Accelerator release_alt_before_search(
CreateReleaseAccelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN));
EXPECT_TRUE(ProcessInController(release_alt_before_search));
EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
Expand Down Expand Up @@ -1487,8 +1483,9 @@ class DeprecatedAcceleratorTester : public AcceleratorControllerTest {

ui::Accelerator CreateAccelerator(const AcceleratorData& data) const {
ui::Accelerator result(data.keycode, data.modifiers);
result.set_type(data.trigger_on_press ? ui::ET_KEY_PRESSED
: ui::ET_KEY_RELEASED);
result.set_key_state(data.trigger_on_press
? ui::Accelerator::KeyState::PRESSED
: ui::Accelerator::KeyState::RELEASED);
return result;
}

Expand Down
22 changes: 13 additions & 9 deletions ash/common/accelerators/accelerator_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,9 @@ ui::Accelerator CreateAccelerator(ui::KeyboardCode keycode,
int modifiers,
bool trigger_on_press) {
ui::Accelerator accelerator(keycode, modifiers);
accelerator.set_type(trigger_on_press ? ui::ET_KEY_PRESSED
: ui::ET_KEY_RELEASED);
accelerator.set_key_state(trigger_on_press
? ui::Accelerator::KeyState::PRESSED
: ui::Accelerator::KeyState::RELEASED);
return accelerator;
}

Expand Down Expand Up @@ -283,7 +284,7 @@ bool CanHandlePreviousIme(ImeControlDelegate* ime_control_delegate) {
void HandlePreviousIme(ImeControlDelegate* ime_control_delegate,
const ui::Accelerator& accelerator) {
base::RecordAction(UserMetricsAction("Accel_Previous_Ime"));
if (accelerator.type() == ui::ET_KEY_PRESSED)
if (accelerator.key_state() == ui::Accelerator::KeyState::PRESSED)
ime_control_delegate->HandlePreviousIme();
// Else: consume the Ctrl+Space ET_KEY_RELEASED event but do not do anything.
}
Expand Down Expand Up @@ -369,7 +370,8 @@ bool CanHandleToggleAppList(const ui::Accelerator& accelerator,
// If something else was pressed between the Search key (LWIN)
// being pressed and released, then ignore the release of the
// Search key.
if (previous_accelerator.type() != ui::ET_KEY_PRESSED ||
if (previous_accelerator.key_state() !=
ui::Accelerator::KeyState::PRESSED ||
previous_accelerator.key_code() != ui::VKEY_LWIN) {
return false;
}
Expand Down Expand Up @@ -466,7 +468,7 @@ void HandleCrosh() {

bool CanHandleDisableCapsLock(const ui::Accelerator& previous_accelerator) {
ui::KeyboardCode previous_key_code = previous_accelerator.key_code();
if (previous_accelerator.type() == ui::ET_KEY_RELEASED ||
if (previous_accelerator.key_state() == ui::Accelerator::KeyState::RELEASED ||
(previous_key_code != ui::VKEY_LSHIFT &&
previous_key_code != ui::VKEY_SHIFT &&
previous_key_code != ui::VKEY_RSHIFT)) {
Expand Down Expand Up @@ -554,11 +556,12 @@ bool CanHandleToggleCapsLock(const ui::Accelerator& accelerator,
// This shortcust is set to be trigger on release. Either the current
// accelerator is a Search release or Alt release.
if (accelerator.key_code() == ui::VKEY_LWIN &&
accelerator.type() == ui::ET_KEY_RELEASED) {
accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
// The previous must be either an Alt press or Search press:
// 1. Press Alt, Press Search, Release Search, Release Alt.
// 2. Press Search, Press Alt, Release Search, Release Alt.
if (previous_accelerator.type() == ui::ET_KEY_PRESSED &&
if (previous_accelerator.key_state() ==
ui::Accelerator::KeyState::PRESSED &&
(previous_accelerator.key_code() == ui::VKEY_LWIN ||
previous_accelerator.key_code() == ui::VKEY_MENU)) {
return ime && ime->GetImeKeyboard();
Expand All @@ -567,11 +570,12 @@ bool CanHandleToggleCapsLock(const ui::Accelerator& accelerator,

// Alt release.
if (accelerator.key_code() == ui::VKEY_MENU &&
accelerator.type() == ui::ET_KEY_RELEASED) {
accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
// The previous must be either an Alt press or Search press:
// 3. Press Alt, Press Search, Release Alt, Release Search.
// 4. Press Search, Press Alt, Release Alt, Release Search.
if (previous_accelerator.type() == ui::ET_KEY_PRESSED &&
if (previous_accelerator.key_state() ==
ui::Accelerator::KeyState::PRESSED &&
(previous_accelerator.key_code() == ui::VKEY_LWIN ||
previous_accelerator.key_code() == ui::VKEY_MENU)) {
return ime && ime->GetImeKeyboard();
Expand Down
4 changes: 1 addition & 3 deletions ash/mus/accelerators/accelerator_controller_registrar.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,8 @@ void AcceleratorControllerRegistrar::AddAcceleratorToVector(
accelerator.modifiers());
pre_event_matcher->accelerator_phase =
ui::mojom::AcceleratorPhase::PRE_TARGET;
DCHECK(accelerator.type() == ui::ET_KEY_PRESSED ||
accelerator.type() == ui::ET_KEY_RELEASED);
pre_event_matcher->type_matcher->type =
accelerator.type() == ui::ET_KEY_PRESSED
accelerator.key_state() == ui::Accelerator::KeyState::PRESSED
? ui::mojom::EventType::KEY_PRESSED
: ui::mojom::EventType::KEY_RELEASED;

Expand Down
1 change: 0 additions & 1 deletion chrome/browser/ui/ash/chrome_screenshot_grabber_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

#include "chrome/browser/ui/ash/chrome_screenshot_grabber.h"

#include "ash/common/accelerators/accelerator_controller.h"
#include "ash/shell.h"
#include "ash/test/ash_test_base.h"
#include "base/bind.h"
Expand Down
Loading

0 comments on commit d56e56c

Please sign in to comment.