RemoteMacViews: Store ui::TextInputClient in views::TextInputHost

There is currently a ui::TextInputClient* stored in BridgedContentView.
This is problematic because the ui::TextInputClient lives in the
browser process while the BridgedContentView lives in the app process.

Cut-and-paste-move textInputClient_ and pendingTextInputClient_  from
BridgedContentView to TextInputHost, and make it a member of the
BridgedNativeWidgetHostImpl class, using the views_bridge_mac::
BridgedNativeWidgetHostHelper structure to close the gap between the
two.

This leaves many direct accesses of the ui::TextInputClient from the
app process, which we can now replace one-by-one with mojo calls in
subsequent patches.

Bug: 901490
Change-Id: Ib4497afd4dd7a4e41541eaab170157e1c7eeb90e
Reviewed-on: https://chromium-review.googlesource.com/c/1391879
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619321}

ui/views/BUILD.gn

@@ -480,6 +480,8 @@ jumbo_component("views") {
     "cocoa/bridged_native_widget_host_impl.mm",
     "cocoa/drag_drop_client_mac.h",
     "cocoa/drag_drop_client_mac.mm",
+    "cocoa/text_input_host.h",
+    "cocoa/text_input_host.mm",
     "cocoa/tooltip_manager_mac.h",
     "cocoa/tooltip_manager_mac.mm",
     "controls/button/label_button_label.cc",

ui/views/cocoa/bridged_native_widget_host_impl.h

@@ -38,6 +38,7 @@ namespace views {
 
 class BridgedNativeWidgetImpl;
 class NativeWidgetMac;
+class TextInputHost;
 
 // The portion of NativeWidgetMac that lives in the browser process. This
 // communicates to the BridgedNativeWidgetImpl, which interacts with the Cocoa
@@ -86,6 +87,8 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
     return bridge_factory_host_;
   }
 
+  TextInputHost* text_input_host() const { return text_input_host_.get(); }
+
   // A NSWindow that is guaranteed to exist in this process. If the bridge
   // object for this host is in this process, then this points to the bridge's
   // NSWindow. Otherwise, it mirrors the id and bounds of the child window.
@@ -205,6 +208,8 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
   static NSView* GetGlobalCaptureView();
 
  private:
+  friend class TextInputHost;
+
   void UpdateCompositorProperties();
   void DestroyCompositor();
   void RankNSViewsRecursive(View* view, std::map<NSView*, int>* rank) const;
@@ -225,6 +230,8 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
                  gfx::Point* baseline_point) override;
   double SheetPositionY() override;
   views_bridge_mac::DragDropClient* GetDragDropClient() override;
+  ui::TextInputClient* GetTextInputClient() override;
+  void GetHasInputContext(bool* has_text_input_context) override;
 
   // BridgeFactoryHost::Observer:
   void OnBridgeFactoryHostDestroying(BridgeFactoryHost* host) override;
@@ -374,7 +381,10 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
 
   // The id that may be used to look up the NSView for |root_view_|.
   const uint64_t root_view_id_;
-  views::View* root_view_ = nullptr;  // Weak. Owned by |native_widget_mac_|.
+
+  // Weak. Owned by |native_widget_mac_|.
+  views::View* root_view_ = nullptr;
+
   std::unique_ptr<DragDropClientMac> drag_drop_client_;
 
   // The mojo pointer to a BridgedNativeWidget, which may exist in another
@@ -401,6 +411,7 @@ class VIEWS_EXPORT BridgedNativeWidgetHostImpl
 
   std::unique_ptr<TooltipManager> tooltip_manager_;
   std::unique_ptr<ui::InputMethod> input_method_;
+  std::unique_ptr<TextInputHost> text_input_host_;
   FocusManager* focus_manager_ = nullptr;  // Weak. Owned by our Widget.
 
   base::string16 window_title_;

ui/views/cocoa/bridged_native_widget_host_impl.mm

@@ -17,6 +17,7 @@
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/mac/coordinate_conversion.h"
 #include "ui/native_theme/native_theme_mac.h"
+#include "ui/views/cocoa/text_input_host.h"
 #include "ui/views/cocoa/tooltip_manager_mac.h"
 #include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_controller.h"
