Convert KeyboardDrivenEventRewriter to a ui::EventRewriter.

Convert KeyboardDrivenEventRewriter to a ui::EventRewriter.

KeyboardDrivenEventRewriter was previously called at the start of
chromeos::EventRewriter, and suppressed further rewriting if it made
any changes. Since EventSource follows that model of breaking out of
the rewriter list, it makes sense to have this be a ui::EventRewriter
itself.

EventRewriterController will also be used for chromeos::EventRewriter
when that is converted to a ui::EventRewriter.

BUG=354034
TEST=unit_test
R=derat@chromium.org, sadrul@chromium.org

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

[kpschoedel, 2014-04-17 21:08:21]

No hurry -- Friday is a stat holiday here, and our building power will be off
for maintenance.

This CL assumes WindowTreeHost::GetEventSource() from
https://codereview.chromium.org/240333007/

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

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:23: return
system::InputDeviceSettings::Get()
It looks like this part of the test is fixed per platform, in which case we
could install this rewriter only in that case, and save the latency otherwise.

[Daniel Erat, 2014-04-18 02:50:06]

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

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.cc:23: for
(aura::Window::Windows::iterator i = windows.begin(), e = windows.end();
nit: i haven't seen this style used in chromium before. something like this is
more common:

  for (...::iterator it = windows.begin();
       it != windows.end(); ++it)

i.e. no need to pre-cache the end iterator, and 'i' is usually just used for int
iterators, with 'it' or 'iter' used for stl iterators

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.cc:26:
AddToEventSource((*i)->GetHost()->GetEventSource());
do you need to unregister it later?

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.cc:37:
source->AddEventRewriter(rewriter_.get());
do you need to unregister it later?

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/event_rewriter_controller.h (right):

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.h:21: explicit
EventRewriterController(ui::EventRewriter* rewriter);
change the argument to a scoped_ptr to make it obvious that ownership is being
transferred (and then use .Pass() at the calling site)

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.h:22:
~EventRewriterController();
virtual

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

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:66:
event.type() == ui::ET_KEY_RELEASED);
nit: include the unexected event type in an error message here?

    << "Unexpected event type " << event.type();

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:80:
rewritten_event->get()->set_flags(
does (*rewritten_event)->set_flags() work here?

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

Looking pretty good!

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:74:
scoped_ptr<EventRewriterController> keyboard_driven_event_rewriter_;
Does this need to be inside USE_X11?

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
File chrome/browser/chromeos/events/event_rewriter_controller.h (right):

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.h:19: class
EventRewriterController : public aura::EnvObserver {
Add doc comments

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/even...
chrome/browser/chromeos/events/event_rewriter_controller.h:39: EventSourceList
event_sources_;
I assume you keep this list so that you can remove the rewriter when needed. But
do you need to keep track of this? Can you just iterate over the root-windows in
Shell and remove from each instead?

[sadrul, 2014-04-22 16:16:23]

Oh, another concern I had: currently, the chromeos::KeyboardDrivenEventRewriter
gets a chance to process an event before chromeos::EventRewriter. This patch
changes that ordering. I understand once chromeos::EventRewriter is converted to
become a ui::EventRewriter, this will not be an issue anymore. In the meantime,
can this change in ordering break something?

[kpschoedel, 2014-04-22 19:51:56]

On 2014/04/22 16:16:23, sadrul wrote:
> Oh, another concern I had: currently, the
chromeos::KeyboardDrivenEventRewriter
> gets a chance to process an event before chromeos::EventRewriter. This patch
> changes that ordering. I understand once chromeos::EventRewriter is converted
to
> become a ui::EventRewriter, this will not be an issue anymore. In the
meantime,
> can this change in ordering break something?

Yes, F6 in particular. The intent is to hold off committing this until
chromeos::EventRewriter is also ready; that should be up for review shortly, as
soon as I straighten out my tree. (This CL also depends on
WindowTreeHost::GetEventSource().)

[kpschoedel, 2014-04-22 19:53:07]

Addressed review comments.

Changed EventRewriterController to own all (here, soon to be two) relevant
rewriters, to preserve ordering.

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/240173007/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:433: #if
defined(USE_X11)
Sorry, didn't notice the upstream change in time.

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_controller.h (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:24: // added to this.
Changed to have one EventRewriterController handle all associated rewriters
instead of one each, because it is this that is informed of new root windows
(i.e. |EventSource|s) and rewriters have an order.

https://codereview.chromium.org/240173007/diff/80001/chrome/chrome_browser_ch...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/chrome_browser_ch...
chrome/chrome_browser_chromeos.gypi:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Sync only. I'll do this right next time.

[sadrul, 2014-04-23 19:32:53]

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

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:116: #include
"chrome/browser/chromeos/events/keyboard_driven_event_rewriter.h"
These should move out of the USE_X11 block?

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.cc:13:
EventRewriterController::EventRewriterController() : initialized_(false) {
Initialize operational_

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.cc:40: operational_ =
false;
It's unclear to me why we need both operational_ and initialized_. Can we use
just one?

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_controller.h (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:27: explicit
EventRewriterController();
Don't need explicit

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:34: // after
ash::Shell has been initialized.
I am curious to know why this is necessary. Is it because we create this
controller before we create the ash::Shell?

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:37: void Shutdown();
Document (Destroys and removes all event-rewriters?)

https://codereview.chromium.org/240173007/diff/80001/chrome/chrome_browser_ch...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/chrome_browser_ch...
chrome/chrome_browser_chromeos.gypi:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/04/22 19:53:08, kpschoedel wrote:
> Sync only. I'll do this right next time.

I don't think you need to change these for existing files in the tree?

https://codereview.chromium.org/240173007/diff/80001/chrome/chrome_browser_ch...
chrome/chrome_browser_chromeos.gypi:1071: ['exclude',
'^browser/chromeos/events/'],
We should probably be more judicious here. (not something you need to address in
this CL, though. We exclude these here because we don't have use_x11 set for
use_ozone. Ideally, we would exclude this for use_x11 == 0 (only the files that
still use X11)).

[sadrul, 2014-04-23 19:33:56]

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

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:712:
scoped_ptr<ui::EventRewriter>(new KeyboardDrivenEventRewriter()).Pass());
From your other CL, you don't need .Pass() here.

[kpschoedel, 2014-04-23 21:33:14]

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

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:116: #include
"chrome/browser/chromeos/events/keyboard_driven_event_rewriter.h"
On 2014/04/23 19:32:53, sadrul wrote:
> These should move out of the USE_X11 block?

Done. Likewise event_rewriter.h in the other CL.

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:712:
scoped_ptr<ui::EventRewriter>(new KeyboardDrivenEventRewriter()).Pass());
On 2014/04/23 19:33:56, sadrul wrote:
> From your other CL, you don't need .Pass() here.

Done.

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.cc:40: operational_ =
false;
On 2014/04/23 19:32:53, sadrul wrote:
> It's unclear to me why we need both operational_ and initialized_. Can we use
> just one?

I'll change to one. I had separate states to enforce that you can't add more
rewriters and/or re-initialize after shutdown, but that's probably overkill.

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter_controller.h (right):

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:27: explicit
EventRewriterController();
On 2014/04/23 19:32:53, sadrul wrote:
> Don't need explicit

Done. (Previous version had 1 arg.)

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:34: // after
ash::Shell has been initialized.
On 2014/04/23 19:32:53, sadrul wrote:
> I am curious to know why this is necessary. Is it because we create this
> controller before we create the ash::Shell?

Partly, but more to ensure that all rewriters have been registered before they
are added to the existing root windows.

https://codereview.chromium.org/240173007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter_controller.h:37: void Shutdown();
On 2014/04/23 19:32:53, sadrul wrote:
> Document (Destroys and removes all event-rewriters?)

Done.

[sadrul, 2014-04-24 22:17:08]

LGTM! (once https://codereview.chromium.org/250103002/ lands, we should be able
to run this through the trybots)

https://codereview.chromium.org/240173007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc (right):

https://codereview.chromium.org/240173007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:67: <<
"Unexpected event type " << event.type();
You can also use event.name()

[kpschoedel, 2014-04-28 14:04:30]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:04:42]

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

[commit-bot: I haz the power, 2014-04-28 14:15:41]

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

[commit-bot: I haz the power, 2014-04-28 14:15:41]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[kpschoedel, 2014-04-28 16:37:58]

ping derat@ — any further issues? 

(This is a dependecy of https://codereview.chromium.org/248343002/ )

[Daniel Erat, 2014-04-28 16:51:55]

sorry, didn't realize you were waiting on me

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_controller.cc:39: operational_ =
false;
is calling AddEventWriter() and Init() again allowed after this point? if not,
i'd prefer that you got rid of the public Shutdown() method (also uncommon in
chrome, apart from in singleton objects) and instead did cleanup in the d'tor:

  ERC::~ERC() {
    // Remove the rewriters from every root window EventSource and destroy them.
    ...
  }

might be clearer to rename |operational_| to |initialized_|, as well.

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc (right):

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:67: <<
"Unexpected event type " << event.type();
nit: indent this four spaces beyond the previous line

[kpschoedel, 2014-04-28 17:23:47]

Addressed latest comments.

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_controller.cc:39: operational_ =
false;
On 2014/04/28 16:51:55, Daniel Erat wrote:
> is calling AddEventWriter() and Init() again allowed after this point? if not,
> i'd prefer that you got rid of the public Shutdown() method (also uncommon in
> chrome, apart from in singleton objects) and instead did cleanup in the d'tor:
> 
>   ERC::~ERC() {
>     // Remove the rewriters from every root window EventSource and destroy
them.
>     ...
>   }
> 
> might be clearer to rename |operational_| to |initialized_|, as well.

Done.

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc (right):

https://codereview.chromium.org/240173007/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/events/keyboard_driven_event_rewriter.cc:67: <<
"Unexpected event type " << event.type();
On 2014/04/28 16:51:55, Daniel Erat wrote:
> nit: indent this four spaces beyond the previous line

Done.

[Daniel Erat, 2014-04-28 18:15:39]

lgtm

[kpschoedel, 2014-04-28 19:36:49]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:38:08]

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

[commit-bot: I haz the power, 2014-04-28 20:50:47]

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

[commit-bot: I haz the power, 2014-04-28 20:50:48]

Try jobs failed on following builders:
  tryserver.chromium on android_aosp

[kpschoedel, 2014-04-29 13:46:22]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-04-29 13:46:34]

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

[commit-bot: I haz the power, 2014-04-29 22:01:07]

Change committed as 266969