event-rewrite: Clean up how events are rewritten.

event-rewrite: Clean up how events are rewritten.

Notable changes:
 * Make the EventRewriter a MessagePumpObserver. This is necessary to make sure
   that the event-rewrite happens before the event is dispatched.
 * Move the EventRewriter in chrome/browser/chromeos (and in chromeos namespace),
   since it does nothing on other platforms.
 * Get rid of the event-rewrite code from ash, since it just adds an extra layer
   of abstraction for no added value.

BUG=343639
R=derat@chromium.org, sky@chromium.org, stevenjb@chromium.org

Previously landed in r251382, but reverted in r251433 because it broke debug builds.

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

[sadrul, 2014-02-14 02:11:22]

I haven't had a chance to test it on a device yet, but the related unit-tests
pass locally.

[Daniel Erat, 2014-02-14 15:56:29]

nice cleanup! adding steven in case he has opinions about where this code should
be.

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:706:
event_rewriter_.reset(new EventRewriter());
nit: blank line after this to set it off from the comment underneath it

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/event_rewriter.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter.cc:204: if (xevent->type == KeyPress ||
xevent->type == KeyRelease) {
nit: don't need curly brackets here

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter.cc:282: ash::wm::WindowState* state =
ash::wm::GetActiveWindowState();
just to double-check (since i honestly don't know): is a given event's target
always the active window?

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter_unittest.cc:192: void
RewriteLocatedEvent(EventRewriter* rewriter,
rename this to RewriteMouseEvent() and make it take a MouseEvent instead?

[sky, 2014-02-14 16:51:03]

LGTM

[sadrul, 2014-02-14 17:23:14]

Addressed the comments, and tested on device. Seems to be working correctly.

I moved the rewriter in chrome/browser/chroemos/ since the
keyboard_driven_event_rewriter also lives in there. Maybe all the event related
code in c/b/chromeos/ could move into c/b/chromeos/events/ (including
system_key_event_listener, xinput_hierarchy_changed_event_listener).

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:706:
event_rewriter_.reset(new EventRewriter());
On 2014/02/14 15:56:29, Daniel Erat wrote:
> nit: blank line after this to set it off from the comment underneath it

Done.

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/event_rewriter.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter.cc:204: if (xevent->type == KeyPress ||
xevent->type == KeyRelease) {
On 2014/02/14 15:56:29, Daniel Erat wrote:
> nit: don't need curly brackets here

Done.

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter.cc:282: ash::wm::WindowState* state =
ash::wm::GetActiveWindowState();
On 2014/02/14 15:56:29, Daniel Erat wrote:
> just to double-check (since i honestly don't know): is a given event's target
> always the active window?

For key-events, the target is either the active window, or is a descendant of
the active window. In either case, the the toplevel window of the target will be
the active window.

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/event_rewriter_unittest.cc:192: void
RewriteLocatedEvent(EventRewriter* rewriter,
On 2014/02/14 15:56:29, Daniel Erat wrote:
> rename this to RewriteMouseEvent() and make it take a MouseEvent instead?

Done.

[Daniel Erat, 2014-02-14 17:33:23]

lgtm

[oshima, 2014-02-14 17:36:07]

On 2014/02/14 17:23:14, sadrul wrote:
> Addressed the comments, and tested on device. Seems to be working correctly.
> 
> I moved the rewriter in chrome/browser/chroemos/ since the
> keyboard_driven_event_rewriter also lives in there. Maybe all the event
related
> code in c/b/chromeos/ could move into c/b/chromeos/events/ (including
> system_key_event_listener, xinput_hierarchy_changed_event_listener).

Being under c/b/ make it impossible to test rewriter using ash_shell.
Most of them should live in ash/events with a delegate interface.
It's probably not the scope of this CL though.

> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:706:
> event_rewriter_.reset(new EventRewriter());
> On 2014/02/14 15:56:29, Daniel Erat wrote:
> > nit: blank line after this to set it off from the comment underneath it
> 
> Done.
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> File chrome/browser/chromeos/event_rewriter.cc (right):
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> chrome/browser/chromeos/event_rewriter.cc:204: if (xevent->type == KeyPress ||
> xevent->type == KeyRelease) {
> On 2014/02/14 15:56:29, Daniel Erat wrote:
> > nit: don't need curly brackets here
> 
> Done.
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> chrome/browser/chromeos/event_rewriter.cc:282: ash::wm::WindowState* state =
> ash::wm::GetActiveWindowState();
> On 2014/02/14 15:56:29, Daniel Erat wrote:
> > just to double-check (since i honestly don't know): is a given event's
target
> > always the active window?
> 
> For key-events, the target is either the active window, or is a descendant of
> the active window. In either case, the the toplevel window of the target will
be
> the active window.
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> File chrome/browser/chromeos/event_rewriter_unittest.cc (right):
> 
>
https://codereview.chromium.org/165463002/diff/380001/chrome/browser/chromeos...
> chrome/browser/chromeos/event_rewriter_unittest.cc:192: void
> RewriteLocatedEvent(EventRewriter* rewriter,
> On 2014/02/14 15:56:29, Daniel Erat wrote:
> > rename this to RewriteMouseEvent() and make it take a MouseEvent instead?
> 
> Done.

[sadrul, 2014-02-14 17:44:41]

On 2014/02/14 17:36:07, oshima wrote:
> On 2014/02/14 17:23:14, sadrul wrote:
> > Addressed the comments, and tested on device. Seems to be working correctly.
> > 
> > I moved the rewriter in chrome/browser/chroemos/ since the
> > keyboard_driven_event_rewriter also lives in there. Maybe all the event
> related
> > code in c/b/chromeos/ could move into c/b/chromeos/events/ (including
> > system_key_event_listener, xinput_hierarchy_changed_event_listener).
> 
> Being under c/b/ make it impossible to test rewriter using ash_shell.
> Most of them should live in ash/events with a delegate interface.
> It's probably not the scope of this CL though.

Yes. The main scope of this CL is to make sure the rewrite happens before
dispatching starts.

Ideally, ui::Events would include enough data for the rewriter so that the
rewriter can work on ui::Events, instead of on raw XEvents.

[stevenjb, 2014-02-14 17:47:28]

Does it make sense to create a c/b/chromeos/events subdirectory for
event_rewriter*, keyboard_driven_event_rewriter*, and maybe
ximput_hierarch_changed_event_listener*?

[sadrul, 2014-02-14 17:48:51]

On 2014/02/14 17:47:28, stevenjb wrote:
> Does it make sense to create a c/b/chromeos/events subdirectory for
> event_rewriter*, keyboard_driven_event_rewriter*, and maybe
> ximput_hierarch_changed_event_listener*?

Yep, that sounds reasonable to me
(https://codereview.chromium.org/165463002/#msg4). I will do the bulk-move in a
follow-up CL.

[stevenjb, 2014-02-14 17:52:17]

Sounds good, lgtm

[sadrul, 2014-02-14 19:10:28]

Committed patchset #4 manually as r251382 (presubmit successful).

[brianderson, 2014-02-14 21:58:12]

This patch broke the linux chrome os build. After lauching, the following error
shows up when I first start to mouse over the window:

[3943:3943:0214/135331:FATAL:event_rewriter.cc(858)] Check failed:
xievent->evtype == XI_ButtonPress || xievent->evtype == XI_ButtonRelease. 
Aborted (core dumped)

Reverting this patch made the error go away.

Note, I built with the following gyp args:
GYP_DEFINES='chromeos=1 dcheck_always_on=1 clang=1' build/gyp_chromium

[oshima, 2014-02-14 22:31:10]

On 2014/02/14 21:58:12, Brian Anderson wrote:
> This patch broke the linux chrome os build. After lauching, the following
error
> shows up when I first start to mouse over the window:
> 
> [3943:3943:0214/135331:FATAL:event_rewriter.cc(858)] Check failed:
> xievent->evtype == XI_ButtonPress || xievent->evtype == XI_ButtonRelease. 
> Aborted (core dumped)
> 
> Reverting this patch made the error go away.
> 
> Note, I built with the following gyp args:
> GYP_DEFINES='chromeos=1 dcheck_always_on=1 clang=1' build/gyp_chromium

Yep, I'm seeing the same. sadrul, I'll revert this soon. let me know if you
already have a fix.

[sadrul, 2014-02-16 04:22:07]

Committed patchset #5 manually as r251607 (presubmit successful).