Refactoring for the InputMethod & InputMethodDelegate interfaces.

Refactoring for the InputMethod & InputMethodDelegate interfaces.

1) DispatchKeyEvent returns void type instead of bool, as it is not used.
2) DispatchKeyEventPostIME returns EventDispatchDetails, so that the caller
   (InputMethod) can know whether its being destroyed after the call of
   DispatchKeyEventPostIME.
3) Both DispatchKeyEvent & DispatchKeyEventPostIME takes ui::KeyEvent* as
   the parameter, so that the stopped_propagation() state can be carried over.

Note: this cl doesn't try to change any existing behaviors/logics, except one thing:
The cl https://codereview.chromium.org/1236923003 may introduce a potential
regression which this cl tries to fix:
DesktopWindowTreeHostWin::DispatchKeyEventPostIME() always call
event->StopPropagation(). However, HWNDMessageHandler::OnKeyEvent() relies
on the event.handled() state to correctly call SetMsgHandled(FALSE).
This cl remove the StopPropagation() call in
DesktopWindowTreeHostWin::DispatchKeyEventPostIME() and let
InputMethodWin/RemoteInputMethodWin call it as appropriate.

TBR=sky@chromium.org
BUG=513917, 474828
TEST=Trybots green.

Committed: https://crrev.com/59c2a2cf79038dd2bcae5c45300a6a50de4ac9fd
Cr-Commit-Position: refs/heads/master@{#341704}

[Shu Chen, 2015-07-30 09:05:23]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 09:05:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/100001

[commit-bot: I haz the power, 2015-07-30 09:07:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 09:07:14]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Shu Chen, 2015-07-30 09:08:25]

shuchen@chromium.org changed reviewers:
+ oshima@chromium.org, sadrul@chromium.org, suzhe@chromium.org,
yukawa@chromium.org

[Shu Chen, 2015-07-30 09:08:26]

Guys, can you please review this cl?

yukawa@/suzhe@ for general IME related stuff.
sadrul@ for approval of ui/...
oshima@ for approval of ash/...

Thanks!

[Shu Chen, 2015-07-30 09:22:21]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 09:22:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/120001

[commit-bot: I haz the power, 2015-07-30 09:36:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 09:36:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2015-07-30 09:44:06]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 09:44:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/140001

[Shu Chen, 2015-07-30 09:59:27]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 09:59:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/160001

[commit-bot: I haz the power, 2015-07-30 10:42:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 10:42:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2015-07-30 15:26:38]

changes in ash looks mechanical, so I'll approve if yukawa@/suzhe@ are happy
with the change/design.

[yukawa, 2015-07-30 18:11:40]

Strategy sounds good to me.  I'm fine if James is OK for this change.

One minor comment for unittest.

https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
File ui/base/ime/remote_input_method_win_unittest.cc (right):

https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
ui/base/ime/remote_input_method_win_unittest.cc:520:
input_method->DispatchKeyEvent(&native_keydown);
Does it make sense to check |native_keydown.handled()|?  Ditto for other test
cases.

[Shu Chen, 2015-07-31 01:18:47]

https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
File ui/base/ime/remote_input_method_win_unittest.cc (right):

https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
ui/base/ime/remote_input_method_win_unittest.cc:520:
input_method->DispatchKeyEvent(&native_keydown);
On 2015/07/30 18:11:39, yukawa wrote:
> Does it make sense to check |native_keydown.handled()|?  Ditto for other test
> cases.

Yes, makes sense. Thanks for catching this.

[yukawa, 2015-07-31 01:25:44]

On 2015/07/31 01:18:47, Shu Chen wrote:
>
https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
> File ui/base/ime/remote_input_method_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/1257603006/diff/160001/ui/base/ime/remote_inp...
> ui/base/ime/remote_input_method_win_unittest.cc:520:
> input_method->DispatchKeyEvent(&native_keydown);
> On 2015/07/30 18:11:39, yukawa wrote:
> > Does it make sense to check |native_keydown.handled()|?  Ditto for other
test
> > cases.
> 
> Yes, makes sense. Thanks for catching this.

