IME for Mus: Send ack for key events after IME driver processes the event.

IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
Committed: https://crrev.com/a2a7eed25d00d069b36bba9a91de4e1c1473a62e
Cr-Commit-Position: refs/heads/master@{#425701}

[Hadi, 2016-10-11 19:59:16]

Description was changed from

==========
IME for Mus: Send event ack after ime processes the event.

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

to

==========
IME for Mus: Send event ack after ime processes the event.

BUG=641355
==========

[Hadi, 2016-10-12 14:54:46]

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

[commit-bot: I haz the power, 2016-10-12 14:55:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2016-10-12 15:05:22]

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

[commit-bot: I haz the power, 2016-10-12 15:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2016-10-12 15:12:58]

Description was changed from

==========
IME for Mus: Send event ack after ime processes the event.

BUG=641355
==========

to

==========
IME for Mus: Send event ack after ime processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

[Hadi, 2016-10-12 15:13:46]

Description was changed from

==========
IME for Mus: Send event ack after ime processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

to

==========
IME for Mus: Send ack for key events after ime driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

[Hadi, 2016-10-12 15:14:03]

Description was changed from

==========
IME for Mus: Send ack for key events after ime driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

to

==========
IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

[Hadi, 2016-10-12 15:14:42]

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

[Hadi, 2016-10-12 15:14:42]

PTAL.

[commit-bot: I haz the power, 2016-10-12 15:34:45]

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

[commit-bot: I haz the power, 2016-10-12 15:34:46]

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, 2016-10-12 16:01:07]

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

[commit-bot: I haz the power, 2016-10-12 16:01:32]

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, 2016-10-12 17:30:22]

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

[commit-bot: I haz the power, 2016-10-12 17:30:23]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-10-13 16:36:29]

https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/input_meth...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/input_meth...
ui/views/mus/input_method_mus.cc:64: new
base::Callback<void(EventResult)>(base::Bind(&DoNothing))));
Send nullptr instead?

https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/text_input...
File ui/views/mus/text_input_client_impl.h (right):

https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/text_input...
ui/views/mus/text_input_client_impl.h:23:
TextInputClientImpl(ui::TextInputClient* text_input_client);
explicit

[Hadi, 2016-10-13 17:35:36]

Addressed both of the points.

On 2016/10/13 16:36:29, sadrul wrote:
>
https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/input_meth...
> File ui/views/mus/input_method_mus.cc (right):
> 
>
https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/input_meth...
> ui/views/mus/input_method_mus.cc:64: new
> base::Callback<void(EventResult)>(base::Bind(&DoNothing))));
> Send nullptr instead?
> 
>
https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/text_input...
> File ui/views/mus/text_input_client_impl.h (right):
> 
>
https://codereview.chromium.org/2412593002/diff/80001/ui/views/mus/text_input...
> ui/views/mus/text_input_client_impl.h:23:
> TextInputClientImpl(ui::TextInputClient* text_input_client);
> explicit

[sadrul, 2016-10-14 17:39:16]

lgtm for now. But I worry that we may be hitting the 1s ack-timeout when
chrome-extension IME is hooked up. So let's keep an eye on it, and continue to
think if a different approach that would allow early-acks.

[sky, 2016-10-14 22:15:52]

LGTM with the following addressed.

https://codereview.chromium.org/2412593002/diff/100001/services/ui/ime/test_i...
File services/ui/ime/test_ime_driver/test_ime_driver.cc (right):

https://codereview.chromium.org/2412593002/diff/100001/services/ui/ime/test_i...
services/ui/ime/test_ime_driver/test_ime_driver.cc:37: };
no ';' here

https://codereview.chromium.org/2412593002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2412593002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:60: void InputMethodMus::DispatchKeyEvent(
Definition and declaration order should match (see style guide).

https://codereview.chromium.org/2412593002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.h (right):

https://codereview.chromium.org/2412593002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.h:47: void DispatchKeyEvent(
move above overrides?

[Hadi, 2016-10-17 14:55:11]

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

[commit-bot: I haz the power, 2016-10-17 14:55:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2016-10-17 14:56:55]

moshayedi@chromium.org changed reviewers:
+ tsepez@chromium.org

[Hadi, 2016-10-17 14:56:56]

Addressed feedback.

tsepez@ can you please review ime.mojom as SECURITY_OWNERS?

[commit-bot: I haz the power, 2016-10-17 15:44:23]

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

[commit-bot: I haz the power, 2016-10-17 15:44:24]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-17 16:23:22]

lgtm

[Hadi, 2016-10-17 16:40:18]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-10-17 16:40:18]

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

[commit-bot: I haz the power, 2016-10-17 16:40:42]

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, 2016-10-17 16:45:20]

Description was changed from

==========
IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

to

==========
IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

[commit-bot: I haz the power, 2016-10-17 16:45:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-17 16:48:02]

Description was changed from

==========
IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355
==========

to

==========
IME for Mus: Send ack for key events after IME driver processes the event.

Before this CL, we sent event ack back to window server without waiting to see
if IME could handle the event or not. This caused some problems with post-target
accelerators (see bug description for more details). This CL fixes the issue by
delaying the ack until IME driver tells the client if it could handle the event
or not.

BUG=641355

Committed: https://crrev.com/a2a7eed25d00d069b36bba9a91de4e1c1473a62e
Cr-Commit-Position: refs/heads/master@{#425701}
==========

[commit-bot: I haz the power, 2016-10-17 16:48:04]

Patchset 7 (id:??) landed as
https://crrev.com/a2a7eed25d00d069b36bba9a91de4e1c1473a62e
Cr-Commit-Position: refs/heads/master@{#425701}