Index: chrome/browser/ui/views/accelerator_table_unittest_mac.mm |
diff --git a/chrome/browser/ui/views/accelerator_table_unittest_mac.mm b/chrome/browser/ui/views/accelerator_table_unittest_mac.mm |
new file mode 100644 |
index 0000000000000000000000000000000000000000..09a3a02be6261e6ffb136eef6394ae3a4f9eacc7 |
--- /dev/null |
+++ b/chrome/browser/ui/views/accelerator_table_unittest_mac.mm |
@@ -0,0 +1,199 @@ |
+// Copyright 2016 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 <stddef.h> |
+ |
+#include <set> |
+ |
+#include "build/build_config.h" |
+#include "chrome/app/chrome_command_ids.h" |
+#import "chrome/browser/global_keyboard_shortcuts_mac.h" |
+#include "chrome/browser/ui/views/accelerator_table.h" |
+#include "testing/gtest/include/gtest/gtest.h" |
+#include "ui/events/event_constants.h" |
+#import "ui/events/keycodes/keyboard_code_conversion_mac.h" |
+ |
+namespace { |
tapted
2016/11/01 00:22:50
nit: nest this inside `namespace chrome {` and rem
Peter Kasting
2016/11/01 00:46:21
Note that we're trying to kill the chrome namespac
themblsha
2016/11/01 14:36:25
Is there a crbug for that? I don't feel that I com
Peter Kasting
2016/11/01 18:20:59
Right, you got it. Here's the bug: https://bugs.c
|
+ |
+// operator< inside anonymous namespace will cause compile-time errors. |
tapted
2016/11/01 00:22:50
Yeah - it needs to satisfy the requirements of Koe
Peter Kasting
2016/11/01 00:46:22
Can we supply a lambda to the std::set template?
|
+struct AcceleratorMappingComparator { |
+ bool operator()(const chrome::AcceleratorMapping& lhs, |
+ const chrome::AcceleratorMapping& rhs) { |
+ return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < |
+ std::tie(rhs.command_id, rhs.keycode, rhs.modifiers); |
+ } |
+}; |
+ |
+using AcceleratorMappingSet = |
+ std::set<chrome::AcceleratorMapping, AcceleratorMappingComparator>; |
+ |
+// If we try to initialize std::set(GetAcceleratorList().begin(), |
+// GetAcceleratorList().end()) we'll crash as we'll get two different copies of |
+// the same vector data. |
Peter Kasting
2016/11/01 00:46:22
Nit: I think you could omit this comment (and the
themblsha
2016/11/01 14:36:24
Did it using a lambda. But not sure what's the bes
Peter Kasting
2016/11/01 18:20:59
You mean, storing the comparator lambda in an auto
themblsha
2016/11/02 17:31:37
auto AcceleratorMappingComparator =
[](const
Peter Kasting
2016/11/02 17:51:28
It's pretty much a tossup for me. I might tweak a
|
+AcceleratorMappingSet FromVector( |
+ const std::vector<chrome::AcceleratorMapping>& v) { |
+ return AcceleratorMappingSet(v.begin(), v.end()); |
+} |
+ |
+} // namespace |
+ |
+namespace chrome { |
+ |
+// Vefifies that only the whitelisted accelerators could have Control key |
+// modifier, while running on macOS. |
+TEST(AcceleratorTableTest, CheckMacOSControlAccelerators) { |
+ // Only the accelerators that also work in Cocoa browser are allowed to appear |
+ // on this whitelist. |
+ const std::set<int> whitelisted_control_shortcuts = { |
+ IDC_SELECT_NEXT_TAB, |
+ IDC_SELECT_PREVIOUS_TAB, |
+ IDC_FULLSCREEN, |
+ }; |
+ |
+ const std::vector<AcceleratorMapping> accelerator_list(GetAcceleratorList()); |
+ |
+ // Control modifier is rarely used on Mac, and all valid uses must be |
+ // whitelisted. |
+ for (const auto& entry : accelerator_list) { |
+ if (whitelisted_control_shortcuts.count(entry.command_id)) |
+ continue; |
+ EXPECT_FALSE(entry.modifiers & ui::EF_CONTROL_DOWN) |
+ << "Found non-whitelisted accelerator that contains Control " |
+ "modifier: " << entry.command_id; |
+ } |
+ |
+ // Test that whitelist is not outdated. |
+ for (const auto& whitelist_entry : whitelisted_control_shortcuts) { |
+ const auto entry = |
+ std::find_if(accelerator_list.begin(), accelerator_list.end(), |
+ [whitelist_entry](const AcceleratorMapping& a) { |
+ return a.command_id == whitelist_entry && |
+ (a.modifiers & ui::EF_CONTROL_DOWN) != 0; |
+ }); |
+ EXPECT_TRUE(entry != accelerator_list.end()) |
Peter Kasting
2016/11/01 00:46:21
Nit: Can this use EXPECT_NE? If not, I suggest us
themblsha
2016/11/01 14:36:25
Looks much better, thanks!
|
+ << "Whitelisted accelerator not found in the actual list: " |
+ << whitelist_entry; |
+ } |
+} |
+ |
+// Verifies that Alt-only (or with just Shift) accelerators are not present in |
+// the list. |
+TEST(AcceleratorTableTest, CheckMacOSAltAccelerators) { |
+ const int kNonShiftMask = |
+ ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN; |
+ for (const auto& entry : GetAcceleratorList()) { |
+ EXPECT_FALSE((entry.modifiers & kNonShiftMask) == ui::EF_ALT_DOWN) |
+ << "Found accelerator that uses solely Alt modifier: " |
+ << entry.command_id; |
+ } |
+} |
+ |
+// Vefifies that accelerator filtering works correctly and we have an |
+// alternative shortcut for each filtered-out accelerator. |
+TEST(AcceleratorTableTest, CheckMacOSFilteredOutAcceleratorsHaveAlternatives) { |
+ // It's okay not to have an alternative accelerator for these commands as |
+ // there's no alternative for them in the Cocoa browser. |
+ const std::set<int> whitelisted_unique_shortcuts = { |
+ IDC_FOCUS_TOOLBAR, |
+ IDC_FOCUS_BOOKMARKS, |
+ IDC_FOCUS_INFOBARS, |
+ IDC_SHOW_APP_MENU, |
+ }; |
+ |
+ const AcceleratorMappingSet accelerator_list( |
Peter Kasting
2016/11/01 00:46:22
Nit: It seems kinda misleading to use "list" in th
themblsha
2016/11/01 14:36:25
Done. I thought that the _list suffix would be fin
|
+ FromVector(GetAcceleratorList())); |
+ const AcceleratorMappingSet full_accelerator_list( |
+ FromVector(GetUnfilteredAcceleratorListForTesting())); |
+ |
+ // Ensure that we still have alternative mappings for all filtered-out |
+ // accelerators. |
+ AcceleratorMappingSet filtered_list(full_accelerator_list); |
+ // Remove all accelerators that are present in |accelerator_list| |
tapted
2016/11/01 00:22:50
nit: fullstop at end
themblsha
2016/11/01 14:36:25
Done.
|
+ for (const auto& m : accelerator_list) { |
+ filtered_list.erase(m); |
tapted
2016/11/01 00:22:50
optional: there's an STL algorithm that will do th
Peter Kasting
2016/11/01 00:46:22
See base::STLSetDifference(), which wraps this in
themblsha
2016/11/01 14:36:25
That's great! There's one big gotcha though: std::
themblsha
2016/11/01 14:36:25
Yeah, I found it in the STL reference and thought
Peter Kasting
2016/11/01 18:20:59
It isn't? Looks allowed to me by the Google style
themblsha
2016/11/02 17:31:37
It says "if your type doesn't have a natural order
Peter Kasting
2016/11/02 17:51:28
Ah, I missed that! Thanks.
|
+ } |
+ EXPECT_GT(filtered_list.size(), 1u); |
+ |
+ for (const auto& removed_entry : filtered_list) { |
+ if (whitelisted_unique_shortcuts.count(removed_entry.command_id)) |
+ continue; |
+ |
+ const auto entry = |
+ std::find_if(accelerator_list.begin(), accelerator_list.end(), |
+ [&removed_entry](const AcceleratorMapping& m) { |
+ return removed_entry.command_id == m.command_id; |
+ }); |
+ EXPECT_TRUE(entry != accelerator_list.end()) |
+ << "Filtered command doesn't have an alternative mapping: " |
+ << removed_entry.command_id; |
+ } |
+ |
+ for (const auto& whitelist_entry : whitelisted_unique_shortcuts) { |
+ const auto entry = |
+ std::find_if(filtered_list.begin(), filtered_list.end(), |
+ [&whitelist_entry](const AcceleratorMapping& m) { |
+ return whitelist_entry == m.command_id; |
+ }); |
+ EXPECT_TRUE(entry != filtered_list.end()) |
+ << "Whitelisted accelerator not found in the filtered-out list: " |
+ << whitelist_entry; |
+ } |
+} |
+ |
+namespace { |
Peter Kasting
2016/11/01 00:46:22
Nit: I don't think there's any style rule on this,
themblsha
2016/11/01 14:36:25
Done.
|
+ |
+void VerifyTable( |
+ const KeyboardShortcutData* (*get_keyboard_shortcut_table)(size_t*), |
+ const std::string& table_name) { |
+ const std::vector<AcceleratorMapping> accelerator_list(GetAcceleratorList()); |
+ |
+ size_t num_shortcuts = 0; |
+ const KeyboardShortcutData *it = get_keyboard_shortcut_table(&num_shortcuts); |
+ for (size_t i = 0; i < num_shortcuts; ++i, ++it) { |
+ int modifiers = 0; |
+ if (it->command_key) |
+ modifiers |= ui::EF_COMMAND_DOWN; |
+ if (it->shift_key) |
+ modifiers |= ui::EF_SHIFT_DOWN; |
+ if (it->cntrl_key) |
+ modifiers |= ui::EF_CONTROL_DOWN; |
+ if (it->opt_key) |
+ modifiers |= ui::EF_ALT_DOWN; |
+ |
+ for (const auto& entry : accelerator_list) { |
+ unichar character; |
+ unichar shifted_character; |
+ const int vkey_code = ui::MacKeyCodeForWindowsKeyCode( |
+ entry.keycode, entry.modifiers, &shifted_character, &character); |
+ |
+ if (it->vkey_code) { |
+ if (modifiers == entry.modifiers && it->vkey_code == vkey_code && |
+ it->chrome_command == entry.command_id) { |
+ EXPECT_TRUE(false) << "Duplicate command: " << entry.command_id |
+ << " in table " << table_name; |
+ } |
+ } else { |
+ if (modifiers == entry.modifiers && |
+ (it->key_char == character || it->key_char == shifted_character) && |
+ it->chrome_command == entry.command_id) { |
+ EXPECT_TRUE(false) << "Duplicate command: " << entry.command_id |
+ << " in table " << table_name; |
+ } |
+ } |
Peter Kasting
2016/11/01 00:46:22
Nit: Not exactly simple, but simpler than this con
themblsha
2016/11/01 14:36:24
Thanks!
|
+ } |
+ } |
+} |
+ |
+} // namespace |
+ |
+// Verifies that we're not processing any duplicate accelerators in |
+// global_keyboard_shortcuts_mac.mm functions. |
+TEST(AcceleratorTableTest, CheckNoDuplicatesGlobalKeyboardShortcutsMac) { |
+ VerifyTable(GetWindowKeyboardShortcutTable, "WindowKeyboardShortcutTable"); |
+ VerifyTable(GetDelayedWindowKeyboardShortcutTable, |
+ "DelayedWindowKeyboardShortcutTable"); |
+ VerifyTable(GetBrowserKeyboardShortcutTable, "BrowserKeyboardShortcutTable"); |
+} |
+ |
+} // namespace chrome |