RemoteMacViews: Don't redispatch events to the browser

Keyboard events that are targeted first at the web contents area, but
are not handled by the web contents, are bounced back to the browser
via the in content::RenderWidgetHostImpl::OnKeyboardEventAck, where
they are forwarded to BrowserFrameMac::HandleKeyboardEvent to be
redispatched in the browser process.

This redispatch happens in the browser process even if the keyboard
event originated in the app shim process. The result is strange
behavior such as command-N re-focusing Chrome and creating a new
browser window.

Ensure that the redispatch happens in the originating process, by
having BrowserFrameMac::HandleKeyboardEvent delegate the redispatch
to its NativeWidgetMac base class, which will then forward the
event to the app shim process.

Two small wrinkles about this:
- The type used by BrowserFrameMac::HandleKeyboardEvent is
  inconveniently content::NativeWebKeyboardEvent, which cannot be
  accessed in views/
  - the only thing used in redispatch is |os_event|, which was
    reconstructed from a blink::WebKeyboardEvent
  - instead of passing the event over mojo, just pass the parts that
    are extracted by blink::WebKeyboardEvent
  - ideally we should switch to using a ui::KeyEvent, and just pass
    that directly (and add a function to reconstitute a NSEvent
    from a ui::KeyEvent).
- When redispatched in-process, we do want to accurately know the
  return value from the redispatch.
  - To get this accurately, we take a different path in-process
    versus out-of-process (because the mojo call has no return value).
  - Always report out-of-process events as handled, to arrest further
    processing in the browser.

Bug: 895169
Change-Id: I5fe1011374a4ca5050c7af70d6118286f7b46b34
Reviewed-on: https://chromium-review.googlesource.com/c/1292446Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601743}

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

@@ -243,10 +243,5 @@ bool BrowserFrameMac::HandleKeyboardEvent(
 
   // Redispatch the event. If it's a keyEquivalent:, this gives
   // CommandDispatcher the opportunity to finish passing the event to consumers.
-  NSWindow* window = GetNativeWindow().GetNativeNSWindow();
-  DCHECK([window.class conformsToProtocol:@protocol(CommandDispatchingWindow)]);
-  NSObject<CommandDispatchingWindow>* command_dispatching_window =
-      base::mac::ObjCCastStrict<NSObject<CommandDispatchingWindow>>(window);
-  return [[command_dispatching_window commandDispatcher]
-      redispatchKeyEvent:event.os_event];
+  return RedispatchKeyEvent(event.os_event);
 }

ui/views/widget/native_widget_mac.h

@@ -161,6 +161,10 @@ class VIEWS_EXPORT NativeWidgetMac : public internal::NativeWidgetPrivate {
   // Optional hook for subclasses invoked by WindowDestroying().
   virtual void OnWindowDestroying(gfx::NativeWindow window) {}
 
+  // Redispatch a keyboard event using the widget's window's CommandDispatcher.
+  // Return true if the event is handled.
+  bool RedispatchKeyEvent(NSEvent* event);
+
   internal::NativeWidgetDelegate* delegate() { return delegate_; }
   views_bridge_mac::mojom::BridgedNativeWidget* bridge() const;
   BridgedNativeWidgetImpl* bridge_impl() const;

ui/views/widget/native_widget_mac.mm

@@ -639,6 +639,22 @@ BridgeFactoryHost* NativeWidgetMac::GetBridgeFactoryHost() {
   return nullptr;
 }
 
+bool NativeWidgetMac::RedispatchKeyEvent(NSEvent* event) {
+  // If the target window is in-process, then redispatch the event directly,
+  // and give an accurate return value.
+  if (bridge_impl())
+    return bridge_impl()->RedispatchKeyEvent(event);
+
+  // If the target window is out of process then always report the event as
+  // handled (because it should never be handled in this process).
+  bridge()->RedispatchKeyEvent(
+      [event type], [event modifierFlags], [event timestamp],
+      base::SysNSStringToUTF16([event characters]),
+      base::SysNSStringToUTF16([event charactersIgnoringModifiers]),
+      [event keyCode]);
+  return true;
+}
+
 views_bridge_mac::mojom::BridgedNativeWidget* NativeWidgetMac::bridge() const {
   return bridge_host_ ? bridge_host_->bridge() : nullptr;
 }

ui/views_bridge_mac/bridged_native_widget_impl.h

@@ -172,6 +172,10 @@ class VIEWS_EXPORT BridgedNativeWidgetImpl
   bool ShouldRunCustomAnimationFor(
       views_bridge_mac::mojom::VisibilityTransition transition) const;
 
+  // Redispatch a keyboard event using the widget's window's CommandDispatcher.
+  // Return true if the event is handled.
+  bool RedispatchKeyEvent(NSEvent* event);
+
   // display::DisplayObserver:
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t metrics) override;
@@ -217,6 +221,12 @@ class VIEWS_EXPORT BridgedNativeWidgetImpl
   void UpdateTooltip() override;
   void AcquireCapture() override;
   void ReleaseCapture() override;
