Convert chromeos::EventRewriter to a ui::EventRewriter

Convert chromeos::EventRewriter to a ui::EventRewriter

This does not yet replace chromeos::EventRewriter, since StickyKeys,
KeyboardDriverEventRewriter, and device IDs remain to be converted
(crbug.com/354035, crbug.com/360377).
At that point the new_* files can be folded back into the originals
to maintain history.

BUG=354034
TEST=unit_test

[kpschoedel, 2014-04-14 20:02:59]

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/new_event_rewriter.cc (right):

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:176: if (xev &&
xev->xkey.send_event)
I don't know whether there's a plan for replacing ui_controls::SendKeyPress()
that affects how to test for this, or whether we should add an EF_ flag, or use
EF_SYNTHESIZED.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:183: changed |=
RewriteModifierKeys(event);
KeyboardDrivenEventRewriter() used to be called here, but since that prevents
further rewriting, it makes sense to uncouple it into its own ui::EventRewriter.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:185: changed |=
RewriteExtendedKeys(event);
RewriteStickyKeys() will be called at this point, and the sequencing will then
be a little more complicated, since RewriteStickyKeys() can insert or discard
events.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:210: bool
KeyboardEventRewriter::IsAppleKeyboard(const ui::Event& event) const {
Passing the Event to IsAppleKeyboard() and HasDiamondKey() on the assumption
that 360377 will have the device in (or obtainable) from it, and for consistency
with TopRowKeysAreFunctionKeys().

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:232: bool
KeyboardEventRewriter::TopRowKeysAreFunctionKeys(
Should this become device-dependent later (post 360377)? External keyboards
might have function key labels that contradict Chrome's.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:287: // Do nothing if we
have just logged in as guest but have not restarted chrome
This may be obsolete; I'll check this shortly.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:388: #if USE_X11
Forgot to remove #if when XKeyboard -> ImeKeyboard; will do.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:667: static const
KeyboardRemapping kNumberKeysToFkeys[] = {
I notice this (in the original) does not handle numpad digits; I can't tell
whether that's intentional.

https://codereview.chromium.org/237363005/diff/1/chrome/chrome_browser_chrome...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/237363005/diff/1/chrome/chrome_browser_chrome...
chrome/chrome_browser_chromeos.gypi:310:
'browser/chromeos/events/new_event_rewriter.cc',
These can be folded back to event_rewriter_* once the other subissues of
crbug.com/354036 are complete.

[Daniel Erat, 2014-04-14 20:05:11]

Sadrul has more state about this than me, so I will generously let him take the
first look unless there's something specific that you want me to comment on...
:-)

[sadrul, 2014-04-14 21:51:32]

The overall approach looks reasonable. I would split up this CL into smaller
CLs:

 * The change in EventRewriter interface.
 * The change to add the EF_NUMPAD flag.
 * The change to make chromeos::EventRewriter a ui::EventRewriter.

The last change would need a lot of proof-reading to make sure there aren't
typos etc. If there's a way to put up a patch that minimizes the diff, and shows
up +/- from the current files, instead of new files, that would be wonderful.

Also, we shouldn't have both chromeos::EventRewriter and
chromeos::KeyboardEventRewriter in the tree at the same time, because there is a
chance one of them will get out of sync over time. I think it should be possible
to just turn the chromeos::EventRewriter into a ui::EventRewriter (and call it
KeyboardEventRewriter as you have done in this CL) in the same CL?

The sequence of event-dispatch with the current rewriters as I understand it is:

  1 The chromeos::KeyboardDrivenEventRewriter gets the first chance to rewrite
the X event.
  2 The chromeos::EventRewiter gets a chance to rewrite the X event.
  3 ui::Event is created.
  4 ui::EventRewriters get a chance to rewrite the event (there currently isn't
any).
  5 The sticky-key thingy gets a chance to rewrite the ui::Event.

It should be possible to remove the chromeos::EventRewriter in step 2, and
convert it into a ui::EventRewriter (so that it gets to rewrite the event in
step 4), and maintain the correct ordering of rewriters, right? Can we just do
that? If it depends on other issues to be resolved first, then let's try to
resolve those first.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/new_event_rewriter.cc (right):

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:172: #if USE_X11
chromium style is to use #if defined(USE_X11) instead of '#if USE_X11'

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:176: if (xev &&
xev->xkey.send_event)
On 2014/04/14 20:02:59, kpschoedel wrote:
> I don't know whether there's a plan for replacing ui_controls::SendKeyPress()
> that affects how to test for this, or whether we should add an EF_ flag, or
use
> EF_SYNTHESIZED.

This doesn't really look right. The referenced by doesn't make a lot of sense,
because cut/copy/paste etc. from the wrench menu shouldn't be sending key-events
using SendKeyPress(). My suggestion would be to remove this piece of code.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:183: changed |=
RewriteModifierKeys(event);
On 2014/04/14 20:02:59, kpschoedel wrote:
> KeyboardDrivenEventRewriter() used to be called here, but since that prevents
> further rewriting, it makes sense to uncouple it into its own
ui::EventRewriter.

Yep, that sounds like a good plan, as long as the corresponding rewriter gets
the first chance to rewrite the event.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:232: bool
KeyboardEventRewriter::TopRowKeysAreFunctionKeys(
On 2014/04/14 20:02:59, kpschoedel wrote:
> Should this become device-dependent later (post 360377)? External keyboards
> might have function key labels that contradict Chrome's.

Perhaps. You can leave a note here accordingly.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/new_event_rewriter_unittest.cc (right):

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter_unittest.cc:139:
EXPECT_EQ(GetExpectedResultAsString(ui::VKEY_A,
How are you creating these tests? It would be great if this shows up as a change
to the existing tests, so that it is easier to proof-read the expectations
(ideally, the expectations wouldn't change at all).

https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h#...
ui/events/event_constants.h:93: EF_NUMPAD              = 1 << 12, // Key
originates from number pad (Xkb only)
Maybe this should be separate KeyEventFlags (like MouseEventFlags below)

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:431: static const unsigned int
numpad_keysyms[] = {
Does this need to be static?

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:470: return EF_NUMPAD;
There's an IsKeypadKey macro in X11/Xutil.h. Can we use that here?

https://codereview.chromium.org/237363005/diff/1/ui/events/x/events_x.cc
File ui/events/x/events_x.cc (right):

https://codereview.chromium.org/237363005/diff/1/ui/events/x/events_x.cc#newc...
ui/events/x/events_x.cc:141: flags |= ui::EF_COMMAND_DOWN;
If we are going to use it here, we should update the doc for EF_COMMAND_DOWN to
explain what it means in X11 (the doc currently says only useful on OSX)

https://codereview.chromium.org/237363005/diff/1/ui/events/x/events_x.cc#newc...
ui/events/x/events_x.cc:153: int GetEventFlagsFromXKeyEvent(XEvent* xevent)
{ in this line

https://codereview.chromium.org/237363005/diff/1/ui/events/x/events_x.cc#newc...
ui/events/x/events_x.cc:156:
ui::EventFlagsFromXKeysym(XLookupKeysym(&xevent->xkey, 0));
If we use the macro from Xutil.h, then perhaps we don't need to have
EventFlagsFromXKeysym at all, and just add the flag from here?

[kpschoedel, 2014-04-14 22:25:45]

OK, I'll abandon this CL and start uploading smaller pieces with the suggested
changes.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/new_event_rewriter.cc (right):

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter.cc:172: #if USE_X11
On 2014/04/14 21:51:32, sadrul wrote:
> chromium style is to use #if defined(USE_X11) instead of '#if USE_X11'

OK, I'll use that in the replacement CL. The original file had '#if USE_X11'.

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/new_event_rewriter_unittest.cc (right):

https://codereview.chromium.org/237363005/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/new_event_rewriter_unittest.cc:139:
EXPECT_EQ(GetExpectedResultAsString(ui::VKEY_A,
On 2014/04/14 21:51:32, sadrul wrote:
> How are you creating these tests? It would be great if this shows up as a
change
> to the existing tests, so that it is easier to proof-read the expectations
> (ideally, the expectations wouldn't change at all).

These are copies from the old unit tests, with the XK_ codes removed. This
should be clearer in the replacement CL.

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:431: static const unsigned int
numpad_keysyms[] = {
On 2014/04/14 21:51:32, sadrul wrote:
> Does this need to be static?
If it's not static, the compiler is expected to treat it as dynamically
initializing the array on each call. On a quick test, gcc is not able or willing
to optimize this away (though clang is).

https://codereview.chromium.org/237363005/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:470: return EF_NUMPAD;
On 2014/04/14 21:51:32, sadrul wrote:
> There's an IsKeypadKey macro in X11/Xutil.h. Can we use that here?

Yes, thanks for pointing that out.

[kpschoedel, 2014-04-15 14:08:30]

> Maybe this should be separate KeyEventFlags (like MouseEventFlags below)

OK. Is the intent that KeyEventFlags overlap MouseEventFlags (i.e. also start
from 1<<16)?

https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h#...
ui/events/event_constants.h:93: EF_NUMPAD              = 1 << 12, // Key
originates from number pad (Xkb only)
On 2014/04/14 21:51:32, sadrul wrote:
> Maybe this should be separate KeyEventFlags (like MouseEventFlags below)

OK. Is the intent that KeyEventFlags overlap MouseEventFlags (i.e. also start
from 1<<16)?

[sadrul, 2014-04-15 16:32:00]

On 2014/04/15 14:08:30, kpschoedel wrote:
> > Maybe this should be separate KeyEventFlags (like MouseEventFlags below)
> 
> OK. Is the intent that KeyEventFlags overlap MouseEventFlags (i.e. also start
> from 1<<16)?
> 
> https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h
> File ui/events/event_constants.h (right):
> 
>
https://codereview.chromium.org/237363005/diff/1/ui/events/event_constants.h#...
> ui/events/event_constants.h:93: EF_NUMPAD              = 1 << 12, // Key
> originates from number pad (Xkb only)
> On 2014/04/14 21:51:32, sadrul wrote:
> > Maybe this should be separate KeyEventFlags (like MouseEventFlags below)
> 
> OK. Is the intent that KeyEventFlags overlap MouseEventFlags (i.e. also start
> from 1<<16)?

Good question. I think that makes sense, yeah.