InputMethod should stop TextInputClient::InsertChar/InsertText calls when the event is stopped prop…

InputMethod should stop TextInputClient::InsertChar/InsertText calls when the event is stopped propagation after DispatchKeyEventPostIME().

BUG=502113, 476893
TEST=Verified on pixel device.

Committed: https://crrev.com/dea2093b666c5a28af5b2128f999c54c6fb79bd3
Cr-Commit-Position: refs/heads/master@{#338657}

[Shu Chen, 2015-06-25 08:30:27]

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

[Shu Chen, 2015-06-25 08:50:33]

shuchen@chromium.org changed reviewers:
+ yukawa@chromium.org

[Shu Chen, 2015-06-25 08:50:34]

+yukawa@.

[James Su, 2015-06-25 09:45:30]

Better add a unit test.

[James Su, 2015-06-25 13:38:23]

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
I'd still prefer to change this method parameter to ui::KeyEvent*.

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:205: return copied_event.stopped_propagation();
And we can return ui::EventDispatchDetails. Above DCHECK can also be removed.

[Shu Chen, 2015-06-26 02:05:20]

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:200: bool
WindowTreeHost::DispatchKeyEventPostIME(const ui::KeyEvent& event) {
On 2015/06/25 13:38:23, James Su wrote:
> I'd still prefer to change this method parameter to ui::KeyEvent*. 

This can be a separated cl. Let's keep the cl as small as possible.

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:205: return copied_event.stopped_propagation();
On 2015/06/25 13:38:23, James Su wrote:
> And we can return ui::EventDispatchDetails. Above DCHECK can also be removed.

Ditto.

[Shu Chen, 2015-06-26 03:36:56]

On 2015/06/25 09:45:30, James Su wrote:
> Better add a unit test.

Done.

[James Su, 2015-06-26 03:41:26]

LGTM

https://codereview.chromium.org/1209913002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/1209913002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:411: prev_client->InsertChar(ch,
event.flags());
this change is not necessary.

[Shu Chen, 2015-06-26 04:00:00]

https://codereview.chromium.org/1209913002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/1209913002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:411: prev_client->InsertChar(ch,
event.flags());
On 2015/06/26 03:41:26, James Su wrote:
> this change is not necessary.

Done.

[Shu Chen, 2015-06-26 06:22:53]

Pinging...

Sadrul/Yohei, can you please take a look? Thanks

[yukawa, 2015-06-26 06:30:23]

Hmm, I'm not familiar with Chrome OS IME handling.  I'm fine as long as James
and Sadrul are fine.

https://codereview.chromium.org/1209913002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos_unittest.cc (right):

https://codereview.chromium.org/1209913002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos_unittest.cc:333: bool
stop_propagation_post_ime;
nit: bool stop_propagation_post_ime_;

[Shu Chen, 2015-06-26 06:36:02]

On 2015/06/26 06:30:23, yukawa wrote:
> Hmm, I'm not familiar with Chrome OS IME handling.  I'm fine as long as James
> and Sadrul are fine.
> 

Thanks.

Sadrul, please note that this CL is not the key cl to fix bug 502113 & 476893.
However, it is still important to make InputMethod respect the stop-propagation
result from DispatchKeyEventPostIME.

I can be responsible for the changes of input method stuff. But the change of
ui/aura/window_tree_host.cc requires your approval.

Thanks!

[Shu Chen, 2015-06-26 06:37:41]

https://codereview.chromium.org/1209913002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos_unittest.cc (right):

https://codereview.chromium.org/1209913002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos_unittest.cc:333: bool
stop_propagation_post_ime;
On 2015/06/26 06:30:23, yukawa wrote:
> nit: bool stop_propagation_post_ime_;

Done.

[sadrul, 2015-06-27 03:06:29]

lgtm

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:205: return copied_event.stopped_propagation();
On 2015/06/26 02:05:20, Shu Chen wrote:
> On 2015/06/25 13:38:23, James Su wrote:
> > And we can return ui::EventDispatchDetails. Above DCHECK can also be
removed.
> 
> Ditto.

Can you please add a TODO here for both of these (and maybe file a bug and
reference that from the comment here)? Thanks.

[Shu Chen, 2015-06-27 09:43:56]

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/1209913002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:205: return copied_event.stopped_propagation();
On 2015/06/27 03:06:28, sadrul wrote:
> On 2015/06/26 02:05:20, Shu Chen wrote:
> > On 2015/06/25 13:38:23, James Su wrote:
> > > And we can return ui::EventDispatchDetails. Above DCHECK can also be
> removed.
> > 
> > Ditto.
> 
> Can you please add a TODO here for both of these (and maybe file a bug and
> reference that from the comment here)? Thanks.

