macOS: Let reserved keyEquivalents bypass the main menu.

The main menu has a built-in 100ms delay. Combined with higher priority for
incoming NSEvents compared to chrome tasks, holding down key equivalents [e.g.
cmd + w] starves the chrome task queue. The implementation of tab-closing
requires the main thread to process the on unload ACK from the renderer before
closing the tab [which is a chrome task], so this causes a live-lock.

This CL lets reserved keyEquivalents bypass the main menu. This avoids the
live-lock indicated above. This also loses the built-in 100ms throttle, so this
CL adds a custom 50ms throttle [disabled in tests].

Bug: 836947
Change-Id: Idb88ee84d7aa82da4104893ed85358c75e385ea3
Reviewed-on: https://chromium-review.googlesource.com/1108095
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569285}

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h

@@ -13,6 +13,9 @@
 // shortcuts and executing them with chrome::ExecuteCommand.
 @interface ChromeCommandDispatcherDelegate : NSObject<CommandDispatcherDelegate>
 
+// Permanently disabled throttling of repeated commands.
++ (void)disableThrottleForTesting;
+
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_CHROME_COMMAND_DISPATCHER_DELEGATE_H_

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -16,7 +16,56 @@
 #import "chrome/browser/ui/cocoa/tabs/tab_strip_controller.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 
+namespace {
+
+// For commands that bypass the main menu, we need a custom throttle
+// implementation since we don't have main menu's 100ms throttle. If a command
+// is repeated, and less than 50ms has passed, ignore it. 50ms was chosen as a
+// time that feels good - 100ms feels too long.
+constexpr NSTimeInterval kThrottleTimeIntervalSeconds = 0.05;
+
+// Browser tests disable throttling so that they can quickly send key events.
+bool g_throttling_enabled = true;
+
+}  // namespace
+
+@interface ChromeCommandDispatcherDelegate ()
+// We track the last time we let a hotkey bypass the main menu. This allows us
+// to implement a custom throttle. By default, the main menu has a built-in
+// 100ms throttle [also used to highlight the .
+@property(nonatomic, retain) NSDate* lastMainMenuBypassDate;
+@property(nonatomic, assign) int lastMainMenuBypassChromeCommand;
+@end
+
 @implementation ChromeCommandDispatcherDelegate
+@synthesize lastMainMenuBypassDate = lastMainMenuBypassDate_;
+@synthesize lastMainMenuBypassChromeCommand = lastMainMenuBypassChromeCommand_;
+
++ (void)disableThrottleForTesting {
+  g_throttling_enabled = false;
+}
+
+- (void)dealloc {
+  [lastMainMenuBypassDate_ release];
+  [super dealloc];
+}
+
+- (BOOL)shouldThrottleChromeCommand:(int)command {
+  if (!g_throttling_enabled)
+    return NO;
+  return self.lastMainMenuBypassChromeCommand == command &&
+         self.lastMainMenuBypassDate &&
+         fabs([self.lastMainMenuBypassDate timeIntervalSinceNow]) <
+             kThrottleTimeIntervalSeconds;
+}
+
+- (void)executeChromeCommandBypassingMainMenu:(int)command
+                                      browser:(Browser*)browser {
+  self.lastMainMenuBypassDate = [NSDate date];
+  self.lastMainMenuBypassChromeCommand = command;
+
+  chrome::ExecuteCommand(browser, command);
+}
 
 - (BOOL)eventHandledByExtensionCommand:(NSEvent*)event
                               priority:(ui::AcceleratorManager::HandlerPriority)
@@ -76,21 +125,25 @@
   //
   // By not passing the event to AppKit, we do lose out on the brief
   // highlighting of the NSMenu.
