Add native X11 testing to chromeos::EventRewriter unit tests

Add native event testing to chromeos::EventRewriter unit tests.

Most tests (not having state) are converted to a table-driven form,
so that multiple test operations can be done on the same test data.
Additional tests based on X11 native events are done on these cases.

The fallback X11 keycode to keysym mapping is expanded, because the
X server used by some automated tests does not support some of the
keycodes used by ChromeOS top-row special keys.


R=sadrul@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272428

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273280

[kpschoedel, 2014-05-08 18:59:24]

I've uploaded this as two patch sets.

The first patch set converts most of the test cases (excepting some stateful
ones) into a uniform table form, so that more testing can be done on them. There
is no change to functionality.

The second patch set adds some native X11 event testing to these cases.

[sadrul, 2014-05-14 01:23:55]

https://codereview.chromium.org/270633012/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/270633012/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:192:
{KeyTestCase::TEST_ALL, ui::ET_KEY_PRESSED,
I think we want a blank-space after {?

https://codereview.chromium.org/270633012/diff/20001/ui/events/test/events_te...
File ui/events/test/events_test_utils_x11.cc (right):

https://codereview.chromium.org/270633012/diff/20001/ui/events/test/events_te...
ui/events/test/events_test_utils_x11.cc:29: ((flags & ui::EF_NUMPAD_KEY) ?
Mod2Mask : 0) |
We don't map Mod3 to MOD3 and Mod2 to NUMPAD in GetEventFlagsFromXState() (in
ui/events/x/events_x.cc). Should we?

[kpschoedel, 2014-05-14 02:23:56]

https://codereview.chromium.org/270633012/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/270633012/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:192:
{KeyTestCase::TEST_ALL, ui::ET_KEY_PRESSED,
On 2014/05/14 01:23:55, sadrul wrote:
> I think we want a blank-space after {?

This is from clang-format.

https://codereview.chromium.org/270633012/diff/20001/ui/events/test/events_te...
File ui/events/test/events_test_utils_x11.cc (right):

https://codereview.chromium.org/270633012/diff/20001/ui/events/test/events_te...
ui/events/test/events_test_utils_x11.cc:29: ((flags & ui::EF_NUMPAD_KEY) ?
Mod2Mask : 0) |
On 2014/05/14 01:23:55, sadrul wrote:
> We don't map Mod3 to MOD3 and Mod2 to NUMPAD in GetEventFlagsFromXState() (in
> ui/events/x/events_x.cc). Should we?

Mod3Mask, yes; it's set when the caps lock key is down, as distinct from
CapsLock, which is set when caps locking is active, which I didn't realize until
it caused a regression, so that is already in another CL:
https://codereview.chromium.org/277443004/

Mod2Mask is not present on input because Chrome doesn't manage NumLock state. It
does get set in the outgoing X11 events.

[kpschoedel, 2014-05-20 15:31:04]

Sync with master.

[Daniel Erat, 2014-05-20 17:02:45]

i'll defer to sadrul on this unless there's something in particular that you
need me to review or approve

[sadrul, 2014-05-20 17:13:57]

lgtm

[kpschoedel, 2014-05-20 17:16:48]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:17:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/40001

[commit-bot: I haz the power, 2014-05-20 20:51:07]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-20 20:57:19]

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

[commit-bot: I haz the power, 2014-05-20 20:57:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[kpschoedel, 2014-05-20 21:08:47]

Daniel, could you rubber-stamp Sadrul's review?

[Daniel Erat, 2014-05-20 21:09:44]

lgtm

[Daniel Erat, 2014-05-20 21:09:50]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:10:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/40001

[commit-bot: I haz the power, 2014-05-20 21:42:17]

Change committed as 271762

[Nicolas Zea, 2014-05-21 01:32:24]

On 2014/05/20 21:42:17, I haz the power (commit-bot) wrote:
> Change committed as 271762

FYI this appears to be breaking linux chromiumos asan.

http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

[kpschoedel, 2014-05-21 15:20:39]

Patch set 4 only touches ui/events/keycodes/keyboard_code_conversion_x.cc

[kpschoedel, 2014-05-21 15:32:16]

Sorry, forgot the review doesn't automatically copy my local commit message to
explain the change:


Expand the fallback keycode to keysym mapping.

Some automated test runs were failing, apparently because the X server
used does not support some of the keycodes used by ChromeOS top-row
special keys. Expanded |DefaultXKeysymFromHardwareKeycode()| to cover
these along with all the common 105-key keyboard codes.

[sadrul, 2014-05-21 15:55:51]

On 2014/05/21 15:32:16, kpschoedel wrote:
> Sorry, forgot the review doesn't automatically copy my local commit message to
> explain the change:
> 
> 
> Expand the fallback keycode to keysym mapping.
> 
> Some automated test runs were failing, apparently because the X server
> used does not support some of the keycodes used by ChromeOS top-row
> special keys. Expanded |DefaultXKeysymFromHardwareKeycode()| to cover
> these along with all the common 105-key keyboard codes.

Please include a description of this change in the CL description too.

lgtm

[kpschoedel, 2014-05-21 16:04:49]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:57:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/60001

[commit-bot: I haz the power, 2014-05-22 13:32:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 13:34:59]

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

[commit-bot: I haz the power, 2014-05-22 13:35:00]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[kpschoedel, 2014-05-22 14:22:34]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:23:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/60001

[commit-bot: I haz the power, 2014-05-22 14:27:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 14:29:22]

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

[commit-bot: I haz the power, 2014-05-22 14:29:22]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[kpschoedel, 2014-05-22 16:00:11]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-22 16:02:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/60001

[commit-bot: I haz the power, 2014-05-22 16:06:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 16:09:10]

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

[commit-bot: I haz the power, 2014-05-22 16:09:11]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[kpschoedel, 2014-05-23 03:56:19]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-23 03:57:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/60001

[commit-bot: I haz the power, 2014-05-23 07:17:59]

Change committed as 272428

[kpschoedel, 2014-05-23 14:02:39]

A revert of this CL has been created in
https://codereview.chromium.org/294053014/ by kpschoedel@chromium.org.

The reason for reverting is: Some key codes are not recognized by the X server
used by valgrind tests, leading to test failures.
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v....

[kpschoedel, 2014-05-28 13:58:31]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-05-28 13:59:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/270633012/110001

[commit-bot: I haz the power, 2014-05-28 17:28:19]

Change committed as 273280