rule-based: Fix escaping issue in rulebased input.

We used to have a JSON-based rulebased API, where we needed to escape
certain characters in the protocol. We then added an API based purely on
Mojo types. However, the code for the JSON one was copied for the Mojo
one, so we also escaped certain characters. This means that we actually
insert escaped characters rather than the characters themselves.

For example, typing "\" gives "\\".

We delete the escaping code and add tests to prevent this in the future.

Bug: 1014384
Change-Id: I8865869aa020b94c961c71826fa72d6cf2c96b67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1859809Reviewed-by: Keith Lee <keithlee@chromium.org>
Reviewed-by: Leo Zhang <googleo@chromium.org>
Commit-Queue: Leo Zhang <googleo@chromium.org>
Auto-Submit: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706777}

chromeos/services/ime/ime_service_unittest.cc

@@ -303,5 +303,60 @@ TEST_F(ImeServiceTest, RuleBasedDevaPhone) {
   EXPECT_EQ(response.operations, expected_operations);
 }
 
+// Tests escapable characters. See https://crbug.com/1014384.
+TEST_F(ImeServiceTest, RuleBasedDoesNotEscapeCharacters) {
+  bool success = false;
+  TestClientChannel test_channel;
+  mojo::Remote<mojom::InputChannel> to_engine_remote;
+
+  remote_manager_->ConnectToImeEngine(
+      "m17n:deva_phone", to_engine_remote.BindNewPipeAndPassReceiver(),
+      test_channel.CreatePendingRemote(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  remote_manager_.FlushForTesting();
+  EXPECT_TRUE(success);
+
+  mojom::KeypressResponseForRulebased response;
+
+  // Test Shift+Quote ('"').
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "Quote", true, false,
+                                           false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, true);
+  ASSERT_EQ(1U, response.operations.size());
+  EXPECT_EQ(mojom::OperationMethodForRulebased::COMMIT_TEXT,
+            response.operations[0]->method);
+  EXPECT_EQ("\"", response.operations[0]->arguments);
+
+  // Backslash.
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "Backslash", false, false,
+                                           false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, true);
+  ASSERT_EQ(1U, response.operations.size());
+  EXPECT_EQ(mojom::OperationMethodForRulebased::COMMIT_TEXT,
+            response.operations[0]->method);
+  EXPECT_EQ("\\", response.operations[0]->arguments);
+
+  // Shift+Comma ('<')
+  to_engine_remote->ProcessKeypressForRulebased(
+      mojom::KeypressInfoForRulebased::New("keydown", "Comma", true, false,
+                                           false, false, false),
+      base::BindOnce(&TestProcessKeypressForRulebasedCallback, &response));
+  to_engine_remote.FlushForTesting();
+
+  EXPECT_EQ(response.result, true);
+  ASSERT_EQ(1U, response.operations.size());
+  EXPECT_EQ(mojom::OperationMethodForRulebased::COMMIT_TEXT,
+            response.operations[0]->method);
+  EXPECT_EQ("<", response.operations[0]->arguments);
+}
+
 }  // namespace ime
 }  // namespace chromeos

chromeos/services/ime/input_engine.cc

@@ -4,8 +4,6 @@
 
 #include "chromeos/services/ime/input_engine.h"
 
-#include "base/json/json_reader.h"
-#include "base/json/string_escape.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -41,10 +39,9 @@ mojom::KeypressResponseForRulebasedPtr GenerateKeypressResponseForRulebased(
       mojom::KeypressResponseForRulebased::New();
   keypress_response->result = process_key_result.key_handled;
   if (!process_key_result.commit_text.empty()) {
-    std::string commit_text;
-    base::EscapeJSONString(process_key_result.commit_text, false, &commit_text);
     keypress_response->operations.push_back(mojom::OperationForRulebased::New(
-        mojom::OperationMethodForRulebased::COMMIT_TEXT, commit_text));
+        mojom::OperationMethodForRulebased::COMMIT_TEXT,
+        process_key_result.commit_text));
   }
   // Need to add the setComposition operation to the result when the key is
   // handled and commit_text and composition_text are both empty.
@@ -53,11 +50,9 @@ mojom::KeypressResponseForRulebasedPtr GenerateKeypressResponseForRulebased(
   if (!process_key_result.composition_text.empty() ||
       (process_key_result.key_handled &&
        process_key_result.commit_text.empty())) {
-    std::string composition_text;
-    base::EscapeJSONString(process_key_result.composition_text, false,
-                           &composition_text);
     keypress_response->operations.push_back(mojom::OperationForRulebased::New(
-        mojom::OperationMethodForRulebased::SET_COMPOSITION, composition_text));
+        mojom::OperationMethodForRulebased::SET_COMPOSITION,
+        process_key_result.composition_text));
   }
   return keypress_response;
 }