More Impl and fix testing for IME service.

Add more logic and make the service testing really works.

Tested locally by enabling build flags (enable_cros_ime_service and
enable_cros_ime_decoder), then pass the testings
chromeos/services:chromeos_services_unittests

Bug: 837156
Change-Id: I05f557f808c210e788630ce4749b5c261ed19b44
Reviewed-on: https://chromium-review.googlesource.com/1146537
Commit-Queue: Leo Zhang <googleo@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577386}

chromeos/services/ime/BUILD.gn

@@ -45,9 +45,9 @@ source_set("unit_tests") {
     ":lib",
     "//base",
     "//mojo/public/cpp/bindings",
-    "//services/identity/public/mojom",
     "//services/service_manager/public/cpp",
     "//services/service_manager/public/cpp:service_test_support",
+    "//testing/gmock",
   ]
   sources = [
     "ime_service_unittest.cc",

chromeos/services/ime/decoder/decoder_engine.cc

@@ -43,15 +43,26 @@ DecoderEngine::DecoderEngine(
 
 DecoderEngine::~DecoderEngine() {}
 
-void DecoderEngine::BindRequest(const std::string& ime_spec,
+bool DecoderEngine::BindRequest(const std::string& ime_spec,
                                 mojom::InputChannelRequest request,
                                 mojom::InputChannelPtr client,
                                 const std::vector<uint8_t>& extra) {
-  // TODO(https://crbug.com/837156): Build a ClientWrapper with the clientPtr,
-  // inside we ensure the clientPtr will run in the correct SequencedTaskRunner,
-  // then pass the wrapper to the native library instead of the clientPtr.
+  if (!IsImeSupported(ime_spec))
+    return false;
+  // TODO(https://crbug.com/837156): Notify the input logic loaded from the
+  // shared library about the change by forwarding the information received.
+  // Moreover, in order to make safe calls on the clientPtr inside the input
+  // logic with multiple threads, we will send a wrapper of the clientPtr,
+  // where we make sure it runs in the correct SequencedTaskRunner.
   channel_bindings_.AddBinding(this, std::move(request));
   // TODO(https://crbug.com/837156): Registry connection error handler.
+  return true;
+}
+
+bool DecoderEngine::IsImeSupported(const std::string& ime_spec) {
+  // TODO(https://crbug.com/837156): Check the capability of the loaded shared
+  // library first.
+  return InputEngine::IsImeSupported(ime_spec);
 }
 
 void DecoderEngine::ProcessMessage(const std::vector<uint8_t>& message,

chromeos/services/ime/decoder/decoder_engine.h

@@ -28,10 +28,11 @@ class DecoderEngine : public InputEngine {
   ~DecoderEngine() override;
 
   // InputEngine overrides:
-  void BindRequest(const std::string& ime_spec,
+  bool BindRequest(const std::string& ime_spec,
                    mojom::InputChannelRequest request,
                    mojom::InputChannelPtr client,
                    const std::vector<uint8_t>& extra) override;
+  bool IsImeSupported(const std::string& ime_spec) override;
   void ProcessMessage(const std::vector<uint8_t>& message,
                       ProcessMessageCallback callback) override;
 

chromeos/services/ime/ime_service.cc

@@ -50,8 +50,9 @@ void ImeService::ConnectToImeEngine(
     const std::vector<uint8_t>& extra,
     ConnectToImeEngineCallback callback) {
   DCHECK(input_engine_);
-  input_engine_->BindRequest(ime_spec, std::move(to_engine_request),
-                             std::move(from_engine), extra);
+  bool bound = input_engine_->BindRequest(
+      ime_spec, std::move(to_engine_request), std::move(from_engine), extra);
+  std::move(callback).Run(bound);
 }
 
 void ImeService::BindInputEngineManagerRequest(

chromeos/services/ime/ime_service.h

@@ -23,7 +23,7 @@ class ImeService : public service_manager::Service,
   ~ImeService() override;
 
  private:
-  // service_manager::Service:
+  // service_manager::Service overrides:
   void OnStart() override;
   void OnBindInterface(const service_manager::BindSourceInfo& source_info,
                        const std::string& interface_name,

chromeos/services/ime/ime_service_unittest.cc

@@ -4,81 +4,144 @@
 
 #include "base/bind.h"
 
+#include "chromeos/services/ime/ime_service.h"
 #include "chromeos/services/ime/public/mojom/constants.mojom.h"
 #include "chromeos/services/ime/public/mojom/input_engine.mojom.h"
 
 #include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "services/service_manager/public/cpp/service_context.h"
 #include "services/service_manager/public/cpp/service_test.h"
+#include "services/service_manager/public/mojom/service_factory.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::_;
 
 namespace chromeos {
 namespace ime {
 
 namespace {
-
 const char kTestServiceName[] = "ime_unittests";
+const char kInvalidImeSpec[] = "ime_spec_never_support";
 const std::vector<uint8_t> extra{0x66, 0x77, 0x88};
 
-void RunCallback(bool* success, bool result) {
+void ConnectCallback(bool* success, bool result) {
   *success = result;
 }
 
-class ImeServiceTest : public service_manager::test::ServiceTest {
+class TestClientChannel : mojom::InputChannel {
  public:
-  ImeServiceTest() : service_manager::test::ServiceTest(kTestServiceName) {}
-  ~ImeServiceTest() override {}
+  TestClientChannel() : binding_(this) {}
 
-  void SetUp() override {
-    ServiceTest::SetUp();
-    // TODO(https://crbug.com/837156): Start or bind other services used.
-    // Eg.  connector()->StartService(kServiceName) or
-    //      connector()->BindInterface(kServiceName, &ptr);
+  mojom::InputChannelPtr CreateInterfacePtrAndBind() {
+    mojom::InputChannelPtr ptr;
+    binding_.Bind(mojo::MakeRequest(&ptr));
+    return ptr;
   }
 
+  // mojom::InputChannel implementation.
+  MOCK_METHOD2(ProcessText, void(const std::string&, ProcessTextCallback));
+  MOCK_METHOD2(ProcessMessage,
+               void(const std::vector<uint8_t>& message,
+                    ProcessMessageCallback));
+
  private:
-  DISALLOW_COPY_AND_ASSIGN(ImeServiceTest);
+  mojo::Binding<mojom::InputChannel> binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestClientChannel);
 };
 
-class FakeClient : mojom::InputChannel {
+class ImeServiceTestClient : public service_manager::test::ServiceTestClient,
+                             public service_manager::mojom::ServiceFactory {
  public:
-  FakeClient() : binding_(this) {}
+  ImeServiceTestClient(service_manager::test::ServiceTest* test)
+      : service_manager::test::ServiceTestClient(test) {
+    registry_.AddInterface<service_manager::mojom::ServiceFactory>(
+        base::BindRepeating(&ImeServiceTestClient::Create,
+                            base::Unretained(this)));
+  }
 
-  mojom::InputChannelPtr CreateInterfacePtrAndBind() {
-    mojom::InputChannelPtr ptr;
-    binding_.Bind(mojo::MakeRequest(&ptr));
-    return ptr;
+ protected:
+  void OnBindInterface(const service_manager::BindSourceInfo& source_info,
+                       const std::string& interface_name,
+                       mojo::ScopedMessagePipeHandle interface_pipe) override {
+    registry_.BindInterface(interface_name, std::move(interface_pipe));
   }
 
- private:
-  // mojom::InputEngineClient:
-  void ProcessText(const std::string& message,
-                   ProcessTextCallback callback) override{};
-  void ProcessMessage(const std::vector<uint8_t>& message,
-                      ProcessMessageCallback callback) override{};
+  // service_manager::mojom::ServiceFactory
+  void CreateService(
+      service_manager::mojom::ServiceRequest request,
+      const std::string& name,
+      service_manager::mojom::PIDReceiverPtr pid_receiver) override {
+    if (name == mojom::kServiceName) {
+      service_context_.reset(new service_manager::ServiceContext(
+          CreateImeService(), std::move(request)));
+    }
+  }
 
-  mojo::Binding<mojom::InputChannel> binding_;
+  void Create(service_manager::mojom::ServiceFactoryRequest request) {
+    service_factory_bindings_.AddBinding(this, std::move(request));
+  }
+
+ private:
+  service_manager::BinderRegistry registry_;
+  mojo::BindingSet<service_manager::mojom::ServiceFactory>
+      service_factory_bindings_;
 
-  DISALLOW_COPY_AND_ASSIGN(FakeClient);
+  std::unique_ptr<service_manager::ServiceContext> service_context_;
+  DISALLOW_COPY_AND_ASSIGN(ImeServiceTestClient);
 };
 
-}  // namespace
+class ImeServiceTest : public service_manager::test::ServiceTest {
+ public:
+  ImeServiceTest() : service_manager::test::ServiceTest(kTestServiceName) {}
+  ~ImeServiceTest() override {}
 
-// Tests activating an IME and set its delegate.
-TEST_F(ImeServiceTest, ConnectImeEngine) {
-  connector()->StartService(mojom::kServiceName);
+  MOCK_METHOD1(SentTextCallback, void(const std::string&));
+  MOCK_METHOD1(SentMessageCallback, void(const std::vector<uint8_t>&));
 
-  mojom::InputEngineManagerPtr manager;
-  connector()->BindInterface(mojom::kServiceName, &manager);
+ protected:
+  void SetUp() override {
+    ServiceTest::SetUp();
+    connector()->BindInterface(mojom::kServiceName,
+                               mojo::MakeRequest(&ime_manager_));
 
-  bool success = false;
-  FakeClient client;
+    // TODO(https://crbug.com/837156): Start or bind other services used.
+    // Eg.  connector()->StartService(mojom::kSomeServiceName);
+  }
+
+  // service_manager::test::ServiceTest
+  std::unique_ptr<service_manager::Service> CreateService() override {
+    return std::make_unique<ImeServiceTestClient>(this);
+  }
+
+  void TearDown() override {
+    ime_manager_.reset();
+    ServiceTest::TearDown();
+  }
+
+  mojom::InputEngineManagerPtr ime_manager_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ImeServiceTest);
+};
+
+}  // namespace
 
+// Tests that the service is instantiated and it will return false when
+// activating an IME engine with an invalid IME spec.
+TEST_F(ImeServiceTest, ConnectInvalidImeEngine) {
+  bool success = true;
+  TestClientChannel test_channel;
   mojom::InputChannelPtr to_engine_ptr;
 
-  manager->ConnectToImeEngine("FakeIME", mojo::MakeRequest(&to_engine_ptr),
-                              client.CreateInterfacePtrAndBind(), extra,
-                              base::BindOnce(&RunCallback, &success));
+  ime_manager_->ConnectToImeEngine(
+      kInvalidImeSpec, mojo::MakeRequest(&to_engine_ptr),
+      test_channel.CreateInterfacePtrAndBind(), extra,
+      base::BindOnce(&ConnectCallback, &success));
+  ime_manager_.FlushForTesting();
+  EXPECT_FALSE(success);
 }
 
 }  // namespace ime

chromeos/services/ime/input_engine.cc

@@ -14,14 +14,23 @@ InputEngine::InputEngine() {}
 
 InputEngine::~InputEngine() {}
 
-void InputEngine::BindRequest(const std::string& ime_spec,
+bool InputEngine::BindRequest(const std::string& ime_spec,
                               mojom::InputChannelRequest request,
                               mojom::InputChannelPtr client,
                               const std::vector<uint8_t>& extra) {
+  if (!IsImeSupported(ime_spec))
+    return false;
+
   channel_bindings_.AddBinding(this, std::move(request), ime_spec);
+  return true;
   // TODO(https://crbug.com/837156): Registry connection error handler.
 }
 
+bool InputEngine::IsImeSupported(const std::string& ime_spec) {
+  // TODO(https://crbug.com/837156): Add all supported IME tpyes.
+  return false;
+}
+
 void InputEngine::ProcessText(const std::string& message,
                               ProcessTextCallback callback) {
   auto& ime_spec = channel_bindings_.dispatch_context();
@@ -31,7 +40,7 @@ void InputEngine::ProcessText(const std::string& message,
 
 void InputEngine::ProcessMessage(const std::vector<uint8_t>& message,
                                  ProcessMessageCallback callback) {
-  NOTREACHED() << "No defined protobuf message in this implementation";
+  NOTIMPLEMENTED();  // Protobuf message is not used in the basic engine.
 }
 
 const std::string& InputEngine::Process(const std::string& message,

chromeos/services/ime/input_engine.h

@@ -17,12 +17,16 @@ class InputEngine : public mojom::InputChannel {
   InputEngine();
   ~InputEngine() override;
 
-  // Binds the mojom::InputChannel interface to this object.
-  virtual void BindRequest(const std::string& ime_spec,
+  // Binds the mojom::InputChannel interface to this object and returns true if
+  // the given ime_spec is supported by the engine.
+  virtual bool BindRequest(const std::string& ime_spec,
                            mojom::InputChannelRequest request,
                            mojom::InputChannelPtr client,
                            const std::vector<uint8_t>& extra);
 
+  // Returns whether the given ime_spec is supported by this engine.
+  virtual bool IsImeSupported(const std::string& ime_spec);
+
   // mojom::MessageChannel overrides:
   void ProcessText(const std::string& message,
                    ProcessTextCallback callback) override;

chromeos/services/ime/manifest.json

@@ -4,7 +4,10 @@
   "interface_provider_specs": {
     "service_manager:connector": {
       "provides": {
-        "input_engine" : [ "chromeos.ime.mojom.InputEngine" ]
+        "input_engine" : [
+          "chromeos.ime.mojom.InputEngineManager",
+          "chromeos.ime.mojom.InputChannel"
+        ]
       },
       "requires": {
         "service_manager": [ "service_manager:all_users" ]

chromeos/services/ime/public/mojom/constants.mojom

@@ -4,4 +4,4 @@
 
 module chromeos.ime.mojom;
 
-const string kServiceName = "ime_service";
+const string kServiceName = "ime";

chromeos/services/ime/test_manifest.json

@@ -3,6 +3,11 @@
   "display_name": "IME Unittests",
   "interface_provider_specs": {
     "service_manager:connector": {
+      "provides": {
+        "service_manager:service_factory": [
+          "service_manager.mojom.ServiceFactory"
+        ]
+      },
       "requires": {
         "ime": [ "input_engine" ]
       }