The TODO has already been added in the DeliverEventToProcessor() method.

[Shu Chen, 2015-06-27 09:44:10]

shuchen@chromium.org changed reviewers:
- yukawa@chromium.org

[Shu Chen, 2015-06-27 10:31:39]

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

[Shu Chen, 2015-06-27 10:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1209913002/#ps80001
(title: "fixed test failures.")

[commit-bot: I haz the power, 2015-06-27 10:31:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209913002/80001

[commit-bot: I haz the power, 2015-06-27 11:13:34]

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

[commit-bot: I haz the power, 2015-06-27 11:13:35]

Dry run: Exceeded global retry quota

[Shu Chen, 2015-06-27 15:54:44]

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

[Shu Chen, 2015-06-27 15:54:44]

The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1209913002/#ps100001
(title: "rebased & fixed test failures.")

[commit-bot: I haz the power, 2015-06-27 15:54:53]

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

[Shu Chen, 2015-06-27 16:24:16]

shuchen@chromium.org changed reviewers:
+ oshima@chromium.org

[Shu Chen, 2015-06-27 16:24:16]

+oshima@ for new Ash changes to fix test failures.

Note that the return value of InputMethod::DispatchKeyEvent() is useless,
because it will call DispatchKeyEventPostIME() to continue dispatching the key
event.

[commit-bot: I haz the power, 2015-06-27 17:20:49]

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

[commit-bot: I haz the power, 2015-06-27 17:20:50]

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

[Shu Chen, 2015-06-29 04:57:14]

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

[Shu Chen, 2015-06-29 04:57:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
suzhe@chromium.org
Link to the patchset: https://codereview.chromium.org/1209913002/#ps120001
(title: "fixed test failures.")

[commit-bot: I haz the power, 2015-06-29 04:57:33]

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

[commit-bot: I haz the power, 2015-06-29 05:34:20]

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

[commit-bot: I haz the power, 2015-06-29 05:34:21]

Dry run: This issue passed the CQ dry run.

[oshima, 2015-06-30 15:15:50]

https://codereview.chromium.org/1209913002/diff/120001/ash/ime/input_method_e...
File ash/ime/input_method_event_handler.cc (right):

https://codereview.chromium.org/1209913002/diff/120001/ash/ime/input_method_e...
ash/ime/input_method_event_handler.cc:35:
input_method_->DispatchKeyEvent(*event);
Why we're ignoring the result now? Looks like the current impl returns true
only, but the code should be written based on API spec, not
particular impl.

[Shu Chen, 2015-07-01 02:02:42]

https://codereview.chromium.org/1209913002/diff/120001/ash/ime/input_method_e...
File ash/ime/input_method_event_handler.cc (right):

https://codereview.chromium.org/1209913002/diff/120001/ash/ime/input_method_e...
ash/ime/input_method_event_handler.cc:35:
input_method_->DispatchKeyEvent(*event);
On 2015/06/30 15:15:50, oshima (OOO until July 6th) wrote:
> Why we're ignoring the result now? Looks like the current impl returns true
> only, but the code should be written based on API spec, not
> particular impl.

I will change the API spec in a separated CL.
The InputMethod::DispatchKeyEvent() return type should be void.
And its parameter should be KeyEvent* instead of const KeyEvent&.
Always return true in InputMethodChromeOS::DispatchKeyEvent() is actually a
workaround.

DispatchKeyEventPostIME() used to returns event.handled() but now
event.stopped_propagation(), which means sometimes it will return false. As I
noticed, some Views will do event.SetHandled() after handling the events, while
EventHandlers do event.StopPropagation().
If DispatchKeyEvent() uses DispatchKeyEventPostIME()'s return value to return
(not happen on ChromeOS but for other platforms), event->StopPropagation() won't
be called and there would be duplicated handling.

From another perspective, the key event flow should be always using
DispatchKeyEventPostIME() to continue dispatching instead of leveraging event
bits. So that IMEH should always consume the key events.

[oshima, 2015-07-09 19:42:03]

sorry for delay. lgtm

[Shu Chen, 2015-07-14 07:36:18]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-07-14 07:36:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209913002/120001

[commit-bot: I haz the power, 2015-07-14 08:26:12]

Committed patchset #7 (id:120001)

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

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