Don't rewrite events twice.

Don't rewrite events twice.

Rewriters are first tried in AshWindowTreeHostPlatform::DispatchEvent(). We
also rewrite them in WindowTreeHost::DispatchKeyEventPostIME().

This for example causes problems when ARC++ is focused while spoken feedback
is active. So this CL removes the 2nd rewrite.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
Review-Url: https://codereview.chromium.org/2916673002
Cr-Commit-Position: refs/heads/master@{#476848}
Committed: https://chromium.googlesource.com/chromium/src/+/9aa11de928b45b4bf70124d926f9359d8d48586c

[Hadi, 2017-05-31 19:31:57]

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

[commit-bot: I haz the power, 2017-05-31 19:32:49]

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-31 19:38:00]

Description was changed from

==========
Don't rewrite events not handled by Spoken Feedback.

This causes infinite recursion and events are not delivered to the intended
target.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be try infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
==========

to

==========
Don't rewrite events not handled by Spoken Feedback.

This causes infinite recursion and events are not delivered to the intended
target.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
==========

[commit-bot: I haz the power, 2017-05-31 22:13:39]

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

[commit-bot: I haz the power, 2017-05-31 22:13:40]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-06-01 19:04:39]

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

[commit-bot: I haz the power, 2017-06-01 19:05:18]

Dry run: CQ is trying da patch.

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

[Hadi, 2017-06-01 19:07:36]

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

[commit-bot: I haz the power, 2017-06-01 19:07:51]

Dry run: CQ is trying da patch.

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

[Hadi, 2017-06-01 19:08:17]

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

[Hadi, 2017-06-01 19:08:18]

PTAL. Thanks.

[sadrul, 2017-06-01 19:10:48]

Code looks good. Can the test live in ui/aura/?

[Hadi, 2017-06-01 19:17:04]

On 2017/06/01 19:10:48, sadrul wrote:
> Code looks good. Can the test live in ui/aura/?

The correct rewrite happens in AshWindowTreeHostPlatform::DispatchEvent(), so I
put the test here. Alternatively, we could test that
aura::WindowTreeHost::DispatchKeyEventPostIME() does not rewrite in
window_tree_host_unittest, but doing the end to end test seemed more relevant as
it may be able to catch some other similar bugs in future (I don't have strong
opinion here though).

[commit-bot: I haz the power, 2017-06-01 20:16:24]

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

[commit-bot: I haz the power, 2017-06-01 20:16:25]

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

[David Tseng, 2017-06-01 22:11:09]

Sorry, but this doesn't actually fix the issue. Can you please revert on tot and
on m60? This is causing dev channel issues (ChromeVox is entirely unusable).

SpokenFeedbackRewriter captures the keys not just when the Search key is down,
but also when the extension requests it be captured.

But more seriously, if you simply log all events coming through
SpokenFeedbackEventRewriter::RewriteEvent, you will see the you get dup key
presses for each key press e.g. pressing 'a' results in two key press (not
release) events for key code 65.

[David Tseng, 2017-06-01 22:26:09]

On 2017/06/01 22:11:09, David Tseng wrote:
> Sorry, but this doesn't actually fix the issue. Can you please revert on tot
and
> on m60? This is causing dev channel issues (ChromeVox is entirely unusable).
> 
> SpokenFeedbackRewriter captures the keys not just when the Search key is down,
> but also when the extension requests it be captured.
> 
> But more seriously, if you simply log all events coming through
> SpokenFeedbackEventRewriter::RewriteEvent, you will see the you get dup key
> presses for each key press e.g. pressing 'a' results in two key press (not
> release) events for key code 65.

Update: I should have looked at the code first. Please re-title (the root cause
doesn't have to to do with spoken feedback but trying event rewriters twice).

[Hadi, 2017-06-02 14:03:51]

Description was changed from

==========
Don't rewrite events not handled by Spoken Feedback.

This causes infinite recursion and events are not delivered to the intended
target.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
==========

to

==========
Don't rewrite events twice.

Rewriters are first tried in AshWindowTreeHostPlatform::DispatchEvent(). We
also rewrite them in WindowTreeHost::DispatchKeyEventPostIME().

This for example causes problems when ARC++ is focused while spoken feedback
is active. So this CL removes the 2nd rewrite.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
==========

[Hadi, 2017-06-02 14:04:03]

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

[commit-bot: I haz the power, 2017-06-02 14:04:34]

Dry run: CQ is trying da patch.

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

[Hadi, 2017-06-02 14:05:26]

Move the unittest to window_tree_host_unittest, PTAL.

[commit-bot: I haz the power, 2017-06-02 14:47:42]

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

[commit-bot: I haz the power, 2017-06-02 14:47:43]

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

[Hadi, 2017-06-02 17:02:33]

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

[commit-bot: I haz the power, 2017-06-02 17:02:46]

Dry run: CQ is trying da patch.

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

[Hadi, 2017-06-02 17:03:07]

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

[commit-bot: I haz the power, 2017-06-02 17:03:21]

Dry run: CQ is trying da patch.

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

[Hadi, 2017-06-02 17:36:06]

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

[commit-bot: I haz the power, 2017-06-02 17:36:24]

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-06-02 18:59:25]

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

