rule-based: Use native code for activating/deactivating engine.

<This change is hidden behind a flag>

As part of migrating PK rule-based IMEs to native code, this CL migrates
activate/deactivate to native code path.

We port RuleBasedController#onActivate/onDeactivate in
chos/pk/rulebasedcontroller.js.

Note that we need these to call through to the extension, as it has
side effects that may affect the extension when it is running (e.g.
when VK is up).

We also added a bunch of metrics collection that was in the extension
code.

Note that the JS code has already been guarded by the runtime flag.

Bug: 1009903
Change-Id: Ib72ea3916d6f66c17c6553a7f137e1eb1647e181
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1970294
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726174}

chrome/browser/chromeos/input_method/native_input_method_engine.cc

@@ -6,6 +6,7 @@
 #include "base/feature_list.h"
 #include "base/i18n/i18n_constants.h"
 #include "base/i18n/icu_string_conversions.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -35,6 +36,24 @@ std::string NormalizeString(const std::string& str) {
   return normalized_str;
 }
 
+enum class ImeServiceEvent {
+  kUnknown = 0,
+  kInitSuccess = 1,
+  kInitFailed = 2,
+  kActivateImeSuccess = 3,
+  kActivateImeFailed = 4,
+  kServiceDisconnected = 5,
+  kMaxValue = kServiceDisconnected
+};
+
+void LogEvent(ImeServiceEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("InputMethod.Mojo.Extension.Event", event);
+}
+
+void LogLatency(const char* name, const base::TimeDelta& latency) {
+  base::UmaHistogramCustomCounts(name, latency.InMilliseconds(), 0, 1000, 50);
+}
+
 }  // namespace
 
 NativeInputMethodEngine::NativeInputMethodEngine() = default;
@@ -70,8 +89,18 @@ NativeInputMethodEngine::GetNativeObserver() const {
 NativeInputMethodEngine::ImeObserver::ImeObserver(
     std::unique_ptr<InputMethodEngineBase::Observer> base_observer)
     : base_observer_(std::move(base_observer)), receiver_from_engine_(this) {
-  input_method::InputMethodManager::Get()->ConnectInputEngineManager(
+  auto* ime_manager = input_method::InputMethodManager::Get();
+
+  const auto start = base::Time::Now();
+  ime_manager->ConnectInputEngineManager(
       remote_manager_.BindNewPipeAndPassReceiver());
+  LogLatency("InputMethod.Mojo.Extension.ServiceInitLatency",
+             base::Time::Now() - start);
+
+  remote_manager_.set_disconnect_handler(base::BindOnce(
+      &ImeObserver::OnError, base::Unretained(this), base::Time::Now()));
+
+  LogEvent(ImeServiceEvent::kInitSuccess);
 }
 
 NativeInputMethodEngine::ImeObserver::~ImeObserver() = default;
@@ -83,10 +112,16 @@ void NativeInputMethodEngine::ImeObserver::OnActivate(
     // manifest, but the InputEngineManager expects the prefix "m17n:".
     // TODO(https://crbug.com/1012490): Migrate to m17n prefix and remove this.
     const auto new_engine_id = "m17n:" + engine_id.substr(4);
+
+    // Deactivate any existing engine.
+    remote_to_engine_.reset();
+    receiver_from_engine_.reset();
+
     remote_manager_->ConnectToImeEngine(
         new_engine_id, remote_to_engine_.BindNewPipeAndPassReceiver(),
         receiver_from_engine_.BindNewPipeAndPassRemote(), {},
-        base::BindOnce(&ImeObserver::OnConnected, base::Unretained(this)));
+        base::BindOnce(&ImeObserver::OnConnected, base::Unretained(this),
+                       base::Time::Now()));
   }
   base_observer_->OnActivate(engine_id);
 }
