MacViews: Views accelerators table should match the Cocoa one.

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

 // Copyright 2014 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 "chrome/browser/ui/views/frame/browser_frame_mac.h"
 
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"
+#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"
 #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) {

[tapted, 2016/10/26 08:40:47]

comment?

[themblsha, 2016/10/26 17:10:03]

It's adapted from +[BrowserWindowUtils shouldHandleKeyboardEvent:] plus one
check from +handleKeyboardEvent:inWindow:, as I was trying to preserve the
original Cocoa behaviour. Guess I need to research it more.

[tapted, 2016/10/28 04:59:48]

Hm - it's possible that this and HandleExtraKeyboardShortcut should be absorbed
into ChromeCommandDispatcherDelegate..

But yeah - we still need to nail down the role that
ChromeCommandDispatcherDelegate is playing in the event sequence for a
BrowserFrameMac. Perhaps my earlier suggestion will clear things up a bit. But
currently it looks like `HandleExtraKeyboardShortcut` is being invoked one time
too many for BrowserFrameMac

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

Without my changes the only code path that should call
ChromeCommandDispatcherDelegate seems to be this one:

-[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:]
  * -[CommandDispatcher performKeyEquivalent:]
    * -[NativeWidgetMacNSWindow performKeyEquivalent:]

The key equivalent system will automatically consult the Menu in case the window
won't eat the event.

+  // Do not fire shortcuts on key up.
+  if ([event.os_event type] != NSKeyDown)
+    return false;
+
+  if (event.skip_in_browser ||
+      event.type == content::NativeWebKeyboardEvent::Char)
+    return false;
+
+  DCHECK(event.os_event != NULL);

[tapted, 2016/10/26 08:40:47]

DCHECK(event.os_event) - this should probably be at the stat of the method

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

I guess it could fail in case the event was simulated (using DevTools or some
other means), so I'd prefer to leave it in its current position.

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

Ah, good point - I think I've encountered strangeness about this too, in bug
triage: https://bugs.chromium.org/p/chromium/issues/detail?id=627221#c2 

However, the code is still weird. If event.os_event is null then the code on
line 24 will behave unexpectedly

`[null type] != NSKeyDown` turns into `0 != NSKeyDown` which is always true. So
it's impossible for this DCHECK to fail, even if event.os_event is null.

So we should have the skip_in_browser check first (let's check ::Char here too,
since the "Ignore synthesized events" comment applies to both), then the DCHECK,
then the NSKeyDown check.

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

Makes sense, done.

+  return true;
+}
+
+// Copied from chrome_command_dispatcher_delegate.mm's
+// HandleExtraBrowserKeyboardShortcut and adapted for MacViews to only consult
+// the CommandForMacViewsKeyboardShortcut table.
+bool HandleExtraKeyboardShortcut(NSEvent* event, Browser* browser) {
+  // 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;
+
+  return command_dispatcher::HandleExtraKeyboardShortcut(
+      event, browser->window()->GetNativeWindow(),
+      CommandForMacViewsKeyboardShortcut);
+}
+
+}  // namespace
+
 BrowserFrameMac::BrowserFrameMac(BrowserFrame* browser_frame,
                                  BrowserView* browser_view)
     : views::NativeWidgetMac(browser_frame),
       browser_view_(browser_view),
       command_dispatcher_delegate_(
           [[ChromeCommandDispatcherDelegate alloc] init]) {}
 
 BrowserFrameMac::~BrowserFrameMac() {
 }
 
@@ skipping 36 matching lines @@
           initWithContentRect:ui::kWindowSizeDeterminedLater
                     styleMask:style_mask
                       backing:NSBackingStoreBuffered
                         defer:NO]);
   [ns_window setCommandDispatcherDelegate:command_dispatcher_delegate_];
   [ns_window setCommandHandler:[[[BrowserWindowCommandHandler alloc] init]
                                    autorelease]];
   return ns_window.autorelease();
 }
 
+int BrowserFrameMac::GetMinimizeButtonOffset() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserFrameMac, NativeBrowserFrame implementation:
 
 views::Widget::InitParams BrowserFrameMac::GetWidgetParams() {
   views::Widget::InitParams params;
   params.native_widget = this;
   return params;
 }
 
 bool BrowserFrameMac::UseCustomFrame() const {
   return false;
 }
 
 bool BrowserFrameMac::UsesNativeSystemMenu() const {
   return true;
 }
 
 bool BrowserFrameMac::ShouldSaveWindowPlacement() const {
   return true;
 }
 
 void BrowserFrameMac::GetWindowPlacement(
     gfx::Rect* bounds,
     ui::WindowShowState* show_state) const {
   return NativeWidgetMac::GetWindowPlacement(bounds, show_state);
 }
 
-int BrowserFrameMac::GetMinimizeButtonOffset() const {
-  NOTIMPLEMENTED();
-  return 0;
+bool BrowserFrameMac::PreHandleKeyboardEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  // Copied from BrowserWindowCocoa::PreHandleKeyboardEvent().

[tapted, 2016/10/26 08:40:47]

BrowserWindowCocoa may not be with us forever, so this comment needs to be
standalone. We need to capture "why Mac is special" here. i.e. why does
BrowserFrameMac need special handling for weirdness that the Mac platform throws
at us and other platforms don't. 

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

Tried my best to fill the code with sensible comments :-)

+  if (!ShouldHandleKeyboardEvent(event))
+    return false;
+
+  // CommandForKeyEventOnMacViews consults the [NSApp mainMenu] and
+  // CommandForMacViewsKeyboardShortcut accelerator table internally.
+  int command_id = CommandForKeyEventOnMacViews(event.os_event);
+  if (command_id == -1)
+    return false;
+
+  Browser* browser = browser_view_->browser();
+  if (!browser->command_controller()->IsReservedCommandOrKey(
+          command_id, event))
+    return false;
+
+  return HandleExtraKeyboardShortcut(event.os_event, browser);
 }
+
+bool BrowserFrameMac::HandleKeyboardEvent(
+    const content::NativeWebKeyboardEvent& event) {
+  if (!ShouldHandleKeyboardEvent(event))
+    return false;
+
+  return HandleExtraKeyboardShortcut(event.os_event, browser_view_->browser());
+}