Refactoring the mojo stuff in InputMethodEngine.

1) Makes MojoHelper lives as long as InputMethodEngine.
   This is because the mojo connection breakage should NOT be triggered by
   InputMethodEngine::Disable. Otherwise, the engine client and IEFR will both
   handle the connection error and would have race conditions.
2) Makes sure InputMethodEngineBase doesn't call GetInputContextHandler() and
   let subclasses to override all the methods to callback to the engine client.

Bug: 937167
Change-Id: Ic567e5dca153eca9f1b2eb739c66f25a55a30088
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545442Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Shu Chen <shuchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646642}

chrome/browser/chromeos/input_method/input_method_engine.cc

@@ -64,11 +64,17 @@ const int kDefaultPageSize = 9;
 class MojoHelper : public ime::mojom::ImeEngine,
                    public ime::mojom::ImeEngineFactory {
  public:
-  MojoHelper(InputMethodEngine* engine,
-             ime::mojom::ImeEngineFactoryRegistryPtr registry)
-      : engine_(engine), factory_binding_(this), engine_binding_(this) {
+  explicit MojoHelper(InputMethodEngine* engine)
+      : engine_(engine), factory_binding_(this), engine_binding_(this) {}
+  ~MojoHelper() override = default;
+
+  void Activate(ime::mojom::ImeEngineFactoryRegistryPtr registry) {
     ime::mojom::ImeEngineFactoryPtr factory_ptr;
+    factory_binding_.Close();
     factory_binding_.Bind(mojo::MakeRequest(&factory_ptr));
+    factory_binding_.set_connection_error_handler(
+        base::BindOnce(&MojoHelper::OnConnectionLost, base::Unretained(this)));
+
     if (registry) {
       registry_ = std::move(registry);
     } else {
@@ -79,11 +85,11 @@ class MojoHelper : public ime::mojom::ImeEngine,
     }
     registry_->ActivateFactory(std::move(factory_ptr));
   }
-  ~MojoHelper() override = default;
 
   // ime::mojom::ImeEngineFactory overrides:
   void CreateEngine(ime::mojom::ImeEngineRequest engine_request,
                     ime::mojom::ImeEngineClientPtr client) override {
+    engine_binding_.Close();
     engine_binding_.Bind(std::move(engine_request));
     engine_client_ = std::move(client);
   }
@@ -112,7 +118,11 @@ class MojoHelper : public ime::mojom::ImeEngine,
     engine_->SetCompositionBounds(bounds);
   }
 
-  ime::mojom::ImeEngineClientPtr& engine_client() { return engine_client_; }
+  bool IsConnected() const { return engine_client_.is_bound(); }
+
+  ime::mojom::ImeEngineClientProxy* engine_client() {
+    return engine_client_.get();
+  }
 
   void FlushForTesting() {
     if (registry_)
@@ -122,6 +132,14 @@ class MojoHelper : public ime::mojom::ImeEngine,
   }
 
  private:
+  void OnConnectionLost() {
+    // After the connection to |ImeEngineFactoryRegistry| is broken, break the
+    // connection to the client, so that the client can reconnect through Window
+    // Service.
+    engine_binding_.Close();
+    engine_client_.reset();
+  }
+
   InputMethodEngine* engine_;
   mojo::Binding<ime::mojom::ImeEngineFactory> factory_binding_;
   mojo::Binding<ime::mojom::ImeEngine> engine_binding_;
@@ -152,20 +170,16 @@ InputMethodEngine::InputMethodEngine()
     : candidate_window_(new ui::CandidateWindow()),
       window_visible_(false),
       is_mirroring_(false),
-      is_casting_(false) {}
+      is_casting_(false) {
+  mojo_helper_ = std::make_unique<MojoHelper>(this);
+}
 
 InputMethodEngine::~InputMethodEngine() {}
 
 void InputMethodEngine::Enable(const std::string& component_id) {
   InputMethodEngineBase::Enable(component_id);
   EnableInputView();
-  mojo_helper_ = std::make_unique<MojoHelper>(
-      this, std::move(ime_engine_factory_registry_));
-}
-
-void InputMethodEngine::Disable() {
-  InputMethodEngineBase::Disable();
-  mojo_helper_.reset();
+  mojo_helper_->Activate(std::move(ime_engine_factory_registry_));
 }
 
 bool InputMethodEngine::IsActive() const {
@@ -355,19 +369,33 @@ void InputMethodEngine::UpdateComposition(
     const ui::CompositionText& composition_text,
     uint32_t cursor_pos,
     bool is_visible) {
-  ui::IMEInputContextHandlerInterface* input_context =
-      ui::IMEBridge::Get()->GetInputContextHandler();
-  if (input_context)
-    input_context->UpdateCompositionText(composition_text, cursor_pos,
-                                         is_visible);
+  if (mojo_helper_->IsConnected()) {
+    NOTIMPLEMENTED_LOG_ONCE();
+  } else {
+    ui::IMEInputContextHandlerInterface* input_context =
+        ui::IMEBridge::Get()->GetInputContextHandler();
+    if (input_context)
+      input_context->UpdateCompositionText(composition_text, cursor_pos,
+                                           is_visible);
+  }
 }
 
 void InputMethodEngine::CommitTextToInputContext(int context_id,
                                                  const std::string& text) {
-  ui::IMEBridge::Get()->GetInputContextHandler()->CommitText(text);
+  bool committed = false;
+  if (mojo_helper_->IsConnected()) {
+    NOTIMPLEMENTED_LOG_ONCE();
+  } else {
+    ui::IMEInputContextHandlerInterface* input_context =
+        ui::IMEBridge::Get()->GetInputContextHandler();
+    if (input_context) {
+      input_context->CommitText(text);
+      committed = true;
+    }
+  }
 
-  // Records histograms for committed characters.
-  if (!composition_text_->text.empty()) {
+  if (committed && !composition_text_->text.empty()) {
+    // Records histograms for committed characters.
     base::string16 wtext = base::UTF8ToUTF16(text);
     UMA_HISTOGRAM_CUSTOM_COUNTS("InputMethod.CommitLength", wtext.length(), 1,
                                 25, 25);
@@ -375,24 +403,42 @@ void InputMethodEngine::CommitTextToInputContext(int context_id,
   }
 }
 
+void InputMethodEngine::DeleteSurroundingTextToInputContext(
+    int offset,
+    size_t number_of_chars) {
+  if (mojo_helper_->IsConnected()) {
+    NOTIMPLEMENTED_LOG_ONCE();
+  } else {
+    ui::IMEInputContextHandlerInterface* input_context =
+        ui::IMEBridge::Get()->GetInputContextHandler();
+    if (input_context)
+      input_context->DeleteSurroundingText(offset, number_of_chars);
+  }
+}
+
 bool InputMethodEngine::SendKeyEvent(ui::KeyEvent* event,
                                      const std::string& code) {
   DCHECK(event);
   if (event->key_code() == ui::VKEY_UNKNOWN)
     event->set_key_code(ui::DomKeycodeToKeyboardCode(code));
 
-  ui::IMEInputContextHandlerInterface* input_context =
-      ui::IMEBridge::Get()->GetInputContextHandler();
-  if (!input_context)
-    return false;
-
   // Marks the simulated key event is from the Virtual Keyboard.
   ui::Event::Properties properties;
   properties[ui::kPropertyFromVK] = std::vector<uint8_t>();
   event->SetProperties(properties);
 
-  input_context->SendKeyEvent(event);
-  return true;
+  bool sent = false;
+  if (mojo_helper_->IsConnected()) {
+    NOTIMPLEMENTED_LOG_ONCE();
+  } else {
+    ui::IMEInputContextHandlerInterface* input_context =
+        ui::IMEBridge::Get()->GetInputContextHandler();
+    if (input_context) {
+      input_context->SendKeyEvent(event);
+      sent = true;
+    }
+  }
+  return sent;
 }
 
 void InputMethodEngine::EnableInputView() {

chrome/browser/chromeos/input_method/input_method_engine.h

@@ -91,7 +91,6 @@ class InputMethodEngine : public ::input_method::InputMethodEngineBase {
 
   // InputMethodEngineBase overrides.
   void Enable(const std::string& component_id) override;
-  void Disable() override;
   bool IsActive() const override;
 
   // ui::IMEEngineHandlerInterface overrides.
@@ -146,6 +145,8 @@ class InputMethodEngine : public ::input_method::InputMethodEngineBase {
                          bool is_visible) override;
   void CommitTextToInputContext(int context_id,
                                 const std::string& text) override;
+  void DeleteSurroundingTextToInputContext(int offset,
+                                           size_t number_of_chars) override;
   bool SendKeyEvent(ui::KeyEvent* event, const std::string& code) override;
 
   // Enables overriding input view page to Virtual Keyboard window.

chrome/browser/ui/input_method/input_method_engine.cc

@@ -107,6 +107,15 @@ void InputMethodEngine::CommitTextToInputContext(int context_id,
   }
 }
 
+void InputMethodEngine::DeleteSurroundingTextToInputContext(
+    int offset,
+    size_t number_of_chars) {
+  ui::IMEInputContextHandlerInterface* input_context =
+      ui::IMEBridge::Get()->GetInputContextHandler();
+  if (input_context)
+    input_context->DeleteSurroundingText(offset, number_of_chars);
+}
+
 bool InputMethodEngine::SendKeyEvent(ui::KeyEvent* event,
                                      const std::string& code) {
   DCHECK(event);

chrome/browser/ui/input_method/input_method_engine.h

@@ -33,6 +33,8 @@ class InputMethodEngine : public InputMethodEngineBase,
                          bool is_visible) override;
   void CommitTextToInputContext(int context_id,
                                 const std::string& text) override;
+  void DeleteSurroundingTextToInputContext(int offset,
+                                           size_t number_of_chars) override;
   bool SendKeyEvent(ui::KeyEvent* ui_event, const std::string& code) override;
   bool IsActive() const override;
 

chrome/browser/ui/input_method/input_method_engine_base.cc

@@ -211,9 +211,8 @@ void InputMethodEngineBase::Enable(const std::string& component_id) {
 void InputMethodEngineBase::Disable() {
   std::string last_component_id{active_component_id_};
   active_component_id_.clear();
-  if (ui::IMEBridge::Get()->GetInputContextHandler())
-    ui::IMEBridge::Get()->GetInputContextHandler()->CommitText(
-        base::UTF16ToUTF8(composition_text_->text));
+  CommitTextToInputContext(context_id_,
+                           base::UTF16ToUTF8(composition_text_->text));
   composition_text_.reset(new ui::CompositionText());
   observer_->OnDeactivated(last_component_id);
 }
@@ -320,10 +319,7 @@ bool InputMethodEngineBase::DeleteSurroundingText(int context_id,
 
   // TODO(nona): Return false if there is ongoing composition.
 
-  ui::IMEInputContextHandlerInterface* input_context =
-      ui::IMEBridge::Get()->GetInputContextHandler();
-  if (input_context)
-    input_context->DeleteSurroundingText(offset, number_of_chars);
+  DeleteSurroundingTextToInputContext(offset, number_of_chars);
 
   return true;
 }
@@ -418,21 +414,14 @@ void InputMethodEngineBase::KeyEventHandled(const std::string& extension_id,
   handling_key_event_ = false;
   // When finish handling key event, take care of the unprocessed commitText
   // and setComposition calls.
-  ui::IMEInputContextHandlerInterface* input_context =
-      ui::IMEBridge::Get()->GetInputContextHandler();
   if (commit_text_changed_) {
-    if (input_context) {
-      input_context->CommitText(text_);
-    }
+    CommitTextToInputContext(context_id_, text_);
     text_ = "";
     commit_text_changed_ = false;
   }
 
   if (composition_changed_) {
-    if (input_context) {
-      input_context->UpdateCompositionText(
-          composition_, composition_.selection.start(), true);
-    }
+    UpdateComposition(composition_, composition_.selection.start(), true);
     composition_ = ui::CompositionText();
     composition_changed_ = false;
   }

chrome/browser/ui/input_method/input_method_engine_base.h

@@ -212,6 +212,9 @@ class InputMethodEngineBase : virtual public ui::IMEEngineHandlerInterface {
   // Notifies InputContextHanlder to commit |text|.
   virtual void CommitTextToInputContext(int context_id,
                                         const std::string& text) = 0;
+  // Notifies InputContextHandler to delete surrounding text.
+  virtual void DeleteSurroundingTextToInputContext(int offset,
+                                                   size_t number_of_chars) = 0;
   // Sends the key event to the window tree host.
   virtual bool SendKeyEvent(ui::KeyEvent* ui_event,
                             const std::string& code) = 0;