MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/frame/browser_frame_mac.mm

Index: chrome/browser/ui/views/frame/browser_frame_mac.mm
diff --git a/chrome/browser/ui/views/frame/browser_frame_mac.mm b/chrome/browser/ui/views/frame/browser_frame_mac.mm
index 5e0242dd9be3f5e00a98808a45081af35210d0a5..bb4df14c4f84b1a448d815bc10bc308b63080585 100644
--- a/chrome/browser/ui/views/frame/browser_frame_mac.mm
+++ b/chrome/browser/ui/views/frame/browser_frame_mac.mm
@@ -4,14 +4,61 @@
 
 #include "chrome/browser/ui/views/frame/browser_frame_mac.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/browser_command_controller.h"
+#include "chrome/browser/ui/browser_commands.h"
 #import "chrome/browser/ui/cocoa/browser_window_command_handler.h"
 #import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h"
+#import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h"

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

remove dupe

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

Done.

 #include "chrome/browser/ui/views/frame/browser_frame.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #import "chrome/browser/ui/views/frame/native_widget_mac_frameless_nswindow.h"
 #include "components/web_modal/web_contents_modal_dialog_host.h"
+#include "content/public/browser/native_web_keyboard_event.h"
 #import "ui/base/cocoa/window_size_constants.h"
 
+namespace {
+
+bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) {
+  // Do not fire shortcuts on key up.
+  if ([event.os_event type] != NSKeyDown)
+    return false;
+
+  // |event.skip_in_browser| is true when it shouldn't be handled by the browser
+  // if it was ignored by the renderer. http://codereview.chromium.org/295003
+  // The comment about skip_in_browser in native_web_keyboard_event.h seems to
+  // be outdated.

[themblsha, 2016/10/28 17:32:23]

Added comment about the outdated comment. Will remove it in next update.

Should I file a bug about this?

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

I think it's still mostly correct -- backspace may still trigger a notice.
You're right that skip_in_browser is now used for other things, but I think it's
OK. 

+  if (event.skip_in_browser)
+    return false;
+
+  // Ignore syntesized keyboard events. http://codereview.chromium.org/460105

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

syntesized -> synthesized

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

Done.

+  if (event.type == content::NativeWebKeyboardEvent::Char)
+    return false;
+
+  DCHECK(event.os_event);
+  return true;
+}
+
+// Returns true if |event| was handled.
+bool HandleExtraKeyboardShortcut(NSEvent* event,
+                                 Browser* browser,
+                                 ChromeCommandDispatcherDelegate* delegate) {
+  // Send the event to the menu before sending it to the browser/window
+  // shortcut handling, so that if a user configures cmd-left to mean
+  // "previous tab", it takes precedence over the built-in "history back"
+  // binding.
+  if ([[NSApp mainMenu] performKeyEquivalent:event])
+    return true;
+
+  // Invoke ChromeCommandDispatcherDelegate for Mac-specific shortcuts that
+  // can't be handled by accelerator_table.cc.
+  return [delegate
+      handleExtraKeyboardShortcut:event
+                           window:browser->window()->GetNativeWindow()];
+}
+
+}  // namespace
+
 BrowserFrameMac::BrowserFrameMac(BrowserFrame* browser_frame,
                                  BrowserView* browser_view)
     : views::NativeWidgetMac(browser_frame),
@@ -68,6 +115,11 @@ NativeWidgetMacNSWindow* BrowserFrameMac::CreateNSWindow(
   return ns_window.autorelease();
 }
 
+int BrowserFrameMac::GetMinimizeButtonOffset() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserFrameMac, NativeBrowserFrame implementation:
 
@@ -95,7 +147,37 @@ void BrowserFrameMac::GetWindowPlacement(
   return NativeWidgetMac::GetWindowPlacement(bounds, show_state);
 }
 
-int BrowserFrameMac::GetMinimizeButtonOffset() const {
-  NOTIMPLEMENTED();
-  return 0;
+// Mac is special because the user could override the menu shortcuts (see
+// comment in HandleExtraKeyboardShortcut), and there's a set of additional
+// accelerator tables (handled by ChromeCommandDispatcherDelegate) that couldn't
+// be ported to accelerator_table.cc: see global_keyboard_shortcuts_views_mac.mm
+bool BrowserFrameMac::PreHandleKeyboardEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  if (!ShouldHandleKeyboardEvent(event))
+    return false;
+
+  // CommandForKeyEventOnMacViews consults the [NSApp mainMenu] and Mac-specific

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

update comment (CommandForKeyEvent)

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

Done.

+  // accelerator table internally.
+  int command_id = CommandForKeyEvent(event.os_event);
+  if (command_id == -1)
+    return false;
+
+  // Only handle a small list of reserved commands that we don't want to be
+  // handled by the renderer.
+  Browser* browser = browser_view_->browser();
+  if (!browser->command_controller()->IsReservedCommandOrKey(
+          command_id, event))
+    return false;
+
+  return HandleExtraKeyboardShortcut(event.os_event, browser,
+                                     command_dispatcher_delegate_);
+}
+
+bool BrowserFrameMac::HandleKeyboardEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  if (!ShouldHandleKeyboardEvent(event))
+    return false;
+
+  return HandleExtraKeyboardShortcut(event.os_event, browser_view_->browser(),
+                                     command_dispatcher_delegate_);
 }