MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/accelerator_table_unittest_mac.mm

+// 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 remove the extra chrome::
prefixes

[Peter Kasting, 2016/11/01 00:46:21]

Note that we're trying to kill the chrome namespace entirely.  If it's easy,
feel free to remove the namespace scoping from AcceleratorMapping.  If not, then
either namespace everything in this file or qualify everything.

[themblsha, 2016/11/01 14:36:25]

Is there a crbug for that? I don't feel that I completely understand what that
implies: removing the `namespace chrome` in
src/chrome/browser/ui/views/accelerator_table.h, and fixing all the resulting
compilation errors on all platforms? This type is referenced only in seven
files, so it shouldn't be too hard.

Added `using namespace chrome;` here and removed the namespace from the test.

 

[Peter Kasting, 2016/11/01 18:20:59]

Right, you got it.  Here's the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=289619

Don't feel the need to do things like that in this CL unless they're easy and
you want to (and you're touching a lot of the files already).  Separate changes
like that can easily go in separate CLs too.

+
+// operator< inside anonymous namespace will cause compile-time errors.

[tapted, 2016/11/01 00:22:50]

Yeah - it needs to satisfy the requirements of Koenig Lookup, but I actually
like this better since it means we won't get conflicts if someone later decides
a public comparator might be useful. We can probably omit this comment.

[Peter Kasting, 2016/11/01 00:46:22]

Can we supply a lambda to the std::set template?  That would allow writing this
in a more condensed way.

+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 type alias) by just making
this a lambda in the relevant function below.  Then it seems more clear what
it's doing:

  // Convert the accelerator lists to sets for easy searching/filtering.
  auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) {
    return std::set<chrome::AcceleratorMapping, AcceleratorMappingComparator>(
        v.begin(), v.end());
  }
  auto accelerators = ToSet(GetAcceleratorList());
  auto unfiltered_accelerators =
      ToSet(GetUnfilteredAcceleratorListForTesting());

[themblsha, 2016/11/01 14:36:24]

Did it using a lambda. But not sure what's the best to forward-declare the
comparator lambda type, using a function pointer for now, as the alternative
that used decltype(lambda) was a little longer.

[Peter Kasting, 2016/11/01 18:20:59]

You mean, storing the comparator lambda in an auto locally, then after that
using decltype of that auto to define the set template?  I don't know the syntax
for that precisely offhand, so I'd have to see the actual code diff to compare
the readability.  The decltype route sounds nice in principle because it lets
you just change the lambda to whatever you want without worrying about also
updating a parallel comparator type, but the difference seems small.

[themblsha, 2016/11/02 17:31:37]

  auto AcceleratorMappingComparator =
      [](const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) {
    return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) <
           std::tie(rhs.command_id, rhs.keycode, rhs.modifiers);
  };
  auto ToSet = [&AcceleratorMappingComparator](
      const std::vector<AcceleratorMapping>& v) {
    return std::set<AcceleratorMapping, decltype(AcceleratorMappingComparator)>(
        v.begin(), v.end(), AcceleratorMappingComparator);
  };

I like the version I submitted better :-)

[Peter Kasting, 2016/11/02 17:51:28]

It's pretty much a tossup for me.  I might tweak a bit for brevity:

  auto Comparator =
      [](const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) {
    return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) <
           std::tie(rhs.command_id, rhs.keycode, rhs.modifiers);
  };
  auto ToSet = [&](const std::vector<AcceleratorMapping>& v) {
    return std::set<AcceleratorMapping, decltype(Comparator)>(
        v.begin(), v.end(), Comparator);
  };

...which to me makes it a slight win, but not a big deal either way.

+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 using EXPECT_FALSE(entry ==
filtered_list.end()) so that failure isn't mentally a double-negative like "if
it's _not_ true that the entry isn't the end, ..." (3 places)

[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 the name of a variable that's a
set.  I suggest just "accelerators". (2 places)

[themblsha, 2016/11/01 14:36:25]

Done. I thought that the _list suffix would be fine as it's a copy from
GetAcceleratorList() but it's good to have a second opinion :-)

+      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 this in linear time rather than
n*log(n), but it's a real mouthful.

std::set_difference(
    full_accelerator_list.begin(), full_accelerator_list.end(),
    accelerator_list.begin(), accelerator_list.end(),
    std::inserter(filtered_list, filtered_list.end()));

[Peter Kasting, 2016/11/01 00:46:22]

See base::STLSetDifference(), which wraps this in some syntactic sugar to give
you:

  auto filtered_list =
      base::STLSetDifference(full_accelerator_list, accelerator_list);

[themblsha, 2016/11/01 14:36:25]

That's great! There's one big gotcha though: std::set_difference needs to know
about the comparator, otherwise it won't compile. The easiest way to make it
work would be to add a global operator< for AcceleratorMapping, but I guess
that's not considered kosher.

Found base::ContainsKey() to be useful though.

[themblsha, 2016/11/01 14:36:25]

Yeah, I found it in the STL reference and thought that it was yucky and verbose,
and we'd need to pass the comparator as well.

[Peter Kasting, 2016/11/01 18:20:59]

It isn't?  Looks allowed to me by the Google style guide, with the requirement
that the behavior must be what you'd expect, and if you overload one comparison
operator you should overload them all.

On one hand that seems like overkill just to allow this function, but OTOH it
would eliminate the need for your lambda above, which would be nice.

[themblsha, 2016/11/02 17:31:37]

It says "if your type doesn't have a natural ordering, but you want to store it
in a std::set, use a custom comparator rather than overloading <."

So I guess the lambda comparator lives on.

[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, but it seems more common to
combine all anonymous namespace functions into one group atop the file rather
than a separate namespace above each applicable test.

[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 conditional:

      EXPECT_FALSE(modifiers == entry.modifiers &&
                   it->chrome_command == entry.command_id &&
                   (it->vkey_code ? (it->vkey_code == vkey_code)
                                  : (it->key_char == character ||
                                     it->key_char == shifted_character)))
          << "Duplicate command: " << entry.command_id
          << " in table " << table_name;

[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