Remove PendingCreateICRequest.

PendingCreateICRequest is no longer necessary because the libibus dependent code is gone, just state flag is fine.
In addition to this, PendingCreateICRequest cause memory leak if the response from ibus-daemon is lost.

BUG=134717,135049
TEST=ran ui_unittests and manually check on lumpy.

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

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

ui/base/ime/input_method_ibus.cc

@@ -38,6 +38,7 @@ namespace {
 
 const int kIBusReleaseMask = 1 << 30;
 const char kClientName[] = "chrome";
+const int kMaxRetryCount = 10;
 
 // Following capability mask is introduced from
 // http://ibus.googlecode.com/svn/docs/ibus-1.4/ibus-ibustypes.html#IBusCapabilite
@@ -162,69 +163,12 @@ void InputMethodIBus::PendingKeyEvent::ProcessPostIME(bool handled) {
   // and 'unmodified_character_' to support i18n VKs like a French VK!
 }
 
-// A class to hold information of a pending request for creating an ibus input
-// context.
-class InputMethodIBus::PendingCreateICRequest {
- public:
-  PendingCreateICRequest(InputMethodIBus* input_method,
-                         PendingCreateICRequest** request_ptr);
-  virtual ~PendingCreateICRequest();
-
-  // Set up signal handlers, or destroy object proxy if the input context is
-  // already abandoned.
-  void InitOrAbandonInputContext();
-
-  // Called if the create input context method call is failed.
-  void OnCreateInputContextFailed();
-
-  // Abandon this pending key event. Its result will just be discarded.
-  void Abandon() {
-    input_method_ = NULL;
-    request_ptr_ = NULL;
-    // Do not reset |ibus_client_| here.
-  }
-
- private:
-  InputMethodIBus* input_method_;
-  PendingCreateICRequest** request_ptr_;
-
-  DISALLOW_COPY_AND_ASSIGN(PendingCreateICRequest);
-};
-
-InputMethodIBus::PendingCreateICRequest::PendingCreateICRequest(
-    InputMethodIBus* input_method,
-    PendingCreateICRequest** request_ptr)
-    : input_method_(input_method),
-      request_ptr_(request_ptr) {
-}
-
-InputMethodIBus::PendingCreateICRequest::~PendingCreateICRequest() {
-  if (request_ptr_) {
-    DCHECK_EQ(*request_ptr_, this);
-    *request_ptr_ = NULL;
-  }
-}
-
-void InputMethodIBus::PendingCreateICRequest::OnCreateInputContextFailed() {
-  // TODO(nona): If the connection between Chrome and ibus-daemon terminates
-  // for some reason, the create ic request will fail. We might want to call
-  // ibus_client_->CreateContext() again after some delay.
-}
-
-void InputMethodIBus::PendingCreateICRequest::InitOrAbandonInputContext() {
-  if (input_method_) {
-    DCHECK(input_method_->IsContextReady());
-    input_method_->SetUpSignalHandlers();
-  } else {
-    GetInputContextClient()->ResetObjectProxy();
-  }
-}
-
 // InputMethodIBus implementation -----------------------------------------
 InputMethodIBus::InputMethodIBus(
     internal::InputMethodDelegate* delegate)
     : ibus_client_(new internal::IBusClient),
-      pending_create_ic_request_(NULL),
+      input_context_state_(INPUT_CONTEXT_STOP),
+      create_input_context_fail_count_(0),
       context_focused_(false),
       composing_text_(false),
       composition_changed_(false),
@@ -423,20 +367,23 @@ void InputMethodIBus::OnDidChangeFocusedClient(TextInputClient* focused_before,
 
 void InputMethodIBus::CreateContext() {
   DCHECK(IsConnected());
-  DCHECK(!pending_create_ic_request_);
 
-  pending_create_ic_request_ = new PendingCreateICRequest(
-      this, &pending_create_ic_request_);
+  if (input_context_state_ != INPUT_CONTEXT_STOP) {
+    DVLOG(1) << "Input context is already created or waiting ibus-daemon"
+                " response.";
+    return;
+  }
+
+  input_context_state_ = INPUT_CONTEXT_WAIT_CREATE_INPUT_CONTEXT_RESPONSE;
 
   // Creates the input context asynchronously.
+  DCHECK(!IsContextReady());
   chromeos::DBusThreadManager::Get()->GetIBusClient()->CreateInputContext(
       kClientName,
       base::Bind(&InputMethodIBus::CreateInputContextDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Unretained(pending_create_ic_request_)),
+                 weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&InputMethodIBus::CreateInputContextFail,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Unretained(pending_create_ic_request_)));
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void InputMethodIBus::SetUpSignalHandlers() {
@@ -478,13 +425,9 @@ void InputMethodIBus::SetUpSignalHandlers() {
 }
 
 void InputMethodIBus::DestroyContext() {
-  if (pending_create_ic_request_) {
-    DCHECK(!IsContextReady());
-    // |pending_create_ic_request_| will be deleted in CreateInputContextDone().
-    pending_create_ic_request_->Abandon();
-    pending_create_ic_request_ = NULL;
+  if (input_context_state_ == INPUT_CONTEXT_STOP)
     return;
-  }
+  input_context_state_ = INPUT_CONTEXT_STOP;
   const chromeos::IBusInputContextClient* input_context =
       chromeos::DBusThreadManager::Get()->GetIBusInputContextClient();
   if (input_context && input_context->IsObjectProxyReady()) {
@@ -906,19 +849,46 @@ void InputMethodIBus::ResetInputContext() {
 }
 
 void InputMethodIBus::CreateInputContextDone(
-    PendingCreateICRequest* ic_request,
     const dbus::ObjectPath& object_path) {
+  DCHECK_NE(INPUT_CONTEXT_RUNNING, input_context_state_);
+
+  if (input_context_state_ == INPUT_CONTEXT_STOP) {
+    // DestroyContext has already been called.
+    return;
+  }
+
   chromeos::DBusThreadManager::Get()->GetIBusInputContextClient()
       ->Initialize(chromeos::DBusThreadManager::Get()->GetIBusBus(),
                    object_path);
-  ic_request->InitOrAbandonInputContext();
-  delete ic_request;
+
+  input_context_state_ = INPUT_CONTEXT_RUNNING;
+  DCHECK(IsContextReady());
+  SetUpSignalHandlers();
 }
 
-void InputMethodIBus::CreateInputContextFail(
-    PendingCreateICRequest* ic_request) {
-  ic_request->OnCreateInputContextFailed();
-  delete ic_request;
+void InputMethodIBus::CreateInputContextFail() {
+  DCHECK_NE(INPUT_CONTEXT_RUNNING, input_context_state_);
+  if (input_context_state_ == INPUT_CONTEXT_STOP) {
+    // CreateInputContext failed but the input context is no longer
+    // necessary, thus do nothing.
+    return;
+  }
+
+  if (++create_input_context_fail_count_ >= kMaxRetryCount) {
+    DVLOG(1) << "CreateInputContext failed even tried "
+             << kMaxRetryCount << " times, give up.";
+    create_input_context_fail_count_ = 0;
+    input_context_state_ = INPUT_CONTEXT_STOP;
+    return;
+  }
+
+  // Try CreateInputContext again.
+  chromeos::DBusThreadManager::Get()->GetIBusClient()->CreateInputContext(
+      kClientName,
+      base::Bind(&InputMethodIBus::CreateInputContextDone,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&InputMethodIBus::CreateInputContextFail,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 bool InputMethodIBus::IsConnected() {

ui/base/ime/input_method_ibus.h

@@ -67,8 +67,16 @@ class UI_EXPORT InputMethodIBus : public InputMethodBase {
                               CompositionText* out_composition) const;
 
  private:
+  enum InputContextState {
+    // The input context is not working.
+    INPUT_CONTEXT_STOP,
+    // The input context is waiting for CreateInputContext reply from
+    // ibus-daemon.
+    INPUT_CONTEXT_WAIT_CREATE_INPUT_CONTEXT_RESPONSE,
+    // The input context is working and ready to communicate with ibus-daemon.
+    INPUT_CONTEXT_RUNNING,
+  };
   class PendingKeyEvent;
-  class PendingCreateICRequest;
 
   // Overridden from InputMethodBase:
   virtual void OnWillChangeFocusedClient(TextInputClient* focused_before,
@@ -160,9 +168,8 @@ class UI_EXPORT InputMethodIBus : public InputMethodBase {
                            bool visible);
   void OnHidePreeditText();
 
-  void CreateInputContextDone(PendingCreateICRequest* ic_request,
-                              const dbus::ObjectPath& object_path);
-  void CreateInputContextFail(PendingCreateICRequest* ic_request);
+  void CreateInputContextDone(const dbus::ObjectPath& object_path);
+  void CreateInputContextFail();
   static void ProcessKeyEventDone(PendingKeyEvent* pending_key_event,
                                   bool is_handled);
   static void ProcessKeyEventFail(PendingKeyEvent* pending_key_event);
@@ -174,9 +181,11 @@ class UI_EXPORT InputMethodIBus : public InputMethodBase {
   // They will be deleted in ProcessKeyEventDone().
   std::set<PendingKeyEvent*> pending_key_events_;
 
-  // The pending request for creating the input context. We need to keep this
-  // pointer so that we can receive or abandon the result.
-  PendingCreateICRequest* pending_create_ic_request_;
+  // Represents input context's state.
+  InputContextState input_context_state_;
+
+  // The count of CreateInputContext message failure.
+  int create_input_context_fail_count_;
 
   // Pending composition text generated by the current pending key event.
   // It'll be sent to the focused text input client as soon as we receive the

ui/base/ime/input_method_ibus_unittest.cc

@@ -20,6 +20,7 @@
 
 namespace ui {
 namespace {
+const int kCreateInputContextMaxTrialCount = 10;
 
 uint32 GetOffsetInUTF16(const std::string& utf8_string, uint32 utf8_offset) {
   string16 utf16_string = UTF8ToUTF16(utf8_string);
@@ -58,38 +59,61 @@ class CreateInputContextSuccessHandler {
 
  private:
   dbus::ObjectPath object_path_;
+
+  DISALLOW_COPY_AND_ASSIGN(CreateInputContextSuccessHandler);
 };
 
 class CreateInputContextFailHandler {
  public:
+  CreateInputContextFailHandler() {}
   void Run(const std::string& client_name,
            const chromeos::IBusClient::CreateInputContextCallback& callback,
            const chromeos::IBusClient::ErrorCallback& error_callback) {
     error_callback.Run();
   }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CreateInputContextFailHandler);
 };
 
 class CreateInputContextNoResponseHandler {
  public:
+  CreateInputContextNoResponseHandler() {}
   void Run(const std::string& client_name,
            const chromeos::IBusClient::CreateInputContextCallback& callback,
            const chromeos::IBusClient::ErrorCallback& error_callback) {
   }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CreateInputContextNoResponseHandler);
 };
 
-class CreateInputContextDelayFailHandler {
+class CreateInputContextDelayHandler {
  public:
-  ~CreateInputContextDelayFailHandler() {
-    error_callback_.Run();
+  explicit CreateInputContextDelayHandler(const dbus::ObjectPath& object_path)
+    : object_path_(object_path) {
   }
 
   void Run(const std::string& client_name,
            const chromeos::IBusClient::CreateInputContextCallback& callback,
            const chromeos::IBusClient::ErrorCallback& error_callback) {
+    callback_ = callback;
     error_callback_ = error_callback;
   }
 
+  void RunCallback(bool success) {
+    if (success)
+      callback_.Run(object_path_);
+    else
+      error_callback_.Run();
+  }
+
+ private:
+  dbus::ObjectPath object_path_;
+  chromeos::IBusClient::CreateInputContextCallback callback_;
   chromeos::IBusClient::ErrorCallback error_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(CreateInputContextDelayHandler);
 };
 
 class InputMethodIBusTest : public internal::InputMethodDelegate,
@@ -253,6 +277,8 @@ class InputMethodIBusTest : public internal::InputMethodDelegate,
   chromeos::MockDBusThreadManagerWithoutGMock* mock_dbus_thread_manager_;
   chromeos::MockIBusClient* mock_ibus_client_;
   chromeos::MockIBusInputContextClient* mock_ibus_input_context_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(InputMethodIBusTest);
 };
 
 // Tests public APIs in ui::InputMethod first.
@@ -358,6 +384,7 @@ TEST_F(InputMethodIBusTest, InitiallyConnected) {
   // However, since the current text input type is 'NONE' (the default), FocusIn
   // shouldn't be called.
   EXPECT_EQ(0, mock_ibus_input_context_client_->focus_in_call_count());
+  EXPECT_TRUE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Create ui::InputMethodIBus, then start ibus-daemon.
@@ -373,6 +400,7 @@ TEST_F(InputMethodIBusTest, InitiallyDisconnected) {
   EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
   EXPECT_EQ(1, mock_ibus_input_context_client_->set_capabilities_call_count());
   EXPECT_EQ(0, mock_ibus_input_context_client_->focus_in_call_count());
+  EXPECT_TRUE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that ui::InputMethodIBus does not crash on "disconnected" signal
@@ -387,6 +415,7 @@ TEST_F(InputMethodIBusTest, Disconnect) {
   // dynamical shutting down.
   mock_dbus_thread_manager_->set_ibus_bus(NULL);
   ime_->OnDisconnected();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that ui::InputMethodIBus re-creates an input context when ibus-daemon
@@ -401,6 +430,7 @@ TEST_F(InputMethodIBusTest, DisconnectThenReconnect) {
             mock_ibus_input_context_client_->reset_object_proxy_call_caount());
   mock_dbus_thread_manager_->set_ibus_bus(NULL);
   ime_->OnDisconnected();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
   chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
   ime_->OnConnected();
   // Check if the old context is deleted.
@@ -409,6 +439,7 @@ TEST_F(InputMethodIBusTest, DisconnectThenReconnect) {
   // Check if a new context is created.
   EXPECT_EQ(2, mock_ibus_client_->create_input_context_call_count());
   EXPECT_EQ(2, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_TRUE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that ui::InputMethodIBus does not crash even if NULL context is
@@ -424,10 +455,14 @@ TEST_F(InputMethodIBusTest, CreateContextFail) {
 
   chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
   ime_->Init(true);
-  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  // InputMethodIBus tries several times if the CreateInputContext method call
+  // is failed.
+  EXPECT_EQ(kCreateInputContextMaxTrialCount,
+            mock_ibus_client_->create_input_context_call_count());
   // |set_capabilities_call_count()| should be zero since a context is not
   // created yet.
   EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that ui::InputMethodIBus does not crash even if ibus-daemon does not
@@ -442,24 +477,105 @@ TEST_F(InputMethodIBusTest, CreateContextNoResp) {
   ime_->Init(true);
   EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
   EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+}
+
+// Confirm that ui::InputMethodIBus does not crash even if ibus-daemon responds
+// after ui::InputMethodIBus is deleted.
+TEST_F(InputMethodIBusTest, CreateContextFailDelayed) {
+  CreateInputContextDelayHandler create_input_context_handler(
+      dbus::ObjectPath("Sample object path"));
+  mock_ibus_client_->set_create_input_context_handler(base::Bind(
+      &CreateInputContextDelayHandler::Run,
+      base::Unretained(&create_input_context_handler)));
+
+  chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
+  ime_->Init(true);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  ime_->SetFocusedTextInputClient(NULL);
+  ime_.reset();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  create_input_context_handler.RunCallback(false);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+}
+
+// Confirm that ui::InputMethodIBus does not crash even if ibus-daemon responds
+// after ui::InputMethodIBus is deleted.
+TEST_F(InputMethodIBusTest, CreateContextSuccessDelayed) {
+  CreateInputContextDelayHandler create_input_context_handler(
+      dbus::ObjectPath("Sample object path"));
+  mock_ibus_client_->set_create_input_context_handler(base::Bind(
+      &CreateInputContextDelayHandler::Run,
+      base::Unretained(&create_input_context_handler)));
+
+  chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
+  ime_->Init(true);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  ime_->SetFocusedTextInputClient(NULL);
+  ime_.reset();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  create_input_context_handler.RunCallback(true);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that ui::InputMethodIBus does not crash even if ibus-daemon responds
-// after ui::InputMethodIBus is deleted. See comments in ~MockIBusClient() as
-// well.
-TEST_F(InputMethodIBusTest, CreateContextDelayed) {
-  CreateInputContextDelayFailHandler create_input_context_handler;
+// after disconnected from ibus-daemon.
+TEST_F(InputMethodIBusTest, CreateContextSuccessDelayedAfterDisconnection) {
+  CreateInputContextDelayHandler create_input_context_handler(
+      dbus::ObjectPath("Sample object path"));
   mock_ibus_client_->set_create_input_context_handler(base::Bind(
-      &CreateInputContextDelayFailHandler::Run,
+      &CreateInputContextDelayHandler::Run,
       base::Unretained(&create_input_context_handler)));
 
   chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
   ime_->Init(true);
   EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
   EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
-  // After this line, the destructor for |ime_| will run first. Then, the
-  // destructor for the handler will run. In the latter function, a new input
-  // context will be created and passed to InitOrAbandonInputContext().
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  ime_->OnDisconnected();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  create_input_context_handler.RunCallback(true);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+}
+
+// Confirm that ui::InputMethodIBus does not crash even if ibus-daemon responds
+// after disconnected from ibus-daemon.
+TEST_F(InputMethodIBusTest, CreateContextFailDelayedAfterDisconnection) {
+  CreateInputContextDelayHandler create_input_context_handler(
+      dbus::ObjectPath("Sample object path"));
+  mock_ibus_client_->set_create_input_context_handler(base::Bind(
+      &CreateInputContextDelayHandler::Run,
+      base::Unretained(&create_input_context_handler)));
+
+  chromeos::DBusThreadManager::Get()->InitIBusBus("dummy address");
+  ime_->Init(true);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  ime_->OnDisconnected();
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
+
+  create_input_context_handler.RunCallback(false);
+  EXPECT_EQ(1, mock_ibus_client_->create_input_context_call_count());
+  EXPECT_EQ(0, mock_ibus_input_context_client_->set_capabilities_call_count());
+  EXPECT_FALSE(mock_ibus_input_context_client_->IsObjectProxyReady());
 }
 
 // Confirm that IBusClient::FocusIn is called on "connected" if input_type_ is