[commit-bot: I haz the power, 2017-06-02 18:59:27]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-06-02 20:04:39]

lgtm

https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_unittest.cc (right):

https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_unittest.cc:82: EXPECT_EQ(0,
event_rewriter.events_seen());
Can you do SendEventToSink() on the WTH, and verify that (1) ime sees the event,
and (2) rewriter sees the event exactly once?

[Hadi, 2017-06-02 20:13:28]

On 2017/06/02 20:04:39, sadrul wrote:
> lgtm
> 
>
https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
> File ui/aura/window_tree_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
> ui/aura/window_tree_host_unittest.cc:82: EXPECT_EQ(0,
> event_rewriter.events_seen());
> Can you do SendEventToSink() on the WTH, and verify that (1) ime sees the
event,
> and (2) rewriter sees the event exactly once?

Done. (I'm not sure what #1 exactly means, we're sending the event directly to
InputMethod here).

[sky, 2017-06-02 21:46:39]

Sadrul is an owner here, so don't wait for me.

[Hadi, 2017-06-03 01:27:24]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-06-03 01:28:00]

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-06-03 01:33:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496453244622070,
"parent_rev": "3d5468a17e28d917b1ec26196e2241a121145f8d", "commit_rev":
"9aa11de928b45b4bf70124d926f9359d8d48586c"}

[commit-bot: I haz the power, 2017-06-03 01:33:41]

Description was changed from

==========
Don't rewrite events twice.

Rewriters are first tried in AshWindowTreeHostPlatform::DispatchEvent(). We
also rewrite them in WindowTreeHost::DispatchKeyEventPostIME().

This for example causes problems when ARC++ is focused while spoken feedback
is active. So this CL removes the 2nd rewrite.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179
==========

to

==========
Don't rewrite events twice.

Rewriters are first tried in AshWindowTreeHostPlatform::DispatchEvent(). We
also rewrite them in WindowTreeHost::DispatchKeyEventPostIME().

This for example causes problems when ARC++ is focused while spoken feedback
is active. So this CL removes the 2nd rewrite.

If Spoken Feedback is active but ARC++ is focused, this is the path for
super+right-arrow:
 1. AshWindowTreeHostPlatform::DispatchEvent() tries rewriters before sending
    event to sink.
 2. SpokenFeedbackEventRewriter sees super key is down, so sends the event to
    the Spoken Feedback event extension and tells ash to discard the event.
 3. The Spoken Feedback extension doesn't handle the event, and the event is
    resent to ash from WebContentsImpl::HandleKeyboardEvent(), which sends it
    to SpokenFeedbackEventRewriterDelegate::HandleKeyboardEvent(), which sends
    it to event sink (skipping rewriters).
 4. Event is sent to InputMethod, it is not handled by InputMethod, so it is
    sent to WindowTreeHost::DispatchKeyEventPostIME().

Now, if at WTH::DispatchKeyEventPostIME() we try to rewrite the event again,
steps 2 to 4 will be tried infinitely and event won't be dispatched to ARC++.

This is what was happening before this CL. This CL skips rewriters at
WTH::DispatchKeyEventPostIME().

BUG=727179

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

[commit-bot: I haz the power, 2017-06-03 01:33:42]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9aa11de928b45b4bf70124d926f9...

[sadrul, 2017-06-03 03:12:49]

On 2017/06/02 20:13:28, Hadi wrote:
> On 2017/06/02 20:04:39, sadrul wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
> > File ui/aura/window_tree_host_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2916673002/diff/100001/ui/aura/window_tree_ho...
> > ui/aura/window_tree_host_unittest.cc:82: EXPECT_EQ(0,
> > event_rewriter.events_seen());
> > Can you do SendEventToSink() on the WTH, and verify that (1) ime sees the
> event,
> > and (2) rewriter sees the event exactly once?
> 
> Done. (I'm not sure what #1 exactly means, we're sending the event directly to
> InputMethod here).

I meant you should send the event to WindowTreeHost, not directly to IME, in the
test, and verify that IME sees the event, and the rewriter sees the event
exactly once.