MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/global_keyboard_shortcuts_views_mac.mm

Index: chrome/browser/global_keyboard_shortcuts_views_mac.mm
diff --git a/chrome/browser/global_keyboard_shortcuts_views_mac.mm b/chrome/browser/global_keyboard_shortcuts_views_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..35b62001f9fed62cffb7a12e5a8949f7440fc783
--- /dev/null
+++ b/chrome/browser/global_keyboard_shortcuts_views_mac.mm
@@ -0,0 +1,40 @@
+// 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 "base/macros.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"
+
+// Lists shortcuts that are impossible to migrate to accelerator_table.cc
+// (crbug.com/25946).
+
+const KeyboardShortcutData* GetWindowKeyboardShortcutTable(

[Peter Kasting, 2016/10/31 18:07:32]

Nit: Out of scope for this CL, but consider modifying the Mac code that calls
these sorts of functions to instead have them directly return
std::vector<KeyboardShortcutData>.  These can be constructed fairly simply (see
my accelerator table comments) and passed around efficiently in C++11, and then
iterated over at the other end using range-based for.

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

Not sure what you mean by C++11: move semantics? But it would imply that the
table would need to be efficiently created from something to be moved to
somewhere else?

Instead I created static vectors using CR_DEFINE_STATIC_LOCAL and pass them
around using const-references. Did I miss something?

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

I simply mean that the combination of things like move-constructors, initializer
lists, and the RVO lets you do something like this without paying much
efficiency cost:

std::vector<T> ConstructT() { return {{1, 2}, {3, 4}, ...}; }
std::vector<T> my_t = ConstructT();

...so I would ultimately move toward syntax like that for these functions
instead of passing around pointers and sizes.

CR_DEFINE_STATIC_LOCAL is possible too, it's just trickier to read and results
in these objects sticking around in memory.  I tend to try to avoid it if I can,
though that's based on more gut reaction than hard logic.

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

Will the std::vector just wrap the initializer list without costly copying of
data during each initialization?

I tried to test it using a small example, compiled it to LLVM IR and the
construction function does quite a lot of stuff before the first branch
instruction.

These table functions are called at least once per each accelerator processing
pass. Could be twice: PreHandle + Handle.

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

I see.  In that case you might indeed want to avoid directly returning a vector,
since I think we'll copy the data once during each init.

The CR_DEFINE_STATIC_LOCAL route would be one way to achieve this.

+    size_t* num_entries) {
+  static const KeyboardShortcutData keyboard_shortcuts[] = {
+      // cmd   shift  cntrl  option
+      // ---   -----  -----  ------
+      // '{' / '}' characters should be matched earlier than virtual key code
+      // (therefore we can match alt-8 as '{' on german keyboards).
+      {true, false, false, false, 0, '}', IDC_SELECT_NEXT_TAB},
+      {true, false, false, false, 0, '{', IDC_SELECT_PREVIOUS_TAB},
+  };
+
+  *num_entries = arraysize(keyboard_shortcuts);
+
+  return keyboard_shortcuts;
+}
+
+const KeyboardShortcutData* GetDelayedWindowKeyboardShortcutTable(
+    size_t* num_entries) {
+  static const KeyboardShortcutData keyboard_shortcuts[] = {};
+  *num_entries = 0;
+  return keyboard_shortcuts;
+}
+
+const KeyboardShortcutData* GetBrowserKeyboardShortcutTable(
+    size_t* num_entries) {
+  static const KeyboardShortcutData keyboard_shortcuts[] = {};
+  *num_entries = 0;
+  return keyboard_shortcuts;
+}