MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/frame/browser_view.cc

 // Copyright 2012 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_view.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 54 matching lines @@
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 #include "chrome/browser/ui/views/download/download_in_progress_dialog_view.h"
 #include "chrome/browser/ui/views/download/download_shelf_view.h"
 #include "chrome/browser/ui/views/exclusive_access_bubble_views.h"
 #include "chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.h"
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "chrome/browser/ui/views/find_bar_host.h"
 #include "chrome/browser/ui/views/frame/browser_view_layout.h"
 #include "chrome/browser/ui/views/frame/browser_view_layout_delegate.h"
+#include "chrome/browser/ui/views/frame/browser_view_platform.h"
 #include "chrome/browser/ui/views/frame/contents_layout_manager.h"
 #include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
 #include "chrome/browser/ui/views/frame/top_container_view.h"
 #include "chrome/browser/ui/views/frame/web_contents_close_handler.h"
 #include "chrome/browser/ui/views/ime/ime_warning_bubble_view.h"
 #include "chrome/browser/ui/views/infobars/infobar_container_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/location_bar/location_icon_view.h"
 #include "chrome/browser/ui/views/location_bar/star_view.h"
 #include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
@@ skipping 1325 matching lines @@
   }
 
 #if defined(OS_CHROMEOS)
   if (chrome::IsAcceleratorDeprecated(accelerator)) {
     if (event.type == blink::WebInputEvent::RawKeyDown)
       *is_keyboard_shortcut = true;
     return false;
   }
 #endif  // defined(OS_CHROMEOS)
 
+  if (BrowserViewPlatform::PreHandleKeyboardEvent(event, browser()))

[tapted, 2016/10/17 07:02:47]

I'm not an owner here, but based on the surrounding code, it might be OK to put
this in #ifdef. That would allow an in-place comment like

#if defined(OS_MACOSX)
  // On Mac, users can configure their own shortcuts for commands in the
  // mainMenu via System Preferences (even while Chrome is running). So
  // first scan through system menus and possibly handle the event natively
  // so the menu bar flashes.
  ...
#endif

But! There's actually already a mechanism for adding platform-specific codepaths
using the |native_browser_frame_| member.

So I think PreHandleKeyboardEvent probably belongs in native_browser_frame.h
(and probably get a native_browser_frame.cc so that we can return false
everywhere but mac). But we'd need to defer to whatever an OWNER wants --
pkasting or sky

edit: based on later code - maybe NativeBrowserFrame would allow a lot more code
reuse here

[themblsha, 2016/10/20 16:41:05]

Thanks! Moved the code there.

+    return true;
+
   chrome::BrowserCommandController* controller = browser_->command_controller();
 
   // Here we need to retrieve the command id (if any) associated to the
   // keyboard event. Instead of looking up the command id in the
   // |accelerator_table_| by ourselves, we block the command execution of
   // the |browser_| object then send the keyboard event to the
   // |focus_manager| as if we are activating an accelerator key.
   // Then we can retrieve the command id from the |browser_| object.
   bool original_block_command_state = controller->block_command_execution();
   controller->SetBlockCommandExecution(true);
@@ skipping 17 matching lines @@
   } else if (processed) {
     // |accelerator| is a non-browser shortcut (e.g. F4-F10 on Ash). Report
     // that we handled it.
     return true;
   }
 
   return false;
 }
 
 void BrowserView::HandleKeyboardEvent(const NativeWebKeyboardEvent& event) {
+  if (BrowserViewPlatform::HandleKeyboardEvent(event, browser()))
+    return;
+
   unhandled_keyboard_event_handler_.HandleKeyboardEvent(event,

[tapted, 2016/10/17 07:02:47]

There is already a mac-specific UnhandledKeyboardEventHandler -- can we reuse
that here? Or comment why we need the separate HandleKeyboardEvent for Mac

[themblsha, 2016/10/20 16:41:05]

Tried to do that:
https://gist.github.com/mblsha/d3ddab85dfff543efe8d4b808ae203aa

But got unresolved external linker error due to the use of BrowserView, and
decided to leave this as is.

                                                         GetFocusManager());
 }
 
 // TODO(devint): http://b/issue?id=1117225 Cut, Copy, and Paste are always
 // enabled in the page menu regardless of whether the command will do
 // anything. When someone selects the menu item, we just act as if they hit
 // the keyboard shortcut for the command by sending the associated key press
 // to windows. The real fix to this bug is to disable the commands when they
 // won't do anything. We'll need something like an overall clipboard command
 // manager to do that.
@@ skipping 1187 matching lines @@
 }
 
 extensions::ActiveTabPermissionGranter*
 BrowserView::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents = GetActiveWebContents();
   if (!web_contents)
     return nullptr;
   return extensions::TabHelper::FromWebContents(web_contents)
       ->active_tab_permission_granter();
 }