Remove PendingCreateICRequest.

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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150517

[Yusuke Sato, 2012-08-06 17:48:35]

thanks for the cleanup.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:371: LOG(ERROR) << "Input context is already
created or waiting ibus-daemon"
no more LOG(ERROR) please. see my comment below.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:879: if (create_input_context_fail_count_++ >=
10) {
be consistent, prefer ++create_input_context_fail_count_.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:879: if (create_input_context_fail_count_++ >=
10) {
10 should be replaced with const int kXXX;

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:880: LOG(ERROR) << "CreateInputContext is
failed even tried 10 times, Give up.";
using LOG(ERROR) is highly discouraged (check out style guide). replace it with
VLOG etc if possible.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.h (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:70: enum InputContextState {
add comments for each enum.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:180: InputContextState input_context_state_;
comment

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:182: int create_input_context_fail_count_;
comment

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus_unittest.cc (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus_unittest.cc:429: EXPECT_EQ(11,
mock_ibus_client_->create_input_context_call_count());
kXXX + 1

[Seigo Nonaka, 2012-08-07 04:28:29]

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:371: LOG(ERROR) << "Input context is already
created or waiting ibus-daemon"
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> no more LOG(ERROR) please. see my comment below.

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:879: if (create_input_context_fail_count_++ >=
10) {
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> 10 should be replaced with const int kXXX;

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:879: if (create_input_context_fail_count_++ >=
10) {
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> be consistent, prefer ++create_input_context_fail_count_.

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:880: LOG(ERROR) << "CreateInputContext is
failed even tried 10 times, Give up.";
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> using LOG(ERROR) is highly discouraged (check out style guide). replace it
with
> VLOG etc if possible.

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.h (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:70: enum InputContextState {
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> add comments for each enum.

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:180: InputContextState input_context_state_;
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> comment

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.h:182: int create_input_context_fail_count_;
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> comment

Done.

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus_unittest.cc (right):

http://codereview.chromium.org/10834175/diff/8001/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus_unittest.cc:429: EXPECT_EQ(11,
mock_ibus_client_->create_input_context_call_count());
On 2012/08/06 17:48:35, Yusuke Sato wrote:
> kXXX + 1

Done.

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

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:41: const int kCreateInputContextMaxTrialCount
= 10;
MaxRetryCount

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:373: << " response.";
nit: you can omit <<.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:378:
INPUT_CONTEXT_WAIT_CREATE_INPUT_CONTEXT_RESPONSE;
nit: no need to wrap?

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:854: << "Already input context is created.";
nit: I think this line does not add any information. Remove.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:857: // Corresponding to cancel abandon input
context under waiting
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().

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:873: << "Already input context is created.";
nit: remove.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:875: // CreateInputContext is failed but the
input context is no longer
s/is//

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:880: if (++create_input_context_fail_count_ >
kCreateInputContextMaxTrialCount) {
nit: >=, probably?

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:881: DVLOG(1) << "CreateInputContext is failed
even tried "
s/is//

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:883: return;
don't we have to reset |input_context_state_| here?

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

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:41: const int kCreateInputContextMaxTrialCount
= 10;
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> MaxRetryCount

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:373: << " response.";
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> nit: you can omit <<.

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:378:
INPUT_CONTEXT_WAIT_CREATE_INPUT_CONTEXT_RESPONSE;
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> nit: no need to wrap?

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:854: << "Already input context is created.";
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> nit: I think this line does not add any information. Remove.

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:857: // Corresponding to cancel abandon input
context under waiting
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:
> 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().

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:873: << "Already input context is created.";
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> nit: remove.

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:875: // CreateInputContext is failed but the
input context is no longer
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> s/is//

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:880: if (++create_input_context_fail_count_ >
kCreateInputContextMaxTrialCount) {
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> nit: >=, probably?

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:881: DVLOG(1) << "CreateInputContext is failed
even tried "
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> s/is//

Done.

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:883: return;
Yes, we need, done.
On 2012/08/07 06:11:47, Yusuke Sato wrote:
> don't we have to reset |input_context_state_| here?

[Yusuke Sato, 2012-08-07 20:03:43]

LGTM with some nits. thanks for adding tests!

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/9007/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:883: return;
On 2012/08/07 19:11:49, Seigo Nonaka wrote:
> Yes, we need, done.

looks like it's not yet done.

> On 2012/08/07 06:11:47, Yusuke Sato wrote:
> > don't we have to reset |input_context_state_| here?
>

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus.cc
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus.cc:856: // Corresponding to cancel abandon input
context under waiting
couldn't understand what this comment meant. please try to rephrase it..

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus.cc:880: << kMaxRetryCount << " times, give up.";
don't we need to reset the counter to zero?

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
File ui/base/ime/input_method_ibus_unittest.cc (right):

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus_unittest.cc:95: if (success) {
remove {}

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus_unittest.cc:97: } else {
remove }{

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus_unittest.cc:106: };
disallow copy&assign

http://codereview.chromium.org/10834175/diff/10/ui/base/ime/input_method_ibus...
ui/base/ime/input_method_ibus_unittest.cc:269: };
disallow copy&assign

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

Thank you for your review, I will submit after check try bots just in case.

https://chromiumcodereview.appspot.com/10834175/diff/9007/ui/base/ime/input_m...
File ui/base/ime/input_method_ibus.cc (right):

https://chromiumcodereview.appspot.com/10834175/diff/9007/ui/base/ime/input_m...
ui/base/ime/input_method_ibus.cc:883: return;
Sorry, forget to commit this change.
Updated.
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> On 2012/08/07 19:11:49, Seigo Nonaka wrote:
> > Yes, we need, done.
> 
> looks like it's not yet done.
> 
> > On 2012/08/07 06:11:47, Yusuke Sato wrote:
> > > don't we have to reset |input_context_state_| here?
> > 
>

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
File ui/base/ime/input_method_ibus.cc (right):

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus.cc:856: // Corresponding to cancel abandon input
context under waiting
Sorry forget to update this comment.
Done
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> couldn't understand what this comment meant. please try to rephrase it..

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus.cc:880: << kMaxRetryCount << " times, give up.";
Yes, we need.
Done.
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> don't we need to reset the counter to zero?

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
File ui/base/ime/input_method_ibus_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus_unittest.cc:95: if (success) {
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> remove {}

Done.

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus_unittest.cc:97: } else {
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> remove }{

Done.

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus_unittest.cc:106: };
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> disallow copy&assign

Done.

https://chromiumcodereview.appspot.com/10834175/diff/10/ui/base/ime/input_met...
ui/base/ime/input_method_ibus_unittest.cc:269: };
On 2012/08/07 20:03:44, Yusuke Sato wrote:
> disallow copy&assign

Done.

[commit-bot: I haz the power, 2012-08-08 06:03:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/10834175/13002

[commit-bot: I haz the power, 2012-08-08 08:35:39]

Change committed as 150517