Remove InputMethodEventHandler.

Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().


This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269
Review-Url: https://codereview.chromium.org/2872343003
Cr-Commit-Position: refs/heads/master@{#472941}
Committed: https://chromium.googlesource.com/chromium/src/+/9f9dddf904a9eae86f20d0fa3f68c1881a360d80

[Hadi, 2017-05-11 15:05:30]

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

[commit-bot: I haz the power, 2017-05-11 15:06:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-11 15:27:43]

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

[commit-bot: I haz the power, 2017-05-11 15:28:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 16:09:39]

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

[commit-bot: I haz the power, 2017-05-11 16:09:40]

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

[Hadi, 2017-05-11 17:24:55]

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

[commit-bot: I haz the power, 2017-05-11 17:25:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-11 17:49:50]

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

[commit-bot: I haz the power, 2017-05-11 17:50:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 18:34:53]

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

[commit-bot: I haz the power, 2017-05-11 18:34:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hadi, 2017-05-15 12:54:23]

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

[commit-bot: I haz the power, 2017-05-15 12:54:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 13:28:57]

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

[commit-bot: I haz the power, 2017-05-15 13:28:58]

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

[Hadi, 2017-05-15 13:45:43]

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

[commit-bot: I haz the power, 2017-05-15 13:46:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-15 14:23:39]

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

[commit-bot: I haz the power, 2017-05-15 14:24:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-15 14:31:41]

Description was changed from

==========
Dispatch key events to input method in WindowEventDispatcher.

BUG=NONE
==========

to

==========
Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().

This also adds ui::KeyEvent::set_skip_ime() to be used by events that are
not handled by Input Method and sent to DispatchKeyEventPostIME(). This
can happen asynchronously. These key events are processed again, this time
skipping Input Method.

This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269
==========

[Hadi, 2017-05-15 14:33:11]

moshayedi@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[Hadi, 2017-05-15 14:33:12]

PTAL. Thanks.

Next step would be SendEventToSink() for key events in WindowTreeClient.

[sadrul, 2017-05-15 14:44:25]

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_platform.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_platform.cc:119: return SendEventToSink(event);
Do you still need this override?

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_x11.cc:215: ui::EventDispatchDetails
AshWindowTreeHostX11::DispatchKeyEventPostIME(
Do you still need this override?

https://codereview.chromium.org/2872343003/diff/140001/ash/mus/ash_window_tre...
File ash/mus/ash_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/mus/ash_window_tre...
ash/mus/ash_window_tree_host_mus.cc:90: return SendEventToSink(event);
ditto

https://codereview.chromium.org/2872343003/diff/140001/ui/base/ime/input_meth...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ui/base/ime/input_meth...
ui/base/ime/input_method_base.cc:136: event->set_skip_ime(true);
Instead of turning the flag on here, can WindowTreeHost notify that
WindowEventDispatcher whether or not to dispatch the key-event to ime?

https://codereview.chromium.org/2872343003/diff/140001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2872343003/diff/140001/ui/events/event.h#newc...
ui/events/event.h:924: bool should_skip_ime() const { return skip_ime_; }
Just want to note that we have previously used event-flags to track whether a
KeyEvent needs to go through IME or not.

https://codereview.chromium.org/2872343003/diff/140001/ui/events/event.h#newc...
ui/events/event.h:964: bool skip_ime_ = false;
Does this need to be retained when dispatching events over mojo?

[commit-bot: I haz the power, 2017-05-15 16:45:11]

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

[commit-bot: I haz the power, 2017-05-15 16:45:12]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-05-15 17:14:54]

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

[commit-bot: I haz the power, 2017-05-15 17:15:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-15 17:20:22]

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

[Hadi, 2017-05-15 17:20:48]

Description was changed from

==========
Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().

This also adds ui::KeyEvent::set_skip_ime() to be used by events that are
not handled by Input Method and sent to DispatchKeyEventPostIME(). This
can happen asynchronously. These key events are processed again, this time
skipping Input Method.

This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269
==========

to

==========
Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().


This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269
==========

[commit-bot: I haz the power, 2017-05-15 17:21:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-15 17:22:13]

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_platform.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_platform.cc:119: return SendEventToSink(event);
On 2017/05/15 14:44:24, sadrul wrote:
> Do you still need this override?

Done.

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_x11.cc:215: ui::EventDispatchDetails
AshWindowTreeHostX11::DispatchKeyEventPostIME(
On 2017/05/15 14:44:24, sadrul wrote:
> Do you still need this override?

Done.

https://codereview.chromium.org/2872343003/diff/140001/ash/mus/ash_window_tre...
File ash/mus/ash_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ash/mus/ash_window_tre...
ash/mus/ash_window_tree_host_mus.cc:90: return SendEventToSink(event);
On 2017/05/15 14:44:24, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/2872343003/diff/140001/ui/base/ime/input_meth...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2872343003/diff/140001/ui/base/ime/input_meth...
ui/base/ime/input_method_base.cc:136: event->set_skip_ime(true);
On 2017/05/15 14:44:24, sadrul wrote:
> Instead of turning the flag on here, can WindowTreeHost notify that
> WindowEventDispatcher whether or not to dispatch the key-event to ime?

Done.

[commit-bot: I haz the power, 2017-05-15 18:10:59]

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

[commit-bot: I haz the power, 2017-05-15 18:11:00]

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

[sadrul, 2017-05-15 18:14:54]

The try failures look related. Mind taking a look at those too?

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/test/aura_test...
File ui/aura/test/aura_test_utils.cc (right):

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/test/aura_test...
ui/aura/test/aura_test_utils.cc:28: void disable_ime() {
host_->dispatcher_->set_skip_ime(true); }
I think this can break after the first key event dispatch? See comment in
WindowTreeHost.

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: ui::EventDispatchDetails
WindowTreeHost::DispatchKeyEventPostIME(
Should WindowTreeHost::DispatchKeyEventPostIME() be marked 'final'?

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:199: dispatcher_->set_skip_ime(false);
Check |details.dispatcher_destroyed| before using |dispatcher_| after call to
SendEventToSink().

Also, it looks like test::DisableIME() would break after the first key event
dispatch. Should you skip this if skip_ime_ was already set when entering this
function?

[Hadi, 2017-05-16 14:21:35]

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

[commit-bot: I haz the power, 2017-05-16 14:22:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-16 14:44:59]

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

[commit-bot: I haz the power, 2017-05-16 14:45:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-16 14:48:41]

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

[commit-bot: I haz the power, 2017-05-16 14:49:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-16 14:53:59]

Addressed feedback + fixed tests.

Also, EventGenerator previously manually sent events to Input Method since we
didn't have ash pre-handlers in views_unittests etc. Now, we don't need to do
that anymore, therefore I also removed manual IME dispatch from EventGenerator.

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:195: ui::EventDispatchDetails
WindowTreeHost::DispatchKeyEventPostIME(
On 2017/05/15 18:14:54, sadrul wrote:
> Should WindowTreeHost::DispatchKeyEventPostIME() be marked 'final'?

Done.

https://codereview.chromium.org/2872343003/diff/180001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:199: dispatcher_->set_skip_ime(false);
On 2017/05/15 18:14:54, sadrul wrote:
> Check |details.dispatcher_destroyed| before using |dispatcher_| after call to
> SendEventToSink().

Done.

> Also, it looks like test::DisableIME() would break after the first key event
> dispatch. Should you skip this if skip_ime_ was already set when entering this
> function?

If IME is disabled (i.e. skip_ime is previously true), execution shouldn't reach
here. Added a DCHECK() for that.

[commit-bot: I haz the power, 2017-05-16 16:40:24]

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

[commit-bot: I haz the power, 2017-05-16 16:40:25]

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

[Hadi, 2017-05-16 17:35:31]

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

[commit-bot: I haz the power, 2017-05-16 17:36:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-16 18:19:35]

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

[commit-bot: I haz the power, 2017-05-16 18:21:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 20:59:31]

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

[commit-bot: I haz the power, 2017-05-16 20:59:33]

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

[Hadi, 2017-05-17 12:21:15]

PTAL. win_chromium_rel_ng failures seem to be not relevant to this issue as they
are failing in some other CLs too.

[moshayedi, 2017-05-17 21:54:28]

On 2017/05/17 12:21:15, Hadi wrote:
> PTAL. win_chromium_rel_ng failures seem to be not relevant to this issue as
they
> are failing in some other CLs too.

ping.

[sadrul, 2017-05-17 21:59:15]

lgtm

[moshayedi, 2017-05-18 14:15:42]

moshayedi@google.com changed reviewers:
+ moshayedi@google.com, reillyg@chromium.org

[moshayedi, 2017-05-18 14:15:43]

sky@ please review ash/*.
reillyg@ please review extensions/*.

[Reilly Grant (use Gerrit), 2017-05-18 15:40:43]

extensions lgtm

[sky, 2017-05-18 16:55:08]

LGTM

https://codereview.chromium.org/2872343003/diff/280001/ui/aura/test/aura_test...
File ui/aura/test/aura_test_utils.cc (right):

https://codereview.chromium.org/2872343003/diff/280001/ui/aura/test/aura_test...
ui/aura/test/aura_test_utils.cc:48: WindowTreeHostTestApi host_test_api(host);
optional: don't bother with the local, e.g.
WindowTreeHostTestApi(host).disable_ime();

[Hadi, 2017-05-18 17:00:05]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-05-18 17:00:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
reillyg@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2872343003/#ps300001
(title: "addressed feedback.")

[commit-bot: I haz the power, 2017-05-18 17:01:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-18 19:30:27]

The CQ bit was unchecked by moshayedi@chromium.org

[Hadi, 2017-05-18 19:30:32]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-05-18 19:31:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 21:49:35]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495135832415780,
"parent_rev": "1334d4270679548dba126bca749e54bb4232310a", "commit_rev":
"9f9dddf904a9eae86f20d0fa3f68c1881a360d80"}

[commit-bot: I haz the power, 2017-05-18 21:49:48]

Description was changed from

==========
Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().


This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269
==========

to

==========
Remove InputMethodEventHandler.

This change removes InputMethodEventHandler and instead sends the events
to IME in WindowEventDispatcher::PreDispatchEvent().


This is a step towards unifying key event processing path in classic ash and
mus+ash.

BUG=719269

Review-Url: https://codereview.chromium.org/2872343003
Cr-Commit-Position: refs/heads/master@{#472941}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f9dddf904a9eae86f20d0fa3f68...
==========

[commit-bot: I haz the power, 2017-05-18 21:49:49]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/9f9dddf904a9eae86f20d0fa3f68...

[yawano, 2017-05-26 03:26:16]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2909603002/ by yawano@chromium.org.

The reason for reverting is: This CL makes TalkBack in Android window unusable.
The issue is that after this CL, key event becomes to go through event rewriter
in Chrome. TalkBack doesn't expect that events are rewritten by Chrome..