MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand.

chrome/browser/global_keyboard_shortcuts_mac_unittest.mm

Index: chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
diff --git a/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm b/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
index 25b261dd2763aeda54cf7aa793de68d4561e2bb1..9c2b0508f7a8678c43369a93e32dbd8b9dee6996 100644
--- a/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
+++ b/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
@@ -12,6 +12,31 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+// Common shortcut table that's shared between Cocoa and MacViews, so it's
+// possible that accelerator matching is working correctly in a portable manner.
+const std::vector<KeyboardShortcutData>& GetTestShortcutTable() {
+  CR_DEFINE_STATIC_LOCAL(std::vector<KeyboardShortcutData>, result, ({
+      // cmd   shift  cntrl  option
+      // ---   -----  -----  ------
+      {true, false, false, false, kVK_ANSI_1, 0, IDC_SELECT_TAB_0},
+      {true, false, false, false, kVK_ANSI_Keypad1, 0, IDC_SELECT_TAB_0},
+  }));
+  return result;
+}
+
+int CommandForTestShortcut(
+    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
+    int vkey_code, unichar key_char) {
+  return CommandForKeyboardShortcut(GetTestShortcutTable(),
+                                    command_key, shift_key,
+                                    cntrl_key, opt_key, vkey_code,
+                                    key_char);
+}
+
+}  // namespace
+
 TEST(GlobalKeyboardShortcuts, ShortcutsToWindowCommand) {
   // Test that an invalid shortcut translates into an invalid command id.
   EXPECT_EQ(
@@ -46,7 +71,7 @@ TEST(GlobalKeyboardShortcuts, ShortcutsToWindowCommand) {
 
   // Test that switching tabs triggers off keycodes and not characters (visible
   // with the Italian keyboard layout).
-  EXPECT_EQ(IDC_SELECT_TAB_0, CommandForWindowKeyboardShortcut(
+  EXPECT_EQ(IDC_SELECT_TAB_0, CommandForTestShortcut(

[tapted, 2016/12/02 07:26:09]

I poked around the code, and I think this expectation simply doesn't make sense
on MacViews. IDC_SELECT_TAB_0 is mapped via the accelerator_table.cc which
_only_ supports mapping via keycodes. It's only the curly braces above that
needed different treatment.

So I think all we need is a good comment here (e.g. paraphrase the above
paragraph), and

#if !BUILDFLAG(MAC_VIEWS_BROWSER)

around this EXPECT


You'll need #include "ui/base/ui_features.h"


then the other changes can be dropped.

Ideally we also point to where equivalent coverage of this exists via
accelerator_table, but the test would just be checking whether the accelerator
table contains
  { ui::VKEY_1, kPlatformModifier, IDC_SELECT_TAB_0 },
which it obviously does, so I don't think that's interesting.

[themblsha, 2016/12/02 17:20:38]

Woohoo! This BUILDFLAG will make dealing with failing test like this much
easier, thanks!

       true, false, false, false, kVK_ANSI_1, '&'));
 }