MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/accelerator_table_unittest_mac.mm

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..cb6c7730d3143149bd0d84a103c61ba30d257fd8
--- /dev/null
+++ b/chrome/browser/ui/views/accelerator_table_unittest_mac.mm
@@ -0,0 +1,182 @@
+// 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"
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"

[tapted, 2016/10/31 10:16:57]

nit: import

[themblsha, 2016/10/31 16:56:29]

Done.

+#include "chrome/browser/ui/views/accelerator_table.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/event_constants.h"
+#include "ui/events/keycodes/keyboard_code_conversion_mac.h"

[tapted, 2016/10/31 10:16:57]

nit: import

[themblsha, 2016/10/31 16:56:29]

Done.

+
+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;

[tapted, 2016/10/31 10:16:57]

nit: (a.modifiers & ui::EF_CONTROL_DOWN) != 0

[themblsha, 2016/10/31 16:56:29]

Done.

+        });
+    EXPECT_TRUE(entry != accelerator_list.end())
+        << "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 std::vector<AcceleratorMapping> accelerator_list(GetAcceleratorList());
+  const std::vector<AcceleratorMapping> full_accelerator_list(
+      GetUnfilteredAcceleratorListForTesting());
+
+  // Ensure that we still have alternative mappings for all filtered-out
+  // accelerators.
+  std::vector<AcceleratorMapping> filtered_list(full_accelerator_list);
+  auto remove_start =
+      std::remove_if(filtered_list.begin(), filtered_list.end(),
+                     [&accelerator_list](const AcceleratorMapping& m1) {
+        // Remove all accelerators that are present in |accelerator_list|
+        return std::find_if(accelerator_list.begin(), accelerator_list.end(),

[tapted, 2016/10/31 10:16:57]

This is a bit mind-blowing. I'd suggest declaring an operator< in the test file

bool operator<(const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) {
  return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < 
      std::tie(rhs.command_id, ..etc)
}

Then declare accelerator_list as a std::set and use `return
accelerator_list.count(m);` here.

[themblsha, 2016/10/31 16:56:29]

Whoa, std::tie makes comparators sooo easy. Replaced the vectors with sets so
the resulting operation became very easy.

+                            [&m1](const AcceleratorMapping& m2) {
+                 return m1.command_id == m2.command_id &&
+                        m1.keycode == m2.keycode &&
+                        m1.modifiers == m2.modifiers;
+               }) != accelerator_list.end();
+      });
+  filtered_list.erase(remove_start, filtered_list.end());
+  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 {
+
+static void VerifyTable(

[tapted, 2016/10/31 10:16:57]

nit: no `static`

[themblsha, 2016/10/31 16:56:29]

Done.

+    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;
+        }
+      }
+    }
+  }
+}
+
+}  // 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