Wires up registering accelerators from mash with the wm

Wires up registering accelerators from mash with the wm

AcceleratorControllerRegistrar is responsible for registering
accelerators known to ash (in AcceleratorController) with the wm. In addition
AcceleratorRegistrarImpl now registers keyboard accelerators with
AcceleratorController.

Accelerators are now registered for pre and post. This better matches
how ash/chrome interact to process accelerators. See comment in
AcceleratorControllerRegistrar for details on this.

BUG=612331, 631545
TEST=none
R=sadrul@chromium.org

Committed: https://crrev.com/f0891a90a6cdb92d0a205d17df42acd81e796a30
Cr-Commit-Position: refs/heads/master@{#408205}

[sky, 2016-07-26 23:36:19]

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

[commit-bot: I haz the power, 2016-07-26 23:36:52]

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

[sky, 2016-07-26 23:37:18]

I'm going to look at porting existing test coverage next.

I'm not too happy about having AcceleratorRegistrarImpl provide double duty. I
think it better to have an interface explicitly for keyboard accelerators that
appdriver/chrome can use, and another for the general purpose EventMatcher.
WDYT?

[commit-bot: I haz the power, 2016-07-27 00:43:15]

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

[commit-bot: I haz the power, 2016-07-27 00:43:16]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-07-27 17:20:01]

lgtm

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_registrar.cc (right):

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.cc:130: }
We install a fair number of accelerators during startup. Should we worry about
the burst of ipcs we would send? Should we consider bundling these into
smaller/a single ipc?

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.cc:156: DCHECK(ids_.size()
< std::numeric_limits<uint16_t>::max());
DCHECK_LT?

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_registrar.h (right):

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.h:61: //
AshFocusManagerFactory.
This seems a bit unfortunate :(

We may be able to avoid installing post- accelerators for some of these. e.g.
looks like we always process accelerators with cmd-down in the pre-target phase,
and a number of 'reserved' accelerators. We probably don't need to install
post-accelerators for these.

[sky, 2016-07-27 17:39:03]

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_registrar.cc (right):

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.cc:130: }
On 2016/07/27 17:20:00, sadrul wrote:
> We install a fair number of accelerators during startup. Should we worry about
> the burst of ipcs we would send? Should we consider bundling these into
> smaller/a single ipc?

Definitely. I filed 632050 and 632049 for this.

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.cc:156: DCHECK(ids_.size()
< std::numeric_limits<uint16_t>::max());
On 2016/07/27 17:20:00, sadrul wrote:
> DCHECK_LT?

Done.

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
File ash/mus/accelerators/accelerator_controller_registrar.h (right):

https://codereview.chromium.org/2187703003/diff/20001/ash/mus/accelerators/ac...
ash/mus/accelerators/accelerator_controller_registrar.h:61: //
AshFocusManagerFactory.
On 2016/07/27 17:20:00, sadrul wrote:
> This seems a bit unfortunate :(
> 
> We may be able to avoid installing post- accelerators for some of these. e.g.
> looks like we always process accelerators with cmd-down in the pre-target
phase,
> and a number of 'reserved' accelerators. We probably don't need to install
> post-accelerators for these.

Once we make it so that all are registered in a single IPC there likely isn't
much value in the optimization.

[sky, 2016-07-27 18:06:49]

The CQ bit was checked by sky@chromium.org

[sky, 2016-07-27 18:06:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2187703003/#ps60001
(title: "merge")

[commit-bot: I haz the power, 2016-07-27 18:07:20]

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, 2016-07-27 19:00:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 19:01:55]

Description was changed from

==========
Wires up registering accelerators from mash with the wm

AcceleratorControllerRegistrar is responsible for registering
accelerators known to ash (in AcceleratorController) with the wm. In addition
AcceleratorRegistrarImpl now registers keyboard accelerators with
AcceleratorController.

Accelerators are now registered for pre and post. This better matches
how ash/chrome interact to process accelerators. See comment in
AcceleratorControllerRegistrar for details on this.

BUG=612331,631545
TEST=none
R=sadrul@chromium.org
==========

to

==========
Wires up registering accelerators from mash with the wm

AcceleratorControllerRegistrar is responsible for registering
accelerators known to ash (in AcceleratorController) with the wm. In addition
AcceleratorRegistrarImpl now registers keyboard accelerators with
AcceleratorController.

Accelerators are now registered for pre and post. This better matches
how ash/chrome interact to process accelerators. See comment in
AcceleratorControllerRegistrar for details on this.

BUG=612331,631545
TEST=none
R=sadrul@chromium.org

Committed: https://crrev.com/f0891a90a6cdb92d0a205d17df42acd81e796a30
Cr-Commit-Position: refs/heads/master@{#408205}
==========

[commit-bot: I haz the power, 2016-07-27 19:01:57]

Patchset 4 (id:??) landed as
https://crrev.com/f0891a90a6cdb92d0a205d17df42acd81e796a30
Cr-Commit-Position: refs/heads/master@{#408205}