+  void RedispatchKeyEvent(uint64_t type,
+                          uint64_t modifier_flags,
+                          double timestamp,
+                          const base::string16& characters,
+                          const base::string16& characters_ignoring_modifiers,
+                          uint32_t key_code) override;
 
   // TODO(ccameron): This method exists temporarily as we move all direct access
   // of TextInputClient out of BridgedContentView.

ui/views_bridge_mac/bridged_native_widget_impl.mm

@@ -961,6 +961,15 @@ bool BridgedNativeWidgetImpl::ShouldRunCustomAnimationFor(
   return true;
 }
 
+bool BridgedNativeWidgetImpl::RedispatchKeyEvent(NSEvent* event) {
+  NSWindow* window = ns_window();
+  DCHECK([window.class conformsToProtocol:@protocol(CommandDispatchingWindow)]);
+  NSObject<CommandDispatchingWindow>* command_dispatching_window =
+      base::mac::ObjCCastStrict<NSObject<CommandDispatchingWindow>>(window);
+  return
+      [[command_dispatching_window commandDispatcher] redispatchKeyEvent:event];
+}
+
 NSWindow* BridgedNativeWidgetImpl::ns_window() {
   return window_.get();
 }
@@ -1099,6 +1108,28 @@ void BridgedNativeWidgetImpl::SetTextInputClient(
   [bridged_view_ setTextInputClient:text_input_client];
 }
 
+void BridgedNativeWidgetImpl::RedispatchKeyEvent(
+    uint64_t type,
+    uint64_t modifier_flags,
+    double timestamp,
+    const base::string16& characters,
+    const base::string16& characters_ignoring_modifiers,
+    uint32_t key_code) {
+  NSEvent* event =
+      [NSEvent keyEventWithType:static_cast<NSEventType>(type)
+                             location:NSZeroPoint
+                        modifierFlags:modifier_flags
+                            timestamp:timestamp
+                         windowNumber:[window_ windowNumber]
+                              context:nil
+                           characters:base::SysUTF16ToNSString(characters)
+          charactersIgnoringModifiers:base::SysUTF16ToNSString(
+                                          characters_ignoring_modifiers)
+                            isARepeat:NO
+                              keyCode:key_code];
+  RedispatchKeyEvent(event);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BridgedNativeWidgetImpl, former BridgedNativeWidgetOwner:
 

ui/views_bridge_mac/mojo/bridged_native_widget.mojom

@@ -156,4 +156,17 @@ interface BridgedNativeWidget {
   // CocoaMouseCaptureDelegate, then captures all mouse events until released.
   AcquireCapture();
   ReleaseCapture();
+
+  // Redispatch a keyboard event using the widget's window's CommandDispatcher.
+  // Note that the callers of this method use content::NativeWebKeyboardEvent,
+  // which would  introduce a layering violation here. Specify explicitly as
+  // arguments only what is used in constructing the |os_event| member of
+  // content::NativeWebKeyboardEvent, to avoid this layering violation.
+  RedispatchKeyEvent(
+      uint64 type,
+      uint64 modifier_flags,
+      double timestamp,
+      mojo_base.mojom.String16 characters,
+      mojo_base.mojom.String16 characters_ignoring_modifiers,
+      uint32 key_code);
 };