MacViews: Fix accelerator handling while Omnibox is in focus.

ui/views/cocoa/bridged_content_view.mm

Index: ui/views/cocoa/bridged_content_view.mm
diff --git a/ui/views/cocoa/bridged_content_view.mm b/ui/views/cocoa/bridged_content_view.mm
index 7c49cd0444346d7291793796970f16a6d7a7c505..9e2a676b8136bce2da2639c097a6c2172755d74c 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -760,10 +760,35 @@ NSAttributedString* GetAttributedString(
 
 // NSResponder implementation.
 
+- (BOOL)performKeyEquivalent:(NSEvent*)theEvent {

[tapted, 2016/11/17 08:20:14]

I think there's more we have to do here. Have a look at -performKeyEquivalent:
in render_widget_host_view_mac.mm . We probably want the firstResponder check,
and then I really like the idea of a shared method like
-keyEvent:wasKeyEquivalent: which both keyDown and performKeyEquivalent call. We
shouldn't need separate performKeyEquivalentEvent_ and keyDownEvent_ members.

I'm also not sure if we want to skip the bubbling in doCommandBySelector -- i.e.
I think we want that NSBeep() -- what are the cases that beep when we don't want
them to?

The private method override `- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event` in
render_widget_host_view_mac.mm looks really useful as well. Maybe it can even
simplify our performKeyEquivalent: further.

[themblsha, 2016/11/17 17:52:16]

Good catch!
 
https://codereview.chromium.org/2074643003/#msg73

Accelerators that result on noop: command will beep, see my older comment for a
list. If we handle both performKeyEquivalent: and keyDown: it could result in
double-beeps, which would be subpar.
Whoa! That's the ticket! Now your original suggestion is more feasible.

+  // Normally having the keyDown: would be enough for processing accelerator
+  // events, but some only result in keyUp: calls. The approach is similar to
+  // what we're doing in keyDown: with the exception that we should not fallback
+  // to next responder in doCommandBySelector:, as this will result in NSBeep().
+
+  // Convert the event into an action message, according to OSX key mappings.
+  performKeyEquivalentEvent_ = theEvent;
+  performKeyEquivalentEventHandled_.reset();
+  [self interpretKeyEvents:@[ theEvent ]];
+  if (auto result = performKeyEquivalentEventHandled_) {
+    DCHECK(!performKeyEquivalentEvent_);
+    performKeyEquivalentEventHandled_.reset();
+    return *result;
+  }
+
+  DCHECK(performKeyEquivalentEvent_);
+  ui::KeyEvent event(performKeyEquivalentEvent_);
+  [self handleKeyEvent:&event];
+  performKeyEquivalentEvent_ = nil;
+  performKeyEquivalentEventHandled_.reset();
+  return event.handled() ? YES : NO;
+}
+
 - (void)keyDown:(NSEvent*)theEvent {
   // Convert the event into an action message, according to OSX key mappings.
   keyDownEvent_ = theEvent;
   [self interpretKeyEvents:@[ theEvent ]];
+
   keyDownEvent_ = nil;
 }
 
@@ -1286,13 +1311,31 @@ NSAttributedString* GetAttributedString(
   if (keyDownEvent_ && [NSStringFromSelector(selector) hasPrefix:@"insert"]) {
     ui::KeyEvent event(keyDownEvent_);
     [self handleKeyEvent:&event];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = YES;
     return;
   }
 
-  if ([self respondsToSelector:selector])
+  if ([self respondsToSelector:selector]) {
     [self performSelector:selector withObject:nil];
-  else
-    [[self nextResponder] doCommandBySelector:selector];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = YES;
+    return;
+  }
+
+  if (performKeyEquivalentEvent_) {
+    ui::KeyEvent event(performKeyEquivalentEvent_);
+    [self handleKeyEvent:&event];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = event.handled() ? YES : NO;
+    // If the event was not handled we must return NO in performKeyEquivalent:
+    // so the menu bar could have a chance on handling the event. Regardless of
+    // that, we don't want to call doCommandBySelector: on nextResponder, as
+    // this will probably result in NSBeep().
+    return;
+  }
+
+  [[self nextResponder] doCommandBySelector:selector];
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range