mash: Don't synthesize extra key press events in the window server

mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

Revert a workaround in InputMethodBridge for these synthesized events,
see https://codereview.chromium.org/2576973003/

BUG=706574, 707055
TEST=added to mus_ws_unittests

Review-Url: https://codereview.chromium.org/2795503002
Cr-Commit-Position: refs/heads/master@{#463036}
Committed: https://chromium.googlesource.com/chromium/src/+/60a7c584a9bef913195fc68ebfae2968f7be9b88

[James Cook, 2017-03-31 23:23:57]

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

[commit-bot: I haz the power, 2017-03-31 23:24:43]

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

[James Cook, 2017-03-31 23:25:57]

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

[commit-bot: I haz the power, 2017-03-31 23:26:05]

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

[James Cook, 2017-03-31 23:28:18]

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

[James Cook, 2017-03-31 23:28:31]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-31 23:29:05]

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

[James Cook, 2017-03-31 23:31:19]

jamescook@chromium.org changed reviewers:
+ erg@chromium.org

[James Cook, 2017-03-31 23:31:20]

erg, please take a look.

I tried to test this manually on Linux Ozone, with some help from
thomasanderson@, but we can't get chrome to start up. Thomas says
tonikitoo@igalia.com is the only person running that config right now, so I
pinged tonikitoo on the bug.

https://codereview.chromium.org/2795503002/diff/40001/services/ui/ws/platform...
File services/ui/ws/platform_display_default.h (left):

https://codereview.chromium.org/2795503002/diff/40001/services/ui/ws/platform...
services/ui/ws/platform_display_default.h:75: #if !defined(OS_ANDROID)
Lemme know if you want me to keep these ifdefs. I think ImageCursors is compiled
on Android, so leaving it null seemed a little better than keeping it ifdef-y.

[commit-bot: I haz the power, 2017-03-31 23:39:49]

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

[commit-bot: I haz the power, 2017-03-31 23:39:50]

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

[James Cook, 2017-03-31 23:47:36]

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

[commit-bot: I haz the power, 2017-03-31 23:48: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-04-01 01:33:12]

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

[commit-bot: I haz the power, 2017-04-01 01:33:13]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-04-01 15:49:52]

tonikitoo@igalia.com changed reviewers:
+ tonikitoo@igalia.com

[tonikitoo, 2017-04-01 15:49:53]

In the commit message: PlatformWindowDefault -> PlatformDisplayDefault

[James Cook, 2017-04-01 16:51:01]

Description was changed from

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformWindowDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformWindowDefault and OzonePlatform.

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

to

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

[James Cook, 2017-04-01 16:51:25]

On 2017/04/01 15:49:53, tonikitoo wrote:
> In the commit message: PlatformWindowDefault -> PlatformDisplayDefault

Fixed commit message, thanks!

[Elliot Glaysher, 2017-04-04 17:11:35]

lgtm

[James Cook, 2017-04-04 23:46:02]

jamescook@chromium.org changed reviewers:
+ rjkroege@chromium.org

[James Cook, 2017-04-04 23:46:03]

rjkroege, can I get OWNERS for ui/ozone?

[James Cook, 2017-04-05 15:19:39]

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

[commit-bot: I haz the power, 2017-04-05 15:20: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-04-05 17:28:42]

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

[commit-bot: I haz the power, 2017-04-05 17:28:43]

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

[James Cook, 2017-04-06 17:54:57]

On 2017/04/04 23:46:03, James Cook wrote:
> rjkroege, can I get OWNERS for ui/ozone?

rjkroege, friendly ping? You mentioned you had a suggestion?

[rjkroege, 2017-04-06 20:01:55]

On 2017/04/06 17:54:57, James Cook wrote:
> On 2017/04/04 23:46:03, James Cook wrote:
> > rjkroege, can I get OWNERS for ui/ozone?
> 
> rjkroege, friendly ping? You mentioned you had a suggestion?

Thanks for the ping. I forgot to get back to this. My apologies. Here's what I
was eventually thinking:

Add *GetOzonePlatform() to PlatformDisplayDelegate and use that in
PlatformDisplay. And then you could easily adjust your test to use the
TestOzonePlatform instance?

And then follow up CLs (which you could probably get me to do by filing bugs)
because I think we will want many more tests that look like an extension of
this:

* a new Ozone entry OzonePlatform* EnsureInstance(const *char name) that makes a
specific platform pointer and returns it with the promise (noted in the
comments) that it does not alter the default instance or do anything that would
interfere with the existing platform where the string specifies the desired
platform by name.

* a new test platform (or set of options for configuring headless because that
was what it was for) that is appropriate for unit tests like this

wdyt?

[James Cook, 2017-04-06 22:57:51]

Rob, I implemented your suggestion. See latest patchset. However, there's
another problem. OzonePlatform is a singleton, and it's leaked after
initialization. If my test runs in a batch with other tests, and any of those
other tests init OzonePlatform, then creating my TestOzonePlatform DCHECKs out
in the OzonePlatform() constructor.

Do you have a suggestion about how to resolve this? I'm not too familiar with
the test suite runners for mus_ws_unittests.