@@ -104,7 +139,7 @@ void NativeInputMethodEngine::ImeObserver::OnKeyEvent(
     const std::string& engine_id,
     const InputMethodEngineBase::KeyboardEvent& event,
     ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) {
-  if (ShouldEngineUseMojo(engine_id)) {
+  if (ShouldEngineUseMojo(engine_id) && remote_to_engine_.is_bound()) {
     remote_to_engine_->ProcessKeypressForRulebased(
         ime::mojom::KeypressInfoForRulebased::New(
             event.type, event.code, event.shift_key, event.altgr_key,
@@ -118,7 +153,7 @@ void NativeInputMethodEngine::ImeObserver::OnKeyEvent(
 
 void NativeInputMethodEngine::ImeObserver::OnReset(
     const std::string& engine_id) {
-  if (ShouldEngineUseMojo(engine_id)) {
+  if (ShouldEngineUseMojo(engine_id) && remote_to_engine_.is_bound()) {
     remote_to_engine_->ResetForRulebased();
   }
   base_observer_->OnReset(engine_id);
@@ -126,6 +161,9 @@ void NativeInputMethodEngine::ImeObserver::OnReset(
 
 void NativeInputMethodEngine::ImeObserver::OnDeactivated(
     const std::string& engine_id) {
+  if (ShouldEngineUseMojo(engine_id)) {
+    remote_to_engine_.reset();
+  }
   base_observer_->OnDeactivated(engine_id);
 }
 
@@ -169,21 +207,41 @@ void NativeInputMethodEngine::ImeObserver::OnScreenProjectionChanged(
 
 void NativeInputMethodEngine::ImeObserver::FlushForTesting() {
   remote_manager_.FlushForTesting();
-  receiver_from_engine_.FlushForTesting();
-  remote_to_engine_.FlushForTesting();
+  if (remote_to_engine_.is_bound())
+    receiver_from_engine_.FlushForTesting();
+  if (remote_to_engine_.is_bound())
+    remote_to_engine_.FlushForTesting();
 }
 
-void NativeInputMethodEngine::ImeObserver::OnConnected(bool bound) {
+void NativeInputMethodEngine::ImeObserver::OnConnected(base::Time start,
+                                                       bool bound) {
+  LogLatency("InputMethod.Mojo.Extension.ActivateIMELatency",
+             base::Time::Now() - start);
+  LogEvent(bound ? ImeServiceEvent::kActivateImeSuccess
+                 : ImeServiceEvent::kActivateImeSuccess);
+
   connected_to_engine_ = bound;
 }
 
+void NativeInputMethodEngine::ImeObserver::OnError(base::Time start) {
+  LOG(ERROR) << "IME Service connection error";
+
+  // If the Mojo pipe disconnection happens in 1000 ms after the service
+  // is initialized, we consider it as a failure. Normally it's caused
+  // by the Mojo service itself or misconfigured on Chrome OS.
+  if (base::Time::Now() - start < base::TimeDelta::FromMilliseconds(1000)) {
+    LogEvent(ImeServiceEvent::kInitFailed);
+  } else {
+    LogEvent(ImeServiceEvent::kServiceDisconnected);
+  }
+}
+
 void NativeInputMethodEngine::ImeObserver::OnKeyEventResponse(
     base::Time start,
     ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback,
     ime::mojom::KeypressResponseForRulebasedPtr response) {
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "InputMethod.Mojo.Extension.Rulebased.ProcessLatency",
-      (base::Time::Now() - start).InMilliseconds(), 0, 1000, 50);
+  LogLatency("InputMethod.Mojo.Extension.Rulebased.ProcessLatency",
+             base::Time::Now() - start);
 
   for (const auto& op : response->operations) {
     switch (op->method) {

chrome/browser/chromeos/input_method/native_input_method_engine.h

@@ -99,7 +99,10 @@ class NativeInputMethodEngine : public InputMethodEngine {
    private:
     // Called when this is connected to the input engine. |bound| indicates
     // the success of the connection.
-    void OnConnected(bool bound);
+    void OnConnected(base::Time start, bool bound);
+
+    // Called when there's a connection error.
+    void OnError(base::Time start);
 
     // Called when a key press is processed by Mojo.
     void OnKeyEventResponse(

chrome/browser/chromeos/input_method/native_input_method_engine_browsertest.cc

@@ -38,7 +38,9 @@ class TestObserver : public InputMethodEngineBase::Observer {
   void OnKeyEvent(
       const std::string& engine_id,
       const InputMethodEngineBase::KeyboardEvent& event,
-      ui::IMEEngineHandlerInterface::KeyEventDoneCallback key_data) override {}
+      ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) override {
+    std::move(callback).Run(/*handled=*/false);
+  }
   void OnInputContextUpdate(
       const ui::IMEEngineHandlerInterface::InputContext& context) override {}
   void OnCandidateClicked(
@@ -130,6 +132,7 @@ class NativeInputMethodEngineTest : public InProcessBrowserTest,
 
 // ID is specified in google_xkb_manifest.json.
 constexpr char kEngineIdVietnameseTelex[] = "vkd_vi_telex";
+constexpr char kEngineIdArabic[] = "vkd_ar";
 
 }  // namespace
 
@@ -183,3 +186,45 @@ IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, VietnameseTelex_Reset) {
 
   SetFocus(nullptr);
 }
+
+IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, SwitchActiveController) {
+  // Swap between two controllers.
+  engine_.Enable(kEngineIdVietnameseTelex);
+  engine_.FlushForTesting();
+  engine_.Disable();
+  engine_.Enable(kEngineIdArabic);
+  engine_.FlushForTesting();
+
+  // Create a fake text field.
+  ui::DummyTextInputClient text_input_client(ui::TEXT_INPUT_TYPE_TEXT);
+  SetFocus(&text_input_client);
+
+  DispatchKeyPress(ui::VKEY_A);
+
+  // Expect to commit 'ش'.
+  ASSERT_EQ(text_input_client.composition_history().size(), 0U);
+  ASSERT_EQ(text_input_client.insert_text_history().size(), 1U);
+  EXPECT_EQ(text_input_client.insert_text_history()[0],
+            base::UTF8ToUTF16(u8"ش"));
+
+  SetFocus(nullptr);
+}
+
+IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, NoActiveController) {
+  engine_.Enable(kEngineIdVietnameseTelex);
+  engine_.FlushForTesting();
+  engine_.Disable();
+
+  // Create a fake text field.
+  ui::DummyTextInputClient text_input_client(ui::TEXT_INPUT_TYPE_TEXT);
+  SetFocus(&text_input_client);
+
+  DispatchKeyPress(ui::VKEY_A);
+  engine_.Reset();
+
+  // Expect no changes.
+  ASSERT_EQ(text_input_client.composition_history().size(), 0U);
+  ASSERT_EQ(text_input_client.insert_text_history().size(), 0U);
+
+  SetFocus(nullptr);
+}