Remove PendingCreateICRequest.

ui/base/ime/input_method_ibus.cc

Index: ui/base/ime/input_method_ibus.cc
diff --git a/ui/base/ime/input_method_ibus.cc b/ui/base/ime/input_method_ibus.cc
index 7fd48b2db3c1e38067397b6cdba8d9ace72d3b04..2e602f6fe029cb231d5d1b47a106651bbde3ae5c 100644
--- a/ui/base/ime/input_method_ibus.cc
+++ b/ui/base/ime/input_method_ibus.cc
@@ -38,6 +38,7 @@ namespace {
 
 const int kIBusReleaseMask = 1 << 30;
 const char kClientName[] = "chrome";
+const int kCreateInputContextMaxTrialCount = 10;

[Yusuke Sato, 2012/08/07 06:11:47]

MaxRetryCount

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

 
 // 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.";

[Yusuke Sato, 2012/08/07 06:11:47]

nit: you can omit <<.

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+    return;
+  }
+
+  input_context_state_ =
+      INPUT_CONTEXT_WAIT_CREATE_INPUT_CONTEXT_RESPONSE;

[Yusuke Sato, 2012/08/07 06:11:47]

nit: no need to wrap?

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

 
   // Creates the input context asynchronously.
   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,47 @@ void InputMethodIBus::ResetInputContext() {
 }
 
 void InputMethodIBus::CreateInputContextDone(
-    PendingCreateICRequest* ic_request,
     const dbus::ObjectPath& object_path) {
+  DCHECK_NE(INPUT_CONTEXT_RUNNING, input_context_state_)
+      << "Already input context is created.";

[Yusuke Sato, 2012/08/07 06:11:47]

nit: I think this line does not add any information. Remove.

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+
+  if (input_context_state_ == INPUT_CONTEXT_STOP) {
+    // Corresponding to cancel abandon input context under waiting

[Yusuke Sato, 2012/08/07 06:11:47]

not 100% sure, but there might be a resource leak here.

1. CreateContext is called
2. DestroyContext is called, but in the function ResetInputContext is not called
3. CreateInputContextDone is called.

In this case, both 2 and 3 don't GetInputContextClient()->ResetObjectProxy().

[Seigo Nonaka, 2012/08/07 19:11:49]

I think your pointed case never happens, because CreateContext emit
CreateInputContext message only if the InputContext is not ready.
On the other hand, DestroyContext calls ResetInputContext only the InputContext
is ready.

However I added some expectation into unittest to check InputContextState, just
in case.


On 2012/08/07 06:11:47, Yusuke Sato wrote:

+    // CreateInputContext response from ibus-daemon. Do nothing.
+    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_)
+      << "Already input context is created.";

[Yusuke Sato, 2012/08/07 06:11:47]

nit: remove.

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+  if (input_context_state_ == INPUT_CONTEXT_STOP) {
+    // CreateInputContext is failed but the input context is no longer

[Yusuke Sato, 2012/08/07 06:11:47]

s/is//

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+    // necessary, thus do nothing.
+    return;
+  }
+
+  if (++create_input_context_fail_count_ > kCreateInputContextMaxTrialCount) {

[Yusuke Sato, 2012/08/07 06:11:47]

nit: >=, probably?

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+    DVLOG(1) << "CreateInputContext is failed even tried "

[Yusuke Sato, 2012/08/07 06:11:47]

s/is//

[Seigo Nonaka, 2012/08/07 19:11:49]

Done.

+             << kCreateInputContextMaxTrialCount << " times, give up.";
+    return;

[Yusuke Sato, 2012/08/07 06:11:47]

don't we have to reset |input_context_state_| here?

[Seigo Nonaka, 2012/08/07 19:11:49]

Yes, we need, done.
On 2012/08/07 06:11:47, Yusuke Sato wrote:

[Yusuke Sato, 2012/08/07 20:03:44]

looks like it's not yet done.

[Seigo Nonaka, 2012/08/08 04:32:52]

Sorry, forget to commit this change.
Updated.
On 2012/08/07 20:03:44, Yusuke Sato wrote:

+  }
+
+  // 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() {