[James Cook, 2017-04-06 23:21:25]

On 2017/04/06 22:57:51, James Cook wrote:
> Rob, I implemented your suggestion. See latest patchset. However, there's
> another problem. OzonePlatform is a singleton, and it's leaked after
> initialization. If my test runs in a batch with other tests, and any of those
> other tests init OzonePlatform, then creating my TestOzonePlatform DCHECKs out
> in the OzonePlatform() constructor.
> 
> Do you have a suggestion about how to resolve this? I'm not too familiar with
> the test suite runners for mus_ws_unittests.

It looks like some tests that use WindowServerTestHelper call into GpuMain which
initializes OzonePlatform. Maybe I should add OzonePlatform::Shutdown() and add
calls to that in various places?

[James Cook, 2017-04-06 23:21:33]

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

[commit-bot: I haz the power, 2017-04-06 23:22:30]

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

[James Cook, 2017-04-07 00:33:54]

rjkroege, please take another look. This works and doesn't seem too ugly to me.

[James Cook, 2017-04-07 00:34:23]

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

[commit-bot: I haz the power, 2017-04-07 00:34:51]

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-04-07 01:36:38]

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

[commit-bot: I haz the power, 2017-04-07 01:36:39]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-04-07 14:12:58]

On 2017/04/06 23:21:25, James Cook wrote:
 > On 2017/04/06 22:57:51, James Cook wrote:
 > > Rob, I implemented your suggestion. See latest patchset. However, 
there's
 > > another problem. OzonePlatform is a singleton, and it's leaked after
 > > initialization. If my test runs in a batch with other tests, and 
any of those
 > > other tests init OzonePlatform, then creating my TestOzonePlatform 
DCHECKs out
 > > in the OzonePlatform() constructor.
 > >
 > > Do you have a suggestion about how to resolve this? I'm not too 
familiar with
 > > the test suite runners for mus_ws_unittests.
 >
 > It looks like some tests that use WindowServerTestHelper call into 
GpuMain which
 > initializes OzonePlatform. Maybe I should add 
OzonePlatform::Shutdown() and add
 > calls to that in various places?

Hi James.

One orthogonal question about the CL: is the cursors+android changes 
related to the double key press issue you are fixing?
In case no, they seem to add a bit of noisy to the fix itself (described 
on the commit message).

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2017-04-07 14:18:21]

Description was changed from

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

to

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

[James Cook, 2017-04-07 14:19:31]

On 2017/04/07 14:12:58, tonikitoo wrote:
> On 2017/04/06 23:21:25, James Cook wrote:
>  > On 2017/04/06 22:57:51, James Cook wrote:
>  > > Rob, I implemented your suggestion. See latest patchset. However, 
> there's
>  > > another problem. OzonePlatform is a singleton, and it's leaked after
>  > > initialization. If my test runs in a batch with other tests, and 
> any of those
>  > > other tests init OzonePlatform, then creating my TestOzonePlatform 
> DCHECKs out
>  > > in the OzonePlatform() constructor.
>  > >
>  > > Do you have a suggestion about how to resolve this? I'm not too 
> familiar with
>  > > the test suite runners for mus_ws_unittests.
>  >
>  > It looks like some tests that use WindowServerTestHelper call into 
> GpuMain which
>  > initializes OzonePlatform. Maybe I should add 
> OzonePlatform::Shutdown() and add
>  > calls to that in various places?
> 
> Hi James.
> 
> One orthogonal question about the CL: is the cursors+android changes 
> related to the double key press issue you are fixing?
> In case no, they seem to add a bit of noisy to the fix itself (described 
> on the commit message).
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Yes, it's related. ImageCursors requires a UI resource bundle that isn't
provided by the test harness. Allowing it to be null seemed simpler than
wiring it up and avoids disk IO in the test. I updated the CL description.

Thanks for noticing! It's nice to have extra eyes on this CL.

[rjkroege, 2017-04-07 15:12:37]

thanks very much. lgtm

[James Cook, 2017-04-07 15:32:02]

jamescook@chromium.org changed reviewers:
+ sky@chromium.org

[James Cook, 2017-04-07 15:32:02]

sky, can I get OWNERS for ui/aura/env.cc?

[tonikitoo, 2017-04-07 15:48:40]

https://codereview.chromium.org/2795503002/diff/120001/ui/ozone/public/ozone_...
File ui/ozone/public/ozone_platform.cc (right):

https://codereview.chromium.org/2795503002/diff/120001/ui/ozone/public/ozone_...
ui/ozone/public/ozone_platform.cc:83: // TODO(spang): Currently need to leak
this object.
delete this line now?

[rjkroege, 2017-04-07 15:51:06]

On 2017/04/07 15:48:40, tonikitoo wrote:
>
https://codereview.chromium.org/2795503002/diff/120001/ui/ozone/public/ozone_...
> File ui/ozone/public/ozone_platform.cc (right):
> 
>
https://codereview.chromium.org/2795503002/diff/120001/ui/ozone/public/ozone_...
> ui/ozone/public/ozone_platform.cc:83: // TODO(spang): Currently need to leak
> this object.
> delete this line now?

