rule-based: Move rule-based logic for whether to send to Mojo.

There are some logic in JS that determines whether to send keys to
Mojo or not. We need to move this code to C++.

Original code: //chos/pk/rulebasedcontroller.js

We decided to move this code to the IME service side, i.e. all keys are
now sent via Mojo, but only some keys are processed by the rule-based
engine.

This shouldn't change behaviour, apart from performance, since we now
send Ctrl keys etc. over Mojo where we didn't before. However, we'll
need to send these keys for UIL-based IMEs anyway, since Mozc needs
to handle shortcut keys.

Once this CL lands, we will delete the corresponding code in JS.

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

chromeos/services/ime/ime_service_unittest.cc

@@ -156,6 +156,116 @@ TEST_F(ImeServiceTest, MultipleClientsRulebased) {
   EXPECT_EQ(1, count);
 }
 
+TEST_F(ImeServiceTest, RuleBasedDoesNotHandleModifierKeys) {
+  bool success = false;
+  TestClientChannel test_channel;
+  mojo::Remote<mojom::InputChannel> to_engine_remote;
+
+  remote_manager_->ConnectToImeEngine(
+      "m17n:ar", to_engine_remote.BindNewPipeAndPassReceiver(),
+      test_channel.CreatePendingRemote(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  remote_manager_.FlushForTesting();
+  EXPECT_TRUE(success);
+
+  constexpr const char* kModifierKeys[] = {
+      "Shift",    "ShiftLeft", "ShiftRight", "Alt",         "AltLeft",
+      "AltRight", "AltGraph",  "CapsLock",   "ControlLeft", "ControlRight"};
+
+  for (const auto* modifier_key : kModifierKeys) {
+    mojom::KeypressResponseForRulebased response;
+    to_engine_remote->ProcessKeypressForRulebased(
+        mojom::KeypressInfoForRulebased::New("keydown", modifier_key, false,
+                                             false, false, false, false),
+        base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+    to_engine_remote.FlushForTesting();
+
+    EXPECT_EQ(response.result, false);
+    ASSERT_EQ(0U, response.operations.size());
+  }
+}
+
+TEST_F(ImeServiceTest, RuleBasedDoesNotHandleCtrlShortCut) {
+  bool success = false;
+  TestClientChannel test_channel;
+  mojo::Remote<mojom::InputChannel> to_engine_remote;
+
+  remote_manager_->ConnectToImeEngine(
+      "m17n:ar", to_engine_remote.BindNewPipeAndPassReceiver(),
+      test_channel.CreatePendingRemote(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  remote_manager_.FlushForTesting();
+  EXPECT_TRUE(success);
+
+  mojom::KeypressResponseForRulebased response;
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "ControlLeft", false,
+                                           false, false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "A", false, false, false,
+                                           true, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, false);
+  ASSERT_EQ(0U, response.operations.size());
+}
+
+TEST_F(ImeServiceTest, RuleBasedDoesNotHandleAltShortCut) {
+  bool success = false;
+  TestClientChannel test_channel;
+  mojo::Remote<mojom::InputChannel> to_engine_remote;
+
+  remote_manager_->ConnectToImeEngine(
+      "m17n:ar", to_engine_remote.BindNewPipeAndPassReceiver(),
+      test_channel.CreatePendingRemote(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  remote_manager_.FlushForTesting();
+  EXPECT_TRUE(success);
+
+  mojom::KeypressResponseForRulebased response;
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "AltLeft", false, false,
+                                           false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "A", false, false, false,
+                                           false, true),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, false);
+  ASSERT_EQ(0U, response.operations.size());
+}
+
+TEST_F(ImeServiceTest, RuleBasedHandlesAltRight) {
+  bool success = false;
+  TestClientChannel test_channel;
+  mojo::Remote<mojom::InputChannel> to_engine_remote;
+
+  remote_manager_->ConnectToImeEngine(
+      "m17n:ar", to_engine_remote.BindNewPipeAndPassReceiver(),
+      test_channel.CreatePendingRemote(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  remote_manager_.FlushForTesting();
+  EXPECT_TRUE(success);
+
+  mojom::KeypressResponseForRulebased response;
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "AltRight", false, false,
+                                           false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "A", false, false, false,
+                                           false, true),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, false);
+  ASSERT_EQ(0U, response.operations.size());
+}
+
 // Tests that the rule-based Arabic keyboard can work correctly.
 TEST_F(ImeServiceTest, RuleBasedArabic) {
   bool success = false;

chromeos/services/ime/input_engine.cc

@@ -57,6 +57,13 @@ mojom::KeypressResponseForRulebasedPtr GenerateKeypressResponseForRulebased(
   return keypress_response;
 }
 
+bool IsModifierKey(const std::string& key_code) {
+  return key_code == "AltLeft" || key_code == "AltRight" ||
+         key_code == "ShiftLeft" || key_code == "ShiftRight" ||
+         key_code == "ControlLeft" || key_code == "ControlRight" ||
+         key_code == "CapsLock";
+}
+
 }  // namespace
 
 InputEngineContext::InputEngineContext(const std::string& ime) : ime_spec(ime) {
@@ -104,8 +111,29 @@ void InputEngine::ProcessKeypressForRulebased(
   auto& context = channel_receivers_.current_context();
   auto& engine = context.get()->engine;
 
+  // According to the W3C spec, |altKey| is false if the AltGr key
+  // is pressed [1]. However, all rule-based input methods on Chrome OS use
+  // the US QWERTY layout as a base layout, with AltGr implemented at this
+  // layer. This means the right Alt key reports as being a normal Alt key, so
+  // |altKey| is true. Thus, we need to take |altKey| and exclude the
+  // right Alt key to determine the status of the "true" Alt key.
+  // [1] https://www.w3.org/TR/uievents-key/#keys-modifier
+  // TODO(https://crbug.com/1014778): Change the base layouts for the
+  // rule-based input methods so that |altKey| is false when AltGr is pressed.
+  if (keypress_info->code == "AltRight") {
+    isAltRightDown_ = keypress_info->type == "keydown";
+  }
+
+  const bool isAltDown = keypress_info->alt && !isAltRightDown_;
+
+  // - Shift/AltRight/Caps/Ctrl are modifier keys for the characters which the
+  // Mojo service may accept, but don't send the keys themselves to Mojo.
+  // - Ctrl+? and Alt+? are shortcut keys, so don't send them to the rule based
+  // engine.
   if (!engine || keypress_info->type.empty() ||
-      keypress_info->type != "keydown") {
+      keypress_info->type != "keydown" ||
+      (IsModifierKey(keypress_info->code) || keypress_info->ctrl ||
+       isAltDown)) {
     std::move(callback).Run(mojom::KeypressResponseForRulebased::New(
         false, std::vector<mojom::OperationForRulebasedPtr>(0)));
     return;
@@ -129,6 +157,7 @@ void InputEngine::ResetForRulebased() {
   if (engine) {
     engine->Reset();
   }
+  isAltRightDown_ = false;
 }
 
 void InputEngine::GetRulebasedKeypressCountForTesting(

chromeos/services/ime/input_engine.h

@@ -61,6 +61,9 @@ class InputEngine : public mojom::InputChannel {
   mojo::ReceiverSet<mojom::InputChannel, std::unique_ptr<InputEngineContext>>
       channel_receivers_;
 
+  // Whether the AltRight key is held down or not. Only used for rule-based.
+  bool isAltRightDown_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(InputEngine);
 };