ime: Fix message ordering bug when focusing quickly after enabling.

IME service always expects an OnInputMethodChanged message first, then
an OnFocus.

However, if we enable NativeInputMethodEngine and then immediately
focus, it will send out OnFocus then OnInputMethodChanged.

This is because OnInputMethodChanged is sent in an async callback,
which may run after we send OnFocus.

Move the code to send OnInputMethodChanged out of the callback and call
it immediately after connecting to IME service. This is ok because Mojo
will queue up any messages automatically if the IME service is not yet
ready.

Bug: b/172990698
Change-Id: I81bc0062f867131d0c8faf32411a845f676b5aeb
Fixed: b/172990698
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2537443
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: My Nguyen <myy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828044}

chrome/browser/chromeos/BUILD.gn

@@ -3134,6 +3134,7 @@ static_library("test_support") {
     "extensions/test_external_cache.h",
     "file_manager/mount_test_util.cc",
     "file_manager/mount_test_util.h",
+    "input_method/stub_input_method_engine_observer.h",
     "lock_screen_apps/fake_lock_screen_profile_creator.cc",
     "lock_screen_apps/fake_lock_screen_profile_creator.h",
     "login/demo_mode/demo_mode_test_helper.cc",
@@ -3205,6 +3206,7 @@ static_library("test_support") {
     "//chromeos/components/drivefs",
     "//chromeos/components/drivefs:test_support",
     "//chromeos/dbus:test_support",
+    "//chromeos/services/ime:test_support",
     "//chromeos/services/multidevice_setup/public/cpp:test_support",
     "//chromeos/settings",
     "//components/crx_file",
@@ -3520,6 +3522,7 @@ source_set("unit_tests") {
     "input_method/input_method_engine_unittest.cc",
     "input_method/input_method_manager_impl_unittest.cc",
     "input_method/input_method_persistence_unittest.cc",
+    "input_method/native_input_method_engine_unittest.cc",
     "input_method/personal_info_suggester_unittest.cc",
     "input_method/ui/candidate_view_unittest.cc",
     "input_method/ui/candidate_window_view_unittest.cc",

chrome/browser/chromeos/input_method/input_method_engine_unittest.cc

@@ -77,6 +77,7 @@ void InitInputMethod() {
   InitializeForTesting(manager);
 }
 
+// TODO(crbug.com/1148157): Use StubInputMethodEngineObserver.
 class TestObserver : public InputMethodEngineBase::Observer {
  public:
   TestObserver() : calls_bitmap_(NONE) {}

chrome/browser/chromeos/input_method/native_input_method_engine.cc

@@ -202,6 +202,9 @@ void NativeInputMethodEngine::ImeObserver::OnActivate(
         receiver_from_engine_.BindNewPipeAndPassRemote(), {},
         base::BindOnce(&ImeObserver::OnConnected, base::Unretained(this),
                        base::Time::Now(), new_engine_id));
+
+    active_engine_id_ = new_engine_id;
+    remote_to_engine_->OnInputMethodChanged(new_engine_id);
   } else {
     // Release the IME service.
     // TODO(b/147709499): A better way to cleanup all.
@@ -452,11 +455,6 @@ void NativeInputMethodEngine::ImeObserver::OnConnected(base::Time start,
              base::Time::Now() - start);
   LogEvent(bound ? ImeServiceEvent::kActivateImeSuccess
                  : ImeServiceEvent::kActivateImeSuccess);
-
-  active_engine_id_ = engine_id;
-  if (ShouldUseFstMojoEngine(engine_id)) {
-    remote_to_engine_->OnInputMethodChanged(engine_id);
-  }
 }
 
 void NativeInputMethodEngine::ImeObserver::OnError(base::Time start) {

chrome/browser/chromeos/input_method/native_input_method_engine_browsertest.cc

@@ -49,6 +49,7 @@ namespace {
 
 constexpr char kEmojiData[] = "happy,😀;😃;😄";
 
+// TODO(crbug.com/1148157): Use StubInputMethodEngineObserver.
 class TestObserver : public InputMethodEngineBase::Observer {
  public:
   TestObserver() = default;

chrome/browser/chromeos/input_method/native_input_method_engine_unittest.cc

+// Copyright 2020 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 "chrome/browser/chromeos/input_method/native_input_method_engine.h"
+
+#include "base/feature_list.h"
+#include "chrome/browser/chromeos/input_method/stub_input_method_engine_observer.h"
+#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/services/ime/mock_input_channel.h"
+#include "chromeos/services/ime/public/mojom/input_engine.mojom.h"
+#include "content/public/test/browser_task_environment.h"
+#include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/ime/chromeos/ime_bridge.h"
+#include "ui/base/ime/chromeos/mock_input_method_manager.h"
+#include "ui/base/ime/text_input_flags.h"
+
+namespace chromeos {
+namespace {
+
+MATCHER_P(MojoEq, value, "") {
+  return *arg == value;
+}
+
+using input_method::InputMethodManager;
+using input_method::StubInputMethodEngineObserver;
+using testing::StrictMock;
+
+constexpr char kEngineIdUs[] = "xkb:us::eng";
+
+class TestInputEngineManager : public ime::mojom::InputEngineManager {
+ public:
+  explicit TestInputEngineManager(ime::mojom::InputChannel* engine)
+      : receiver_(engine) {}
+
+  void ConnectToImeEngine(
+      const std::string& ime_spec,
+      mojo::PendingReceiver<ime::mojom::InputChannel> to_engine_request,
+      mojo::PendingRemote<ime::mojom::InputChannel> from_engine,
+      const std::vector<uint8_t>& extra,
+      ConnectToImeEngineCallback callback) override {
+    receiver_.Bind(std::move(to_engine_request));
+    remote_.Bind(std::move(from_engine));
+    std::move(callback).Run(/*bound=*/true);
+  }
+
+ private:
+  mojo::Receiver<ime::mojom::InputChannel> receiver_;
+  mojo::Remote<ime::mojom::InputChannel> remote_;
+};
+
+class TestInputMethodManager : public input_method::MockInputMethodManager {
+ public:
+  explicit TestInputMethodManager(ime::mojom::InputChannel* engine)
+      : test_input_engine_manager_(engine),
+        receiver_(&test_input_engine_manager_) {}
+
+  void ConnectInputEngineManager(
+      mojo::PendingReceiver<ime::mojom::InputEngineManager> receiver) override {
+    receiver_.Bind(std::move(receiver));
+  }
+
+ private:
+  TestInputEngineManager test_input_engine_manager_;
+  mojo::Receiver<ime::mojom::InputEngineManager> receiver_;
+};
+
+// TODO(crbug.com/1148157): Refactor NativeInputMethodEngine etc. to avoid
+// hidden dependencies on globals such as ImeBridge.
+class NativeInputMethodEngineTest : public ::testing::Test {
+  void SetUp() override {
+    feature_list_.InitWithFeatures(
+        /*enabled_features=*/{features::kAssistPersonalInfo,
+                              features::kAssistPersonalInfoEmail,
+                              features::kAssistPersonalInfoName,
+                              features::kEmojiSuggestAddition,
+                              features::kSystemLatinPhysicalTyping},
+        /*disabled_features=*/{});
+
+    // Needed by NativeInputMethodEngine to interact with the input field.
+    ui::IMEBridge::Initialize();
+
+    // Needed by NativeInputMethodEngine for the virtual keyboard.
+    keyboard_controller_client_ =
+        ChromeKeyboardControllerClient::CreateForTest();
+  }
+
+ private:
+  content::BrowserTaskEnvironment task_environment_;
+  base::test::ScopedFeatureList feature_list_;
+  std::unique_ptr<ChromeKeyboardControllerClient> keyboard_controller_client_;
+};
+
+TEST_F(NativeInputMethodEngineTest, EnableCallsRightMojoFunctions) {
+  testing::StrictMock<ime::MockInputChannel> mock_input_channel;
+  input_method::InputMethodManager::Initialize(
+      new TestInputMethodManager(&mock_input_channel));
+  NativeInputMethodEngine engine;
+  TestingProfile testing_profile;
+  engine.Initialize(std::make_unique<StubInputMethodEngineObserver>(),
+                    /*extension_id=*/"", &testing_profile);
+
+  EXPECT_CALL(mock_input_channel, OnInputMethodChanged(kEngineIdUs));
+
+  engine.Enable(kEngineIdUs);
+  engine.FlushForTesting();
+
+  InputMethodManager::Shutdown();
+}
+
+TEST_F(NativeInputMethodEngineTest, FocusCallsRightMojoFunctions) {
+  testing::StrictMock<ime::MockInputChannel> mock_input_channel;
+  input_method::InputMethodManager::Initialize(
+      new TestInputMethodManager(&mock_input_channel));
+  NativeInputMethodEngine engine;
+  TestingProfile testing_profile;
+  engine.Initialize(std::make_unique<StubInputMethodEngineObserver>(),
+                    /*extension_id=*/"", &testing_profile);
+
+  {
+    testing::InSequence seq;
+    EXPECT_CALL(mock_input_channel, OnInputMethodChanged(kEngineIdUs));
+    EXPECT_CALL(mock_input_channel,
+                OnFocus(MojoEq(ime::mojom::InputFieldInfo(
+                    ime::mojom::InputFieldType::kText,
+                    ime::mojom::AutocorrectMode::kEnabled,
+                    ime::mojom::PersonalizationMode::kEnabled))));
+  }
+
+  ui::IMEEngineHandlerInterface::InputContext input_context(
+      ui::TEXT_INPUT_TYPE_TEXT, ui::TEXT_INPUT_MODE_DEFAULT,
+      ui::TEXT_INPUT_FLAG_NONE, ui::TextInputClient::FOCUS_REASON_MOUSE,
+      /*should_do_learning=*/true);
+  engine.Enable(kEngineIdUs);
+  engine.FocusIn(input_context);
+  engine.FlushForTesting();
+
+  InputMethodManager::Shutdown();
+}
+
+}  // namespace
+}  // namespace chromeos

chrome/browser/chromeos/input_method/stub_input_method_engine_observer.h

+// Copyright 2020 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 CHROME_BROWSER_CHROMEOS_INPUT_METHOD_STUB_INPUT_METHOD_ENGINE_OBSERVER_H_
+#define CHROME_BROWSER_CHROMEOS_INPUT_METHOD_STUB_INPUT_METHOD_ENGINE_OBSERVER_H_
+
+#include <vector>
+
+#include "chrome/browser/chromeos/input_method/input_method_engine_base.h"
+
+namespace chromeos {
+namespace input_method {
+
+class StubInputMethodEngineObserver : public InputMethodEngineBase::Observer {
+ public:
+  StubInputMethodEngineObserver() = default;
+  ~StubInputMethodEngineObserver() override = default;
+
+  void OnActivate(const std::string& engine_id) override {}
+  void OnDeactivated(const std::string& engine_id) override {}
+  void OnFocus(
+      const ui::IMEEngineHandlerInterface::InputContext& context) override {}
+  void OnBlur(int context_id) override {}
+  void OnKeyEvent(
+      const std::string& engine_id,
+      const InputMethodEngineBase::KeyboardEvent& event,
+      ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) override {}
+  void OnInputContextUpdate(
+      const ui::IMEEngineHandlerInterface::InputContext& context) override {}
+  void OnCandidateClicked(
+      const std::string& engine_id,
+      int candidate_id,
+      InputMethodEngineBase::MouseButtonEvent button) override {}
+  void OnMenuItemActivated(const std::string& engine_id,
+                           const std::string& menu_id) override {}
+  void OnSurroundingTextChanged(const std::string& engine_id,
+                                const base::string16& text,
+                                int cursor_pos,
+                                int anchor_pos,
+                                int offset) override {}
+  void OnCompositionBoundsChanged(
+      const std::vector<gfx::Rect>& bounds) override {}
+  void OnScreenProjectionChanged(bool is_projected) override {}
+  void OnReset(const std::string& engine_id) override {}
+  void OnSuggestionsChanged(
+      const std::vector<std::string>& suggestions) override {}
+  void OnInputMethodOptionsChanged(const std::string& engine_id) override {}
+};
+
+}  // namespace input_method
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_INPUT_METHOD_STUB_INPUT_METHOD_ENGINE_OBSERVER_H_

chromeos/services/ime/BUILD.gn

@@ -86,6 +86,7 @@ source_set("services_unittests") {
   testonly = true
   deps = [
     ":lib",
+    ":test_support",
     "//base",
     "//base/test:test_support",
     "//chromeos/constants:constants",
@@ -100,7 +101,17 @@ source_set("services_unittests") {
     "decoder/proto_conversion_unittest.cc",
     "decoder/system_engine_unittest.cc",
     "ime_service_unittest.cc",
+  ]
+}
+
+static_library("test_support") {
+  testonly = true
+  sources = [
     "mock_input_channel.cc",
     "mock_input_channel.h",
   ]
+  deps = [
+    "//chromeos/services/ime/public/mojom",
+    "//testing/gmock",
+  ]
 }

ui/base/ime/chromeos/mock_input_method_manager.cc

@@ -124,7 +124,8 @@ void MockInputMethodManager::State::SetUIStyle(
 MockInputMethodManager::State::~State() = default;
 
 MockInputMethodManager::MockInputMethodManager()
-    : features_enabled_state_(InputMethodManager::FEATURE_ALL) {}
+    : state_(new State()),
+      features_enabled_state_(InputMethodManager::FEATURE_ALL) {}
 
 MockInputMethodManager::~MockInputMethodManager() = default;
 
@@ -193,7 +194,7 @@ scoped_refptr<InputMethodManager::State> MockInputMethodManager::CreateNewState(
 
 scoped_refptr<InputMethodManager::State>
 MockInputMethodManager::GetActiveIMEState() {
-  return nullptr;
+  return state_;
 }
 
 void MockInputMethodManager::SetState(

ui/base/ime/chromeos/mock_input_method_manager.h

@@ -142,6 +142,7 @@ class COMPONENT_EXPORT(UI_BASE_IME_CHROMEOS) MockInputMethodManager
   bool IsKeyboardVisible() override;
 
  private:
+  scoped_refptr<State> state_;
   uint32_t features_enabled_state_;
 
   DISALLOW_COPY_AND_ASSIGN(MockInputMethodManager);