-  //
-  // TODO(erikchen): Add a throttle. Otherwise, it's possible for a user holding
-  // down a hotkey [e.g. cmd + w] to accidentally close too many tabs!
-  // https://crbug.com/846893.
   CommandForKeyEventResult result = CommandForKeyEvent(event);
   if (result.found()) {
     Browser* browser = chrome::FindBrowserWithWindow(window);
     if (browser &&
         browser->command_controller()->IsReservedCommandOrKey(
             result.chrome_command, content::NativeWebKeyboardEvent(event))) {
-      if (result.from_main_menu) {
-        return ui::PerformKeyEquivalentResult::kPassToMainMenu;
+      // If a command is reserved, then we also have it bypass the main menu.
+      // This is based on the rough approximation that reserved commands are
+      // also the ones that we want to be quickly repeatable.
+      // https://crbug.com/836947.
+
+      if ([self shouldThrottleChromeCommand:result.chrome_command]) {
+        // Claim to have handled the command to prevent anyone else from
+        // processing it.
+        return ui::PerformKeyEquivalentResult::kHandled;
       }
 
-      chrome::ExecuteCommand(browser, result.chrome_command);
+      [self executeChromeCommandBypassingMainMenu:result.chrome_command
+                                          browser:browser];
       return ui::PerformKeyEquivalentResult::kHandled;
     }
   }
@@ -117,11 +170,22 @@
   if (result.found()) {
     Browser* browser = chrome::FindBrowserWithWindow(window);
     if (browser) {
+      // postPerformKeyEquivalent: is only called on events that are not
+      // reserved. We want to bypass the main menu if and only if the event is
+      // reserved. As such, we let all events with main menu keyEquivalents be
+      // handled by the main menu.
       if (result.from_main_menu) {
         return ui::PerformKeyEquivalentResult::kPassToMainMenu;
       }
 
-      chrome::ExecuteCommand(browser, result.chrome_command);
+      if ([self shouldThrottleChromeCommand:result.chrome_command]) {
+        // Claim to have handled the command to prevent anyone else from
+        // processing it.
+        return ui::PerformKeyEquivalentResult::kHandled;
+      }
+
+      [self executeChromeCommandBypassingMainMenu:result.chrome_command
+                                          browser:browser];
       return ui::PerformKeyEquivalentResult::kHandled;
     }
   }

chrome/test/BUILD.gn

@@ -4518,7 +4518,7 @@ if (!is_android) {
       "../browser/ssl/cert_verifier_browser_test.h",
       "base/in_process_browser_test.cc",
       "base/in_process_browser_test.h",
-      "base/in_process_browser_test_mac.cc",
+      "base/in_process_browser_test_mac.mm",
       "base/javascript_browser_test.cc",
       "base/javascript_browser_test.h",
       "base/mojo_web_ui_browser_test.cc",

chrome/test/base/in_process_browser_test.cc

@@ -245,6 +245,9 @@ void InProcessBrowserTest::SetUp() {
   CHECK(base::PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS,
                                     default_download_dir_.GetPath()));
 
+#if defined(OS_MACOSX)
+  InProcessBrowserTest::SetUpMacOS();
+#endif
   BrowserTestBase::SetUp();
 }
 

chrome/test/base/in_process_browser_test.h

@@ -229,6 +229,9 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   base::mac::ScopedNSAutoreleasePool* AutoreleasePool() const {
     return autorelease_pool_;
   }
+
+  // Initializes macOS-only state.
+  void SetUpMacOS();
 #endif  // OS_MACOSX
 
   void set_exit_when_last_browser_closes(bool value) {

chrome/test/base/in_process_browser_test_mac.cc → chrome/test/base/in_process_browser_test_mac.mm

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
+#import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "content/public/test/test_navigation_observer.h"
 
@@ -78,9 +79,8 @@ Browser* InProcessBrowserTest::CreateBrowserForPopup(Profile* profile) {
   return browser;
 }
 
-Browser* InProcessBrowserTest::CreateBrowserForApp(
-    const std::string& app_name,
-    Profile* profile) {
+Browser* InProcessBrowserTest::CreateBrowserForApp(const std::string& app_name,
+                                                   Profile* profile) {
   // Making a browser window can cause AppKit to throw objects into the
   // autorelease pool. Flush the pool when this function returns.
   base::mac::ScopedNSAutoreleasePool pool;
@@ -90,3 +90,7 @@ Browser* InProcessBrowserTest::CreateBrowserForApp(
   AddBlankTabAndShow(browser);
   return browser;
 }
+
+void InProcessBrowserTest::SetUpMacOS() {
+  [ChromeCommandDispatcherDelegate disableThrottleForTesting];
+}