Thanks. lgtm.

[Shu Chen, 2015-07-31 03:11:08]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-31 03:11:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/200001

[commit-bot: I haz the power, 2015-07-31 05:32:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-31 05:32:06]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[oshima, 2015-07-31 09:38:34]

ash/ lgtm.

one question.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_delegate.h (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_delegate.h:15: namespace internal {
just curious. Calling an interface that meant to be implemented by a client
*internal* sounds strange. What's the rationale?

[James Su, 2015-07-31 11:43:58]

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:76:
SendFakeProcessKeyEvent(event->flags());
This method should return ui::EventDispatchDetails as well, and also need to
know if the faked event is stopped_propagation or not.

As you won't know how other components handle 229 key code.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:81: if (event->stopped_propagation()) {
needs to check details.target_destroyed here.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:124: if (!filtered) {
just noticed that we send nothing if a key up event is filtered. Is it expected?

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:128: if (event->stopped_propagation()) {
needs to check details.target_destroyed here.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:349: ProcessFilteredKeyPressEvent(event);
I think the result of DispatchKeyEventPostIME() call shouldn't be ignored here.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:394:
ignore_result(DispatchKeyEventPostIME(event));
I think this result shouldn't be ignored.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:467:
ignore_result(DispatchKeyEventPostIME(&evt));
I think this result shouldn't be ignored, and evt.stopped_propagation() should
also be checked everywhere.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_win.cc:103: return DispatchFabricatedKeyEvent(event);
This looks weird, as this method doesn't have return value.

https://codereview.chromium.org/1257603006/diff/200001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1523:
event->StopPropagation();
remove this line?

[Shu Chen, 2015-08-03 01:44:48]

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:76:
SendFakeProcessKeyEvent(event->flags());
On 2015/07/31 11:43:57, James Su wrote:
> This method should return ui::EventDispatchDetails as well, and also need to
> know if the faked event is stopped_propagation or not.
> 
> As you won't know how other components handle 229 key code.

Done.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:81: if (event->stopped_propagation()) {
On 2015/07/31 11:43:58, James Su wrote:
> needs to check details.target_destroyed here.

Done.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:124: if (!filtered) {
On 2015/07/31 11:43:57, James Su wrote:
> just noticed that we send nothing if a key up event is filtered. Is it
expected?

This behavior has not been changed for years.

What is the expected behavior?

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:128: if (event->stopped_propagation()) {
On 2015/07/31 11:43:57, James Su wrote:
> needs to check details.target_destroyed here.

Done.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:349: ProcessFilteredKeyPressEvent(event);
On 2015/07/31 11:43:58, James Su wrote:
> I think the result of DispatchKeyEventPostIME() call shouldn't be ignored
here.

ChromeOS uses InputMethodEventHandler so destruction of WTH won't cause
destruction of InputMethodChromeOS. So it is safe to not check
EventDispatchDetails::dispatcher_destroyed.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:394:
ignore_result(DispatchKeyEventPostIME(event));
On 2015/07/31 11:43:58, James Su wrote:
> I think this result shouldn't be ignored.

ditto.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_chromeos.cc:467:
ignore_result(DispatchKeyEventPostIME(&evt));
On 2015/07/31 11:43:58, James Su wrote:
> I think this result shouldn't be ignored, and evt.stopped_propagation() should
> also be checked everywhere.

ditto & Done.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_delegate.h (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_delegate.h:15: namespace internal {
On 2015/07/31 09:38:34, oshima wrote:
> just curious. Calling an interface that meant to be implemented by a client
> *internal* sounds strange. What's the rationale? 

This exists for years and may be due to historical reasons.

In the current code, the caller of this interface is within ui/base/ime/...
The implementations of this interface are in ash/... and aura::WindowTreeHost.
(Before the IMF refactoring, the implementation is in
wm::core::InputMethodEventFilter)

If you think the namespace causes confusion, I will remove it in a separated cl.

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_win.cc:103: return DispatchFabricatedKeyEvent(event);
On 2015/07/31 11:43:58, James Su wrote:
> This looks weird, as this method doesn't have return value.

Done.

https://codereview.chromium.org/1257603006/diff/200001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1257603006/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1523:
event->StopPropagation();
On 2015/07/31 11:43:58, James Su wrote:
> remove this line?

Done.

[James Su, 2015-08-03 06:58:55]

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:146: event->StopPropagation();
why this line is moved into this if block?

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:248: if (!event.stopped_propagation())
check target_destroyed?

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:267: if (!event.stopped_propagation())
ditto.

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:290: if (!event.stopped_propagation())
ditto

[Shu Chen, 2015-08-03 07:03:55]

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:146: event->StopPropagation();
On 2015/08/03 06:58:55, James Su wrote:
> why this line is moved into this if block?

Event is StopPropagation'ed only when TextInputClient::InsertChar/etc. method is
called.

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:248: if (!event.stopped_propagation())
On 2015/08/03 06:58:55, James Su wrote:
> check target_destroyed?

Done.

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:267: if (!event.stopped_propagation())
On 2015/08/03 06:58:55, James Su wrote:
> ditto.

Done.

https://codereview.chromium.org/1257603006/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:290: if (!event.stopped_propagation())
On 2015/08/03 06:58:55, James Su wrote:
> ditto

Done.

[James Su, 2015-08-03 07:10:21]

lgtm

[Shu Chen, 2015-08-04 01:25:08]

Pinging...

Sadrul, can you please take a look? Thanks

The non-ime related changes under ui/... requires your approval and most of them
are mechanical.

[Shu Chen, 2015-08-04 01:26:08]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-04 01:26:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/260001

[commit-bot: I haz the power, 2015-08-04 01:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 01:28:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Shu Chen, 2015-08-04 02:10:22]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-04 02:10:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/280001

[commit-bot: I haz the power, 2015-08-04 02:33:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 02:33:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2015-08-04 03:00:18]

The build failures and ui_base test failures on linux look related. Mind taking
a look at them?

[Shu Chen, 2015-08-04 03:31:00]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-04 03:31:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/300001

[Shu Chen, 2015-08-04 03:31:38]

On 2015/08/04 03:00:18, sadrul wrote:
> The build failures and ui_base test failures on linux look related. Mind
taking
> a look at them?

I've just fixed the failures. Can you please take another look? Thanks

[Shu Chen, 2015-08-04 03:47:59]

On 2015/08/04 03:31:38, Shu Chen wrote:
> On 2015/08/04 03:00:18, sadrul wrote:
> > The build failures and ui_base test failures on linux look related. Mind
> taking
> > a look at them?
> 
> I've just fixed the failures. Can you please take another look? Thanks

Btw, I used goto statement in input_method_auralinux.cc. Please let me know
whether it is a common practice in chromium.

[sadrul, 2015-08-04 03:52:46]

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:133: goto Exit;
I don't think this needs to use goto. Can we look at should_stop_propagation and
set StopPropagation() right after DispatchKeyEventPostIME()?

[Shu Chen, 2015-08-04 03:59:21]

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:133: goto Exit;
On 2015/08/04 03:52:46, sadrul wrote:
> I don't think this needs to use goto. Can we look at should_stop_propagation
and
> set StopPropagation() right after DispatchKeyEventPostIME()?

Do you mean like below? Please see my inline comment with "BUG".
  details = DispatchKeyEventPostIME(event);
  if (should_stop_propagation)
    event->StopPropagation();
  ...
  // BUG: stopped_propagation() flag maybe set by should_stop_propagation
instead of DispatchKeyEventPostIME().
  if (event->stopped_propagation() || details.target_destroyed) {
  ...

[commit-bot: I haz the power, 2015-08-04 04:46:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 04:46:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Su, 2015-08-04 05:02:48]

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:133: goto Exit;
How about:

if (details.dispatcher_destroyed) {
  // Do nothing
} else if (event->stopped_propagation() || details.target_destroyed) {
  ResetContext();
} else if (event->type() == ui::ET_KEY_PRESSED) {
  ...
}

On 2015/08/04 03:59:20, Shu Chen wrote:
> On 2015/08/04 03:52:46, sadrul wrote:
> > I don't think this needs to use goto. Can we look at should_stop_propagation
> and
> > set StopPropagation() right after DispatchKeyEventPostIME()?
> 
> Do you mean like below? Please see my inline comment with "BUG".
>   details = DispatchKeyEventPostIME(event);
>   if (should_stop_propagation)
>     event->StopPropagation();
>   ...
>   // BUG: stopped_propagation() flag maybe set by should_stop_propagation
> instead of DispatchKeyEventPostIME().
>   if (event->stopped_propagation() || details.target_destroyed) {
>   ...
>

[Shu Chen, 2015-08-04 05:09:31]

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/300001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:133: goto Exit;
On 2015/08/04 05:02:48, James Su wrote:
> How about:
> 
> if (details.dispatcher_destroyed) {
>   // Do nothing
> } else if (event->stopped_propagation() || details.target_destroyed) {
>   ResetContext();
> } else if (event->type() == ui::ET_KEY_PRESSED) {
>   ...
> }
> 
> On 2015/08/04 03:59:20, Shu Chen wrote:
> > On 2015/08/04 03:52:46, sadrul wrote:
> > > I don't think this needs to use goto. Can we look at
should_stop_propagation
> > and
> > > set StopPropagation() right after DispatchKeyEventPostIME()?
> > 
> > Do you mean like below? Please see my inline comment with "BUG".
> >   details = DispatchKeyEventPostIME(event);
> >   if (should_stop_propagation)
> >     event->StopPropagation();
> >   ...
> >   // BUG: stopped_propagation() flag maybe set by should_stop_propagation
> > instead of DispatchKeyEventPostIME().
> >   if (event->stopped_propagation() || details.target_destroyed) {
> >   ...
> > 
> 

Done & thanks for your suggestion.

[sadrul, 2015-08-04 07:15:59]

lgtm

https://codereview.chromium.org/1257603006/diff/320001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:130: // Do nothing when dipatcher is
destroyed.
This is fine for now, but can potentially lead to use-after-free if new code is
added below that uses |this|. So an early return from here would still be
preferable, even if that means dup'ing a couple of lines of code.

[Shu Chen, 2015-08-04 07:36:11]

https://codereview.chromium.org/1257603006/diff/320001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1257603006/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:130: // Do nothing when dipatcher is
destroyed.
On 2015/08/04 07:15:59, sadrul wrote:
> This is fine for now, but can potentially lead to use-after-free if new code
is
> added below that uses |this|. So an early return from here would still be
> preferable, even if that means dup'ing a couple of lines of code.

Thanks for the suggestion.
I was also trying to return earlier to avoid potential use-after-free, which is
why I used goto.
I've now made it duplicate 2 lines of code and return earlier.

[Shu Chen, 2015-08-04 07:36:45]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-08-04 07:36:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org,
oshima@chromium.org, suzhe@chromium.org
Link to the patchset: https://codereview.chromium.org/1257603006/#ps340001
(title: "addressed Sadrul's comment.")

[commit-bot: I haz the power, 2015-08-04 07:36:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/340001

[commit-bot: I haz the power, 2015-08-04 07:46:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 07:46:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Shu Chen, 2015-08-04 08:28:47]

TBR'ing sky@ for mechanical changes of mandoline.

[Shu Chen, 2015-08-04 08:28:55]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-08-04 08:29:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257603006/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257603006/340001

[commit-bot: I haz the power, 2015-08-04 09:48:43]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-08-04 09:49:24]

Patchset 18 (id:??) landed as
https://crrev.com/59c2a2cf79038dd2bcae5c45300a6a50de4ac9fd
Cr-Commit-Position: refs/heads/master@{#341704}