Fix crash bug of IME extension API.

Fix crash bug of IME extension API.

BUG=147526, 147539, 149189
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158527

[Seigo Nonaka, 2012-09-24 04:16:58]

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
File chromeos/dbus/ibus/ibus_engine_service.cc (right):

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.cc:395: //////// do we need to check
engine_handler_?
remove this. And I think the answer is yes, because it corresponds to
ExtensionIME is removed under keyevent handling. If possible, please write the
testcase for them.

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.cc:476: IBusEngineHandlerInterface
*engine_handler_;
IBusEngineHandlerInterfae* engine_handler_

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Set a new IBus engine client
and old client will be overrided.
/Set/Sets/ /overrided/overridden/

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.h:137: // Unset a current IBus engine
client.
Unsets

[hsumita, 2012-09-24 07:32:59]

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
File chromeos/dbus/ibus/ibus_engine_service.cc (right):

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.cc:395: //////// do we need to check
engine_handler_?
On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> remove this. And I think the answer is yes, because it corresponds to
> ExtensionIME is removed under keyevent handling. If possible, please write the
> testcase for them.

Done.

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.cc:476: IBusEngineHandlerInterface
*engine_handler_;
On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> IBusEngineHandlerInterfae* engine_handler_

Done.

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Set a new IBus engine client
and old client will be overrided.
On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> /Set/Sets/ /overrided/overridden/

Done.

https://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engin...
chromeos/dbus/ibus/ibus_engine_service.h:137: // Unset a current IBus engine
client.
On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> Unsets

Done.

[Seigo Nonaka, 2012-09-24 07:38:45]

http://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engine...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

http://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engine...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Set a new IBus engine client
and old client will be overrided.
Seems not fixed, please revise /overrided/overridden/
On 2012/09/24 07:32:59, hsumita wrote:
> On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> > /Set/Sets/ /overrided/overridden/
> 
> Done.

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:135: virtual void
SetEngine(IBusEngineHandlerInterface* handler) = 0;
I think you may want to update mock_ibus_engine_service.h too.

[Seigo Nonaka, 2012-09-24 07:52:45]

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
File chromeos/dbus/ibus/ibus_engine_service.cc (right):

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.cc:135: LOG_IF(ERROR, engine_handler_ ==
NULL) << "Override engine.";
DLOG? and is the comment correct? And should we treat as error?
I think DVLOG_IF(1) is fine for this.

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Sets a new IBus engine client
and old client will be overrided.
pls also /client/handler/

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:137: // Unsets a current IBus engine
client.
/client/handler/

[hsumita, 2012-09-24 08:01:16]

http://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engine...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

http://codereview.chromium.org/10968056/diff/1/chromeos/dbus/ibus/ibus_engine...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Set a new IBus engine client
and old client will be overrided.
It should be fixed on patchset 1.

On 2012/09/24 07:38:45, Seigo Nonaka wrote:
> Seems not fixed, please revise /overrided/overridden/
> On 2012/09/24 07:32:59, hsumita wrote:
> > On 2012/09/24 04:16:58, Seigo Nonaka wrote:
> > > /Set/Sets/ /overrided/overridden/
> > 
> > Done.
>

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
File chromeos/dbus/ibus/ibus_engine_service.cc (right):

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.cc:135: LOG_IF(ERROR, engine_handler_ ==
NULL) << "Override engine.";
On 2012/09/24 07:52:45, Seigo Nonaka wrote:
> DLOG? and is the comment correct? And should we treat as error?
> I think DVLOG_IF(1) is fine for this.

Done.

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
File chromeos/dbus/ibus/ibus_engine_service.h (right):

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:133: // Sets a new IBus engine client
and old client will be overrided.
On 2012/09/24 07:52:45, Seigo Nonaka wrote:
> pls also /client/handler/

Done.

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:135: virtual void
SetEngine(IBusEngineHandlerInterface* handler) = 0;
On 2012/09/24 07:38:45, Seigo Nonaka wrote:
> I think you may want to update mock_ibus_engine_service.h too.

Done.

http://codereview.chromium.org/10968056/diff/4003/chromeos/dbus/ibus/ibus_eng...
chromeos/dbus/ibus/ibus_engine_service.h:137: // Unsets a current IBus engine
client.
On 2012/09/24 07:52:45, Seigo Nonaka wrote:
> /client/handler/

Done.

[Seigo Nonaka, 2012-09-24 08:07:18]

lgtm

[hsumita, 2012-09-24 08:15:24]

Thanks!

I added satorux@ as a owner of chromeos.

[satorux1, 2012-09-24 14:34:03]

LGTM.

nona@, please add OWNERS to chrome/browser/chromeos/input_method and
chromeos/dbus/ibus, that contains nona@chromium.org and satorux@chromium.org, so
you can give owner approval.

[Seigo Nonaka, 2012-09-24 17:51:24]

Sure, I will do.
On 2012/09/24 14:34:03, satorux1 wrote:
> LGTM.
> 
> nona@, please add OWNERS to chrome/browser/chromeos/input_method and
> chromeos/dbus/ibus, that contains mailto:nona@chromium.org and
mailto:satorux@chromium.org, so
> you can give owner approval.

[commit-bot: I haz the power, 2012-09-25 01:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hsumita@chromium.org/10968056/4005

[commit-bot: I haz the power, 2012-09-25 05:01:20]

Change committed as 158527