@@ -90,6 +91,7 @@ BridgedNativeWidgetHostImpl::BridgedNativeWidgetHostImpl(NativeWidgetMac* owner)
       native_widget_mac_(owner),
       root_view_id_(ui::NSViewIds::GetNewId()),
       accessibility_focus_overrider_(this),
+      text_input_host_(new TextInputHost(this)),
       host_mojo_binding_(this) {
   DCHECK(GetIdToWidgetHostImplMap().find(widget_id_) ==
          GetIdToWidgetHostImplMap().end());
@@ -526,6 +528,10 @@ BridgedNativeWidgetHostImpl::GetDragDropClient() {
   return drag_drop_client_.get();
 }
 
+ui::TextInputClient* BridgedNativeWidgetHostImpl::GetTextInputClient() {
+  return text_input_host_->GetTextInputClient();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BridgedNativeWidgetHostImpl, BridgeFactoryHost::Observer:
 void BridgedNativeWidgetHostImpl::OnBridgeFactoryHostDestroying(
@@ -1129,16 +1135,19 @@ void BridgedNativeWidgetHostImpl::OnDidChangeFocus(View* focused_before,
                                                    View* focused_now) {
   ui::InputMethod* input_method =
       native_widget_mac_->GetWidget()->GetInputMethod();
-  if (input_method) {
-    ui::TextInputClient* input_client = input_method->GetTextInputClient();
-    // Sanity check: When focus moves away from the widget (i.e. |focused_now|
-    // is nil), then the textInputClient will be cleared.
-    DCHECK(!!focused_now || !input_client);
-    // TODO(ccameron): TextInputClient is not handled across process borders
-    // yet.
-    if (bridge_impl_)
-      bridge_impl_->SetTextInputClient(input_client);
-  }
+  if (!input_method)
+    return;
+
+  ui::TextInputClient* new_text_input_client =
+      input_method->GetTextInputClient();
+  // Sanity check: When focus moves away from the widget (i.e. |focused_now|
+  // is nil), then the textInputClient will be cleared.
+  DCHECK(!!focused_now || !new_text_input_client);
+  text_input_host_->SetTextInputClient(new_text_input_client);
+}
+
+void BridgedNativeWidgetHostImpl::GetHasInputContext(bool* has_input_context) {
+  return text_input_host_->GetHasInputContext(has_input_context);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

ui/views/cocoa/bridged_native_widget_unittest.mm

@@ -26,6 +26,7 @@
 #include "ui/events/test/cocoa_test_event_utils.h"
 #import "ui/gfx/mac/coordinate_conversion.h"
 #import "ui/views/cocoa/bridged_native_widget_host_impl.h"
+#import "ui/views/cocoa/text_input_host.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
 #include "ui/views/controls/textfield/textfield_model.h"
@@ -550,7 +551,7 @@ Textfield* BridgedNativeWidgetTest::InstallTextField(
   // schedules a task to flash the cursor, so this requires |message_loop_|.
   textfield->RequestFocus();
 
-  [ns_view_ setTextInputClient:textfield];
+  bridge_host()->text_input_host()->SetTextInputClient(textfield);
 
   // Initialize the dummy text view. Initializing this with NSZeroRect causes
   // weird NSTextView behavior on OSX 10.9.
@@ -930,7 +931,7 @@ TEST_F(BridgedNativeWidgetTest, InputContext) {
   EXPECT_FALSE([ns_view_ inputContext]);
   InstallTextField(test_string, ui::TEXT_INPUT_TYPE_TEXT);
   EXPECT_TRUE([ns_view_ inputContext]);
-  [ns_view_ setTextInputClient:nil];
+  bridge_host()->text_input_host()->SetTextInputClient(nullptr);
   EXPECT_FALSE([ns_view_ inputContext]);
   InstallTextField(test_string, ui::TEXT_INPUT_TYPE_NONE);
   EXPECT_FALSE([ns_view_ inputContext]);
@@ -1339,7 +1340,7 @@ TEST_F(BridgedNativeWidgetTest, TextInput_DeleteCommands) {
 // Test that we don't crash during an action message even if the TextInputClient
 // is nil. Regression test for crbug.com/615745.
 TEST_F(BridgedNativeWidgetTest, NilTextInputClient) {
-  [ns_view_ setTextInputClient:nil];
+  bridge_host()->text_input_host()->SetTextInputClient(nullptr);
   NSMutableArray* selectors = [NSMutableArray array];
   [selectors addObjectsFromArray:kMoveActions];
   [selectors addObjectsFromArray:kSelectActions];
@@ -1751,7 +1752,7 @@ TEST_F(BridgedNativeWidgetTest, TextInput_RecursiveUpdateWindows) {
   bool saw_update_windows = false;
   base::RepeatingClosure update_windows_closure = base::BindRepeating(
       [](bool* saw_update_windows, BridgedContentView* view,
-         Textfield* textfield) {
+         BridgedNativeWidgetHostImpl* host, Textfield* textfield) {
         // Ensure updateWindows is not invoked recursively.
         EXPECT_FALSE(*saw_update_windows);
         *saw_update_windows = true;
@@ -1768,7 +1769,7 @@ TEST_F(BridgedNativeWidgetTest, TextInput_RecursiveUpdateWindows) {
         // reacting to InsertChar could theoretically do this, but toolkit-views
         // DCHECKs if there is recursive event dispatch, so call
         // setTextInputClient directly.
-        [view setTextInputClient:textfield];
+        host->text_input_host()->SetTextInputClient(textfield);
 
         // Finally simulate what -[NSApp updateWindows] should _actually_ do,
         // which is to update the input context (from the first responder).
@@ -1777,20 +1778,21 @@ TEST_F(BridgedNativeWidgetTest, TextInput_RecursiveUpdateWindows) {
         // Now, the |textfield| set above should have been set again.
         EXPECT_TRUE(g_fake_current_input_context);
       },
-      &saw_update_windows, ns_view_, textfield);
+      &saw_update_windows, ns_view_, bridge_host(), textfield);
 
   SetHandleKeyEventCallback(base::BindRepeating(
-      [](int* saw_return_count, BridgedContentView* view, Textfield* textfield,
+      [](int* saw_return_count, BridgedContentView* view,
+         BridgedNativeWidgetHostImpl* host, Textfield* textfield,
          const ui::KeyEvent& event) {
         if (event.key_code() == ui::VKEY_RETURN) {
           *saw_return_count += 1;
           // Simulate Textfield::OnBlur() by clearing the input method.
           // Textfield needs to be in a Widget to do this normally.
-          [view setTextInputClient:nullptr];
+          host->text_input_host()->SetTextInputClient(nullptr);
         }
         return false;
       },
-      &vkey_return_count, ns_view_));
+      &vkey_return_count, ns_view_, bridge_host()));
 
   // Starting text (just insert it).
   [ns_view_ insertText:@"ㅂ" replacementRange:NSMakeRange(NSNotFound, 0)];

ui/views/cocoa/text_input_host.h

+// Copyright 2018 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.
+
+#ifndef UI_VIEWS_COCOA_TEXT_INPUT_HOST_H_
+#define UI_VIEWS_COCOA_TEXT_INPUT_HOST_H_
+
+#include "base/macros.h"
+#include "ui/views/views_export.h"
+
+namespace ui {
+class TextInputClient;
+}  // namespace ui
+
+namespace views {
+
+class BridgedNativeWidgetHostImpl;
+
+class VIEWS_EXPORT TextInputHost {
+ public:
+  TextInputHost(BridgedNativeWidgetHostImpl* host_impl);
+  ~TextInputHost();
+
+  // Set the current TextInputClient.
+  void SetTextInputClient(ui::TextInputClient* new_text_input_client);
+
+  // Return true if -[NSView inputContext] should return a non-nil value.
+  void GetHasInputContext(bool* has_input_context);
+
+  // Return a pointer to the host's ui::TextInputClient.
+  // TODO(ccameron): Remove the need for this call.
+  ui::TextInputClient* GetTextInputClient() const;
+
+ private:
+  // Weak. If non-null the TextInputClient of the currently focused views::View
+  // in the hierarchy rooted at the root view of |host_impl_|. Owned by the
+  // focused views::View.
+  ui::TextInputClient* text_input_client_ = nullptr;
+
+  // The TextInputClient about to be set. Requests for a new -inputContext will
+  // use this, but while the input is changing the NSView still needs to service
+  // IME requests using the old |text_input_client_|.
+  ui::TextInputClient* pending_text_input_client_ = nullptr;
+
+  BridgedNativeWidgetHostImpl* const host_impl_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextInputHost);
+};
+
+}  // namespace views
+
+#endif  // UI_VIEWS_COCOA_TEXT_INPUT_HOST_H_

ui/views/cocoa/text_input_host.mm

+// Copyright 2018 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 "ui/views/cocoa/text_input_host.h"
+
+#include "ui/base/ime/text_input_client.h"
+#include "ui/views/cocoa/bridged_native_widget_host_impl.h"
+#include "ui/views_bridge_mac/bridged_native_widget_impl.h"
+
+namespace views {
+
+TextInputHost::TextInputHost(BridgedNativeWidgetHostImpl* host_impl)
+    : host_impl_(host_impl) {}
+
+TextInputHost::~TextInputHost() {}
+
+ui::TextInputClient* TextInputHost::GetTextInputClient() const {
+  return text_input_client_;
+}
+
+void TextInputHost::SetTextInputClient(
+    ui::TextInputClient* new_text_input_client) {
+  if (pending_text_input_client_ == new_text_input_client)
+    return;
+
+  // This method may cause the IME window to dismiss, which may cause it to
+  // insert text (e.g. to replace marked text with "real" text). That should
+  // happen in the old -inputContext (which AppKit stores a reference to).
+  // Unfortunately, the only way to invalidate the the old -inputContext is to
+  // invoke -[NSApp updateWindows], which also wants a reference to the _new_
+  // -inputContext. So put the new inputContext in |pendingTextInputClient_| and
+  // only use it for -inputContext.
+  ui::TextInputClient* old_text_input_client = text_input_client_;
+
+  // Since dismissing an IME may insert text, a misbehaving IME or a
+  // ui::TextInputClient that acts on InsertChar() to change focus a second time
+  // may invoke -setTextInputClient: recursively; with [NSApp updateWindows]
+  // still on the stack. Calling [NSApp updateWindows] recursively may upset
+  // an IME. Since the rest of this method is only to decide whether to call
+  // updateWindows, and we're already calling it, just bail out.
+  if (text_input_client_ != pending_text_input_client_) {
+    pending_text_input_client_ = new_text_input_client;
+    return;
+  }
+
+  // Start by assuming no need to invoke -updateWindows.
+  text_input_client_ = new_text_input_client;
+  pending_text_input_client_ = new_text_input_client;
+
+  if (host_impl_->bridge_impl_ &&
+      host_impl_->bridge_impl_->NeedsUpdateWindows()) {
+    text_input_client_ = old_text_input_client;
+    [NSApp updateWindows];
+    // Note: |pending_text_input_client_| (and therefore +[NSTextInputContext
+    // currentInputContext] may have changed if called recursively.
+    text_input_client_ = pending_text_input_client_;
+  }
+}
+
+void TextInputHost::GetHasInputContext(bool* has_input_context) {
+  *has_input_context = false;
+
+  // If the textInputClient_ does not exist, return nil since this view does not
+  // conform to NSTextInputClient protocol.
+  if (!pending_text_input_client_)
+    return;
+
+  // If a menu is active, and -[NSView interpretKeyEvents:] asks for the
+  // input context, return nil. This ensures the action message is sent to
+  // the view, rather than any NSTextInputClient a subview has installed.
+  bool has_menu_controller = false;
+  host_impl_->GetHasMenuController(&has_menu_controller);
+  if (has_menu_controller)
+    return;
+
+  // When not in an editable mode, or while entering passwords
+  // (http://crbug.com/23219), we don't want to show IME candidate windows.
+  // Returning nil prevents this view from getting messages defined as part of
+  // the NSTextInputClient protocol.
+  switch (pending_text_input_client_->GetTextInputType()) {
+    case ui::TEXT_INPUT_TYPE_NONE:
+    case ui::TEXT_INPUT_TYPE_PASSWORD:
+      return;
+    default:
+      *has_input_context = true;
+  }
+}
+
+}  // namespace views

ui/views_bridge_mac/bridge_factory_impl.mm

@@ -77,6 +77,13 @@ class Bridge : public BridgedNativeWidgetHostHelper {
     // Drag-drop only doesn't work across mojo yet.
     return nullptr;
   }
+  ui::TextInputClient* GetTextInputClient() override {
+    // Text input doesn't work across mojo yet.
+    return nullptr;
+  }
+  void GetHasInputContext(bool* has_input_context) override {
+    *has_input_context = false;
+  }
 
   mojom::BridgedNativeWidgetHostAssociatedPtr host_ptr_;
   std::unique_ptr<BridgedNativeWidgetImpl> bridge_impl_;

ui/views_bridge_mac/bridged_content_view.h

@@ -32,17 +32,6 @@ VIEWS_EXPORT
   // Weak, reset by clearView.
   views::BridgedNativeWidgetImpl* bridge_;
 
-  // Weak. If non-null the TextInputClient of the currently focused View in the
-  // hierarchy rooted at |hostedView_|. Owned by the focused View.
-  // TODO(ccameron): Remove this member.
-  ui::TextInputClient* textInputClient_;
-
-  // The TextInputClient about to be set. Requests for a new -inputContext will
-  // use this, but while the input is changing, |self| still needs to service
-  // IME requests using the old |textInputClient_|.
-  // TODO(ccameron): Remove this member.
-  ui::TextInputClient* pendingTextInputClient_;
-
   // A tracking area installed to enable mouseMoved events.
   ui::ScopedCrTrackingArea cursorTrackingArea_;
 
@@ -62,7 +51,6 @@ VIEWS_EXPORT
 }
 
 @property(readonly, nonatomic) views::BridgedNativeWidgetImpl* bridge;
-@property(assign, nonatomic) ui::TextInputClient* textInputClient;
 @property(assign, nonatomic) BOOL drawMenuBackgroundForBlur;
 
 // Initialize the NSView -> views::View bridge. |viewToHost| must be non-NULL.
@@ -85,6 +73,14 @@ VIEWS_EXPORT
 // or not.
 - (void)updateFullKeyboardAccess;
 
+// The TextInputClient of the currently focused views::View.
+// TODO(ccameron): This cannot be relied on across processes.
+- (ui::TextInputClient*)textInputClient;
+
+// Returns true if it is needed to call -[NSApp updateWindows] while updating
+// the text input client.
+- (bool)needsUpdateWindows;
+
 @end
 
 #endif  // UI_VIEWS_BRIDGE_MAC_BRIDGED_CONTENT_VIEW_H_

ui/views_bridge_mac/bridged_native_widget_host_helper.h

@@ -13,6 +13,10 @@
 
 @class NSView;
 
+namespace ui {
+class TextInputClient;
+}  // namespace ui
+
 namespace views_bridge_mac {
 
 class DragDropClient;
@@ -57,6 +61,14 @@ class VIEWS_BRIDGE_MAC_EXPORT BridgedNativeWidgetHostHelper {
   // Return a pointer to host's DragDropClientMac.
   // TODO(ccameron): Drag-drop behavior needs to be implemented over mojo.
   virtual DragDropClient* GetDragDropClient() = 0;
+
+  // Return a pointer to the host's ui::TextInputClient.
+  // TODO(ccameron): Remove the needs for this call.
+  virtual ui::TextInputClient* GetTextInputClient() = 0;
+
+  // Return true if -[NSView inputContext] should return a non-nil value.
+  // TODO(ccameron): Move this function to the mojo interface.
+  virtual void GetHasInputContext(bool* has_input_context) = 0;
 };
 
 }  // namespace views_bridge_mac

ui/views_bridge_mac/bridged_native_widget_impl.h

@@ -234,9 +234,9 @@ class VIEWS_EXPORT BridgedNativeWidgetImpl
                           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.
-  void SetTextInputClient(ui::TextInputClient* text_input_client);
+  // Return true if [NSApp updateWindows] needs to be called after updating the
+  // TextInputClient.
+  bool NeedsUpdateWindows();
 
   // Compute the window and content size, and forward them to |host_|. This will
   // update widget and compositor size.

ui/views_bridge_mac/bridged_native_widget_impl.mm

@@ -1180,9 +1180,8 @@ void BridgedNativeWidgetImpl::UpdateTooltip() {
   [bridged_view_ updateTooltipIfRequiredAt:point];
 }
 
-void BridgedNativeWidgetImpl::SetTextInputClient(
-    ui::TextInputClient* text_input_client) {
-  [bridged_view_ setTextInputClient:text_input_client];
+bool BridgedNativeWidgetImpl::NeedsUpdateWindows() {
+  return [bridged_view_ needsUpdateWindows];
 }
 
 void BridgedNativeWidgetImpl::RedispatchKeyEvent(