Fix cmd+w closing the browser window instead of just the dictionary popover.

It's been said before and will be said again: keyboard event handling in
Chrome is a monster. Several things came together to make this happen:

First, in most Mac apps, cmd+w closes the dictionary popover if it's
visible. This specifically happens through the `performClose:` action
sent by the main menu. But, in Chrome, cmd+w closes the tab and not the
window, so cmd+w sends a different action. This meant that cmd+w
originally closed the tab and left the dictionary popover floating. This
was fixed many years ago (see r118131 and r322075) by making
`-[AppController menuNeedsUpdate:]` check the current target of
`performClose:` and only map cmd+w to "close tab" when the target is a
browser window.

This would work but for another layer of keyboard shortcut handling
trickery. For a number of reasons (see comment beginning "If this
keyEquivalent" added in r565490), Chrome doesn't actually let
`NSMenuItem`s send their own actions. Instead,
ChromeCommandDispatcherDelegate determines which menu item *would* have
been targeted by a given event, then handles the event itself.

The end result is that, when the dictionary popover is visible, the menu
items are successfully remapped so that cmd+w means "Close Window". But,
any actual cmd+w event is grabbed by ChromeCommandDispatcherDelegate and
sent directly to Chrome's own keyboard shortcut handling… so the browser
window closes.

This change adds a check that was proposed back in r322075, and makes
ChromeCommandDispatcherDelegate only grab the event when the found menu
item's action would be sent to its own window.

Bug: 947752
Change-Id: Ib32988d5fb69c79f899069feb02f7529ea05225f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849431
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705275}

chrome/browser/global_keyboard_shortcuts_mac.h

@@ -35,6 +35,9 @@ struct CommandForKeyEventResult {
   // The command to execute. -1 if none was found.
   int chrome_command;
 
+  // The action, if any, which the key event would trigger.
+  SEL action;
+
   // Whether the command was from a mapping in the main menu. Only relevant if
   // command != -1.
   bool from_main_menu;

chrome/browser/global_keyboard_shortcuts_mac.mm

@@ -59,7 +59,7 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) {
   return result;
 }
 
-int MenuCommandForKeyEvent(NSEvent* event) {
+int MenuCommandForKeyEvent(NSEvent* event, SEL* action) {
   if ([event type] != NSKeyDown)
     return -1;
 
@@ -78,8 +78,10 @@ int MenuCommandForKeyEvent(NSEvent* event) {
   if (!item)
     return -1;
 
-  if ([item action] == @selector(commandDispatch:) && [item tag] > 0)
+  if ([item action] == @selector(commandDispatch:) && [item tag] > 0) {
+    *action = [item action];
     return [item tag];
+  }
 
   // "Close window" doesn't use the |commandDispatch:| mechanism. Menu items
   // that do not correspond to IDC_ constants need no special treatment however,
@@ -118,15 +120,15 @@ GetDelayedShortcutsNotPresentInMainMenu() {
 }
 
 CommandForKeyEventResult NoCommand() {
-  return {-1, /*from_main_menu=*/false};
+  return {-1, nil, /*from_main_menu=*/false};
 }
 
-CommandForKeyEventResult MainMenuCommand(int cmd) {
-  return {cmd, /*from_main_menu=*/true};
+CommandForKeyEventResult MainMenuCommand(int cmd, SEL action) {
+  return {cmd, action, /*from_main_menu=*/true};
 }
 
 CommandForKeyEventResult ShortcutCommand(int cmd) {
-  return {cmd, /*from_main_menu=*/false};
+  return {cmd, nil, /*from_main_menu=*/false};
 }
 
 }  // namespace
@@ -200,9 +202,10 @@ CommandForKeyEventResult CommandForKeyEvent(NSEvent* event) {
   if ([event type] != NSKeyDown)
     return NoCommand();
 
-  int cmdNum = MenuCommandForKeyEvent(event);
+  SEL action;
+  int cmdNum = MenuCommandForKeyEvent(event, &action);
   if (cmdNum != -1)
-    return MainMenuCommand(cmdNum);
+    return MainMenuCommand(cmdNum, action);
 
   // Scan through keycodes and see if it corresponds to one of the non-menu
   // shortcuts.

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -94,17 +94,24 @@
   // By not passing the event to AppKit, we do lose out on the brief
   // highlighting of the NSMenu.
   CommandForKeyEventResult result = CommandForKeyEvent(event);
-  if (result.found()) {
-    auto* bridge =
-        remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window);
-    if (bridge) {
-      bool was_executed = false;
-      bridge->host()->ExecuteCommand(
-          result.chrome_command, WindowOpenDisposition::CURRENT_TAB,
-          true /* is_before_first_responder */, &was_executed);
-      if (was_executed)
-        return ui::PerformKeyEquivalentResult::kHandled;
-    }
+
+  if (!result.found())
+    return ui::PerformKeyEquivalentResult::kUnhandled;
+
+  // If the menu item will dispatch to a different window (real-world example:
+  // the dictionary definition popover), don't handle the event here.
+  if (result.action && [NSApp targetForAction:result.action] != window)
+    return ui::PerformKeyEquivalentResult::kUnhandled;
+
+  auto* bridge =
+      remote_cocoa::NativeWidgetNSWindowBridge::GetFromNativeWindow(window);
+  if (bridge) {
+    bool was_executed = false;
+    bridge->host()->ExecuteCommand(
+        result.chrome_command, WindowOpenDisposition::CURRENT_TAB,
+        true /* is_before_first_responder */, &was_executed);
+    if (was_executed)
+      return ui::PerformKeyEquivalentResult::kHandled;
   }
 
   return ui::PerformKeyEquivalentResult::kUnhandled;