Revise IBus related DBus module.

- IBusEngineFactoryService takes the ownership of engine_id - CreateEngine handler mapping.
- Make IBusEngineFactoryService::CreateEngine asynchronous, and introduce asynchronous unittest.

BUG=None
TEST=ran chromeos_unittests


Review URL: https://chromiumcodereview.appspot.com/10836047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149579 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 130 insertions, 33 deletions

diff --git a/chromeos/dbus/ibus/ibus_engine_factory_service.cc b/chromeos/dbus/ibus/ibus_engine_factory_service.cc
index 6241ad0..ed1b8e0 100644
--- a/chromeos/dbus/ibus/ibus_engine_factory_service.cc
+++ b/chromeos/dbus/ibus/ibus_engine_factory_service.cc
@@ -4,6 +4,9 @@
 
 #include "chromeos/dbus/ibus/ibus_engine_factory_service.h"
 
+#include <map>
+#include <string>
+
 #include "base/string_number_conversions.h"
 #include "chromeos/dbus/ibus/ibus_constants.h"
 #include "dbus/bus.h"
@@ -17,7 +20,6 @@ class IBusEngineFactoryServiceImpl : public IBusEngineFactoryService {
   explicit IBusEngineFactoryServiceImpl(dbus::Bus* bus)
       : bus_(bus),
         weak_ptr_factory_(this) {
-
     exported_object_ = bus_->GetExportedObject(
         dbus::ObjectPath(ibus::engine_factory::kServicePath));
 
@@ -37,14 +39,16 @@ class IBusEngineFactoryServiceImpl : public IBusEngineFactoryService {
 
   // IBusEngineFactoryService override.
   virtual void SetCreateEngineHandler(
+      const std::string& engine_id,
       const CreateEngineHandler& create_engine_handler) OVERRIDE {
-    DCHECK(!create_engine_handler.is_null());
-    create_engine_handler_ = create_engine_handler;
+    DCHECK(!create_engine_callback_map_[engine_id].is_null());
+    create_engine_callback_map_[engine_id] = create_engine_handler;
   }
 
   // IBusEngineFactoryService override.
-  virtual void UnsetCreateEngineHandler() OVERRIDE {
-    create_engine_handler_.Reset();
+  virtual void UnsetCreateEngineHandler(
+      const std::string& engine_id) OVERRIDE {
+    create_engine_callback_map_[engine_id].Reset();
   }
 
  private:
@@ -62,17 +66,28 @@ class IBusEngineFactoryServiceImpl : public IBusEngineFactoryService {
       LOG(ERROR) << "Expected argument is string.";
       return;
     }
-    if(create_engine_handler_.is_null()) {
-      LOG(WARNING) << "The CreateEngine handler is NULL.";
+    if (create_engine_callback_map_[engine_name].is_null()) {
+      LOG(WARNING) << "The CreateEngine handler for " << engine_name
+                   << " is NULL.";
     } else {
-      dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-      dbus::MessageWriter writer(response);
-      const dbus::ObjectPath path = create_engine_handler_.Run(engine_name);
-      writer.AppendObjectPath(path);
-      response_sender.Run(response);
+      create_engine_callback_map_[engine_name].Run(
+          base::Bind(&IBusEngineFactoryServiceImpl::CreateEngineSendReply,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     dbus::Response::FromMethodCall(method_call),
+                     response_sender));
     }
   }
 
+  // Sends reply message for CreateEngine method call.
+  void CreateEngineSendReply(
+      dbus::Response* response,
+      const dbus::ExportedObject::ResponseSender response_sender,
+      const dbus::ObjectPath& path) {
+    dbus::MessageWriter writer(response);
+    writer.AppendObjectPath(path);
+    response_sender.Run(response);
+  }
+
   // Called when the CreateEngine method is exported.
   void CreateEngineExported(const std::string& interface_name,
                             const std::string& method_name,
@@ -82,7 +97,7 @@ class IBusEngineFactoryServiceImpl : public IBusEngineFactoryService {
   }
 
   // CreateEngine method call handler.
-  CreateEngineHandler create_engine_handler_;
+  std::map<std::string, CreateEngineHandler> create_engine_callback_map_;
 
   dbus::Bus* bus_;
   scoped_refptr<dbus::ExportedObject> exported_object_;
@@ -98,8 +113,10 @@ class IBusEngineFactoryServiceStubImpl : public IBusEngineFactoryService {
 
   // IBusEngineFactoryService overrides.
   virtual void SetCreateEngineHandler(
+      const std::string& engine_id,
       const CreateEngineHandler& create_engine_handler) OVERRIDE {}
-  virtual void UnsetCreateEngineHandler() OVERRIDE {}
+  virtual void UnsetCreateEngineHandler(
+      const std::string& engine_id) OVERRIDE {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(IBusEngineFactoryServiceStubImpl);
diff --git a/chromeos/dbus/ibus/ibus_engine_factory_service.h b/chromeos/dbus/ibus/ibus_engine_factory_service.h
index 43f2dd8..ccafde0 100644
--- a/chromeos/dbus/ibus/ibus_engine_factory_service.h
+++ b/chromeos/dbus/ibus/ibus_engine_factory_service.h
@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_DBUS_IBUS_IBUS_ENGINE_FACTORY_SERVICE_H_
 #define CHROMEOS_DBUS_IBUS_IBUS_ENGINE_FACTORY_SERVICE_H_
 
+#include <string>
 #include "base/bind.h"
 #include "base/callback.h"
 #include "chromeos/chromeos_export.h"
@@ -22,20 +23,22 @@ namespace chromeos {
 // service if the extension IME is enabled.
 class CHROMEOS_EXPORT IBusEngineFactoryService {
  public:
-  // The CreateEngine message accepts |engine_id| which identifies the engine
-  // module(ex. "mozc"), and returns an ObjectPath. The ibus-daemon will send
-  // dbus message to returned ObjectPath for engine processing.
-  typedef base::Callback<dbus::ObjectPath (const std::string& engine_id)>
+  typedef base::Callback<void(const dbus::ObjectPath& path)>
+      CreateEngineResponseSender;
+  typedef base::Callback<void(const CreateEngineResponseSender& sender)>
       CreateEngineHandler;
 
   virtual ~IBusEngineFactoryService();
 
-  // Sets CreateEngine method call handler.
+  // Sets CreateEngine method call handler for |engine_id|. If ibus-daemon calls
+  // CreateEngine message with |engine_id|, the |create_engine_handler| will be
+  // called.
   virtual void SetCreateEngineHandler(
+      const std::string& engine_id,
       const CreateEngineHandler& create_engine_handler) = 0;
 
-  // Unsets CreateEngine method call handler.
-  virtual void UnsetCreateEngineHandler() = 0;
+  // Unsets CreateEngine method call handler for |engine_id|.
+  virtual void UnsetCreateEngineHandler(const std::string& engine_id) = 0;
 
   // Factory function, creates a new instance and returns ownership.
   // For normal usage, accesses the sigleton via DBusThreadManager::Get().
diff --git a/chromeos/dbus/ibus/ibus_engine_factory_service_unittest.cc b/chromeos/dbus/ibus/ibus_engine_factory_service_unittest.cc
index b7b68a0..382b55c 100644
--- a/chromeos/dbus/ibus/ibus_engine_factory_service_unittest.cc
+++ b/chromeos/dbus/ibus/ibus_engine_factory_service_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chromeos/dbus/ibus/ibus_engine_factory_service.h"
 
+#include <map>
+#include <string>
 #include "base/message_loop.h"
 #include "chromeos/dbus/ibus/ibus_constants.h"
 #include "dbus/message.h"
@@ -19,14 +21,42 @@ using ::testing::_;
 
 namespace chromeos {
 
-class MockCreateEngineHandler {
+class SynchronousCreateEngineHandler {
  public:
-  MOCK_METHOD1(Run, dbus::ObjectPath(const std::string& engine_name));
+  explicit SynchronousCreateEngineHandler(const dbus::ObjectPath& path)
+      : path_(path) {}
+
+  void Run(const IBusEngineFactoryService::CreateEngineResponseSender& sender) {
+    sender.Run(path_);
+  }
+
+ private:
+  dbus::ObjectPath path_;
+
+  DISALLOW_COPY_AND_ASSIGN(SynchronousCreateEngineHandler);
+};
+
+class AsynchronousCreateEngineHandler {
+ public:
+  AsynchronousCreateEngineHandler(const dbus::ObjectPath& path,
+                                  MessageLoop* message_loop)
+      : path_(path),
+        message_loop_(message_loop) {}
+
+  void Run(const IBusEngineFactoryService::CreateEngineResponseSender& sender) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(sender, path_));
+  }
+
+ private:
+  dbus::ObjectPath path_;
+  MessageLoop* message_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(AsynchronousCreateEngineHandler);
 };
 
 class MockCreateEngineResponseSender {
  public:
-  MockCreateEngineResponseSender(const dbus::ObjectPath expected_path)
+  explicit MockCreateEngineResponseSender(const dbus::ObjectPath& expected_path)
       : expected_path_(expected_path) {}
   MOCK_METHOD1(Run, void(dbus::Response*));
 
@@ -108,7 +138,7 @@ class IBusEngineFactoryServiceTest : public testing::Test {
   }
 };
 
-TEST_F(IBusEngineFactoryServiceTest, CreateEngineTest) {
+TEST_F(IBusEngineFactoryServiceTest, SyncCreateEngineTest) {
   // Set expectations.
   const char kSampleEngine[] = "Sample Engine";
   const dbus::ObjectPath kObjectPath("/org/freedesktop/IBus/Engine/10");
@@ -118,12 +148,12 @@ TEST_F(IBusEngineFactoryServiceTest, CreateEngineTest) {
           Invoke(&response_sender,
                  &MockCreateEngineResponseSender::CheckCreateEngineResponse));
 
+  SynchronousCreateEngineHandler handler(kObjectPath);
   // Set handler expectations.
-  MockCreateEngineHandler handler;
-  EXPECT_CALL(handler, Run(StrEq(kSampleEngine)))
-      .WillOnce(Return(kObjectPath));
-  service_->SetCreateEngineHandler(base::Bind(&MockCreateEngineHandler::Run,
-                                             base::Unretained(&handler)));
+  service_->SetCreateEngineHandler(
+      kSampleEngine,
+      base::Bind(&SynchronousCreateEngineHandler::Run,
+                 base::Unretained(&handler)));
   message_loop_.RunAllPending();
 
   // Invoke method call.
@@ -142,12 +172,53 @@ TEST_F(IBusEngineFactoryServiceTest, CreateEngineTest) {
                  base::Unretained(&response_sender)));
 
   // Unset the handler so expect not calling handler.
-  service_->UnsetCreateEngineHandler();
+  service_->UnsetCreateEngineHandler(kSampleEngine);
   method_exported_map_[ibus::engine_factory::kCreateEngineMethod].Run(
       &method_call,
       base::Bind(&MockCreateEngineResponseSender::CheckCreateEngineResponse,
                  base::Unretained(&response_sender)));
+  message_loop_.RunAllPending();
+}
+
+TEST_F(IBusEngineFactoryServiceTest, AsyncCreateEngineTest) {
+  // Set expectations.
+  const char kSampleEngine[] = "Sample Engine";
+  const dbus::ObjectPath kObjectPath("/org/freedesktop/IBus/Engine/10");
+  MockCreateEngineResponseSender response_sender(kObjectPath);
+  EXPECT_CALL(response_sender, Run(_))
+      .WillOnce(
+          Invoke(&response_sender,
+                 &MockCreateEngineResponseSender::CheckCreateEngineResponse));
 
+  AsynchronousCreateEngineHandler handler(kObjectPath, &message_loop_);
+  // Set handler expectations.
+  service_->SetCreateEngineHandler(
+      kSampleEngine,
+      base::Bind(&AsynchronousCreateEngineHandler::Run,
+                 base::Unretained(&handler)));
+  message_loop_.RunAllPending();
+
+  // Invoke method call.
+  dbus::MethodCall method_call(
+      ibus::engine_factory::kServiceInterface,
+      ibus::engine_factory::kCreateEngineMethod);
+  method_call.SetSerial(10);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kSampleEngine);
+  ASSERT_FALSE(
+      method_exported_map_[ibus::engine_factory::kCreateEngineMethod]
+          .is_null());
+  method_exported_map_[ibus::engine_factory::kCreateEngineMethod].Run(
+      &method_call,
+      base::Bind(&MockCreateEngineResponseSender::Run,
+                 base::Unretained(&response_sender)));
+
+  // Unset the handler so expect not calling handler.
+  service_->UnsetCreateEngineHandler(kSampleEngine);
+  method_exported_map_[ibus::engine_factory::kCreateEngineMethod].Run(
+      &method_call,
+      base::Bind(&MockCreateEngineResponseSender::CheckCreateEngineResponse,
+                 base::Unretained(&response_sender)));
   message_loop_.RunAllPending();
 }
 
diff --git a/chromeos/dbus/ibus/mock_ibus_engine_factory_service.cc b/chromeos/dbus/ibus/mock_ibus_engine_factory_service.cc
index f840e75..4660c9f 100644
--- a/chromeos/dbus/ibus/mock_ibus_engine_factory_service.cc
+++ b/chromeos/dbus/ibus/mock_ibus_engine_factory_service.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
 #include "chromeos/dbus/ibus/mock_ibus_engine_factory_service.h"
 
 namespace chromeos {
@@ -13,10 +14,12 @@ MockIBusEngineFactoryService::~MockIBusEngineFactoryService() {
 }
 
 void MockIBusEngineFactoryService::SetCreateEngineHandler(
+    const std::string& engine_id,
     const CreateEngineHandler& create_engine_handler) {
 }
 
-void MockIBusEngineFactoryService::UnsetCreateEngineHandler() {
+void MockIBusEngineFactoryService::UnsetCreateEngineHandler(
+    const std::string& engine_id) {
 }
 
 }  // namespace chromeos
diff --git a/chromeos/dbus/ibus/mock_ibus_engine_factory_service.h b/chromeos/dbus/ibus/mock_ibus_engine_factory_service.h
index 02eb80c..1436177 100644
--- a/chromeos/dbus/ibus/mock_ibus_engine_factory_service.h
+++ b/chromeos/dbus/ibus/mock_ibus_engine_factory_service.h
@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_DBUS_IBUS_MOCK_IBUS_ENGINE_FACTORY_SERVICE_H_
 #define CHROMEOS_DBUS_IBUS_MOCK_IBUS_ENGINE_FACTORY_SERVICE_H_
 
+#include <string>
 #include "base/basictypes.h"
 #include "chromeos/dbus/ibus/ibus_engine_factory_service.h"
 
@@ -15,8 +16,10 @@ class MockIBusEngineFactoryService : public IBusEngineFactoryService {
   virtual ~MockIBusEngineFactoryService();
 
   virtual void SetCreateEngineHandler(
+      const std::string& engine_id,
       const CreateEngineHandler& create_engine_handler) OVERRIDE;
-  virtual void UnsetCreateEngineHandler() OVERRIDE;
+  virtual void UnsetCreateEngineHandler(
+      const std::string& engine_id) OVERRIDE;
 
   DISALLOW_COPY_AND_ASSIGN(MockIBusEngineFactoryService);
 };