It still leaks if aura is not used (as it would in the mus gpu/ws processes)

[tonikitoo, 2017-04-07 17:48:54]

https://codereview.chromium.org/2795503002/diff/120001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (left):

https://codereview.chromium.org/2795503002/diff/120001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:197: #if defined(USE_X11) ||
defined(USE_OZONE)
Sorry for being picky here, james. You have got the lgtm's needed, so feel free
to ignore the comment, as owners have precedence here. 
But...

While working on something else, I came across this piece of code:

31 void InputMethodBridge::ProcessKeyEvent(
32     std::unique_ptr<ui::Event> event,
33     const ProcessKeyEventCallback& callback) {
34   DCHECK(event->IsKeyEvent());
35   ui::KeyEvent* key_event = event->AsKeyEvent();
36   if (!key_event->is_char()) {
37     input_method_chromeos_->DispatchKeyEvent(
38         key_event, base::MakeUnique<base::Callback<void(bool)>>(callback));
39   } else {
40     // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation
41     // behaviour of Windows. But for ChromeOS, we don't expect those char
42     // events, so we filter them out.
43     const bool handled = true;
44     callback.Run(handled);
45   }
46 }

.. in
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ime_driver/input...

Is the comment in line 40 still accurate?

Maybe a more conservative approach would be keep the linux behavior even on
chromeos off-device/linux builds(if it is really needed as the comment says),
and add a IsRunningAsSystemCompositor() check to not run this code if
OS_CHROMEOS and is running on real ChromeOS device?

[sky, 2017-04-07 19:18:31]

env LGTM

[James Cook, 2017-04-07 21:16:05]

Description was changed from

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

to

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

Revert a workaround in InputMethodBridge for these synthesized events,
see https://codereview.chromium.org/2576973003/

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

[James Cook, 2017-04-07 21:17:51]

+penghuang just FYI

https://codereview.chromium.org/2795503002/diff/120001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (left):

https://codereview.chromium.org/2795503002/diff/120001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:197: #if defined(USE_X11) ||
defined(USE_OZONE)
On 2017/04/07 17:48:54, tonikitoo wrote:
> Sorry for being picky here, james. You have got the lgtm's needed, so feel
free
> to ignore the comment, as owners have precedence here. 
> But...
> 
> While working on something else, I came across this piece of code:
> 
> 31 void InputMethodBridge::ProcessKeyEvent(
> 32     std::unique_ptr<ui::Event> event,
> 33     const ProcessKeyEventCallback& callback) {
> 34   DCHECK(event->IsKeyEvent());
> 35   ui::KeyEvent* key_event = event->AsKeyEvent();
> 36   if (!key_event->is_char()) {
> 37     input_method_chromeos_->DispatchKeyEvent(
> 38         key_event, base::MakeUnique<base::Callback<void(bool)>>(callback));
> 39   } else {
> 40     // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation
> 41     // behaviour of Windows. But for ChromeOS, we don't expect those char
> 42     // events, so we filter them out.
> 43     const bool handled = true;
> 44     callback.Run(handled);
> 45   }
> 46 }
> 
> .. in
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ime_driver/input...
> 
> Is the comment in line 40 still accurate?
> 
> Maybe a more conservative approach would be keep the linux behavior even on
> chromeos off-device/linux builds(if it is really needed as the comment says),
> and add a IsRunningAsSystemCompositor() check to not run this code if
> OS_CHROMEOS and is running on real ChromeOS device?
> 
> 

tonikitoo, good catch! I talked to penghuang@ and the workaround in
InputMethodBridge isn't necessary if I remove this code. So I removed that
workaround as well. Updated CL description.

Thanks for catching this.

[James Cook, 2017-04-07 21:18:07]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-04-07 21:18:07]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
rjkroege@chromium.org, sky@chromium.org, tonikitoo@igalia.com
Link to the patchset: https://codereview.chromium.org/2795503002/#ps160001
(title: "remove InputMethodBridge workaround")

[commit-bot: I haz the power, 2017-04-07 21:18: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, 2017-04-07 23:08:40]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491599887041070,
"parent_rev": "ffcdf35afebd887f573eb77d5ca5358bea5243f0", "commit_rev":
"60a7c584a9bef913195fc68ebfae2968f7be9b88"}

[commit-bot: I haz the power, 2017-04-07 23:09:28]

Description was changed from

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

Revert a workaround in InputMethodBridge for these synthesized events,
see https://codereview.chromium.org/2576973003/

BUG=706574,707055
TEST=added to mus_ws_unittests
==========

to

==========
mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

Revert a workaround in InputMethodBridge for these synthesized events,
see https://codereview.chromium.org/2576973003/

BUG=706574,707055
TEST=added to mus_ws_unittests

Review-Url: https://codereview.chromium.org/2795503002
Cr-Commit-Position: refs/heads/master@{#463036}
Committed:
https://chromium.googlesource.com/chromium/src/+/60a7c584a9bef913195fc68ebfae...
==========

[commit-bot: I haz the power, 2017-04-07 23:09:31]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/60a7c584a9bef913195fc68ebfae...