MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand.

chrome/browser/global_keyboard_shortcuts_mac_unittest.mm

 // Copyright (c) 2009 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 <AppKit/NSEvent.h>
 #include <Carbon/Carbon.h>
 #include <stddef.h>
 
 #include "chrome/browser/global_keyboard_shortcuts_mac.h"
 
 #include "base/macros.h"
 #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(
       -1, CommandForWindowKeyboardShortcut(false, false, false, false, 0, 0));
 
   // Check that all known keyboard shortcuts return valid results.
   for (const auto& shortcut : GetWindowKeyboardShortcutTable()) {
     int cmd_num = CommandForWindowKeyboardShortcut(
         shortcut.command_key, shortcut.shift_key, shortcut.cntrl_key,
         shortcut.opt_key, shortcut.vkey_code, shortcut.key_char);
@@ skipping 14 matching lines @@
       true, false, false, false, kVK_ANSI_Period, '}'));
   EXPECT_EQ(IDC_SELECT_PREVIOUS_TAB, CommandForWindowKeyboardShortcut(
       true, true, false, false, kVK_ANSI_Slash, '{'));
 
   // One more test for Cmd-'{' / Alt-8 (on German keyboard layout).
   EXPECT_EQ(IDC_SELECT_PREVIOUS_TAB, CommandForWindowKeyboardShortcut(
       true, false, false, true, kVK_ANSI_8, '{'));
 
   // 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, '&'));
 }
 
 TEST(GlobalKeyboardShortcuts, KeypadNumberKeysMatch) {
   // Test that the shortcuts that are generated by keypad number keys match the
   // equivalent keys.
   static const struct {
     int keycode;
     int keypad_keycode;
   } equivalents[] = {
@@ skipping 115 matching lines @@
   // cmd-shift-'[' in an RTL context pre 10.9.
   EXPECT_EQ('{', KeyCharacterForEvent(
       KeyEvent(true,  true,  false, false, @"{", @"}")));
   // cmd-shift-'[' in an RTL context on 10.9.
   EXPECT_EQ('{', KeyCharacterForEvent(
       KeyEvent(true,  true,  false, false, @"[", @"}")));
   // Test if getting dead-key events return 0 and do not hang.
   EXPECT_EQ(0,   KeyCharacterForEvent(
       KeyEvent(false, false, false, false, @"",  @"")));
 }