InputManager/AcceleratorManager for athena

InputManager/AcceleratorManager for athena

InputManager provides ToplevelEventTarget and AcceleratorManager.

Added test short cut (ctrl-l) to show the home card.

BUG=375534
TEST=covered by unit tests
R=sadrul@chromium.org, sky@chromium.org

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

[Jun Mukai, 2014-06-04 04:02:34]

https://codereview.chromium.org/305873002/diff/180001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/305873002/diff/180001/athena/home/home_card_i...
athena/home/home_card_impl.cc:135: AF_NONE},
I am not sure if splitting the table into each of the handlers would be a better
design than having a single table of all accelerators.

At least to me for now, having a giant table would be slightly beneficial
because the developers can easily check if there is an accelerator for a
specific key combination; and easier to create the keyboard shortcut overlays /
helpers.

https://codereview.chromium.org/305873002/diff/180001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/305873002/diff/180001/athena/main/athena_laun...
athena/main/athena_launcher.cc:52: //
athena::InputManager::Create()->OnRootWindowDestroying(root_window);
remove this

[oshima, 2014-06-04 04:24:27]

https://codereview.chromium.org/305873002/diff/180001/athena/home/home_card_i...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/305873002/diff/180001/athena/home/home_card_i...
athena/home/home_card_impl.cc:135: AF_NONE},
On 2014/06/04 04:02:34, Jun Mukai wrote:
> I am not sure if splitting the table into each of the handlers would be a
better
> design than having a single table of all accelerators.
> 
> At least to me for now, having a giant table would be slightly beneficial
> because the developers can easily check if there is an accelerator for a
> specific key combination; and easier to create the keyboard shortcut overlays
/
> helpers.

That's actually not true. ChromeOS+ash already has two tables, one in ash and
another in
browser. We're relying on a runtime check and a unit test to make sure that
there is no overlaps. If you never hit these failures, then you were just lucky.
(It did hit me in the past)

And having global table breaks modularity, which I want to avoid in athena.

https://codereview.chromium.org/305873002/diff/180001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/305873002/diff/180001/athena/main/athena_laun...
athena/main/athena_launcher.cc:52: //
athena::InputManager::Create()->OnRootWindowDestroying(root_window);
On 2014/06/04 04:02:34, Jun Mukai wrote:
> remove this

Oops, done. Thanks.

[sadrul, 2014-06-04 04:31:48]

https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
File athena/input/public/accelerator_manager.h (right):

https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
athena/input/public/accelerator_manager.h:50: class AcceleratorManager {
Can we use the existing ui:AcceleratorManager and ui::AcceleratorTarget?

[oshima, 2014-06-04 05:01:06]

On 2014/06/04 04:31:48, sadrul wrote:
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> File athena/input/public/accelerator_manager.h (right):
> 
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> athena/input/public/accelerator_manager.h:50: class AcceleratorManager {
> Can we use the existing ui:AcceleratorManager and ui::AcceleratorTarget?

They're used internally. This adds features that we need in shell/wm
environment.

[sadrul, 2014-06-04 05:09:10]

On 2014/06/04 05:01:06, oshima wrote:
> On 2014/06/04 04:31:48, sadrul wrote:
> >
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> > File athena/input/public/accelerator_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> > athena/input/public/accelerator_manager.h:50: class AcceleratorManager {
> > Can we use the existing ui:AcceleratorManager and ui::AcceleratorTarget?
> 
> They're used internally. This adds features that we need in shell/wm
> environment.

Is there a reason to have a mirror API for athena?
ui::AcceleratorManager/AcceleratorTarget are in ui/base/, so athena (or wm)
should be able to use it directly?

[oshima, 2014-06-04 05:21:22]

On 2014/06/04 05:09:10, sadrul wrote:
> On 2014/06/04 05:01:06, oshima wrote:
> > On 2014/06/04 04:31:48, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> > > File athena/input/public/accelerator_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/305873002/diff/220001/athena/input/public/acc...
> > > athena/input/public/accelerator_manager.h:50: class AcceleratorManager {
> > > Can we use the existing ui:AcceleratorManager and ui::AcceleratorTarget?
> > 
> > They're used internally. This adds features that we need in shell/wm
> > environment.
> 
> Is there a reason to have a mirror API for athena?
> ui::AcceleratorManager/AcceleratorTarget are in ui/base/, so athena (or wm)
> should be able to use it directly?

It's not mirror. It does more than what ui/base/accelerator does, like ash's
AcceleratorController. I'd like to use this with ash in the future, but that's
not ready yet.

 I'm using XxxManager to match other names in athena. If it's confusing,
I can change to XxxController.

[sadrul, 2014-06-04 22:57:52]

Can you briefly document how the various pieces fit together? I think I
understand the current flow in the CL (although the nested message-loop case is
still somewhat unclear), but a clearer doc would be useful.

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:25:
NestedAcceleratorDelegate(AcceleratorManagerImpl* accelerator_manager)
explicit

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:45:
AcceleratorDelegate(AcceleratorManagerImpl* accelerator_manager)
ditto

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:50: virtual void PreProcessAccelerator(
// wm::AcceleratorDelegate:

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:119:
nested_accelerator_controller_.get());
Is |nested_...| supposed to be active only for nested message-loop?

https://codereview.chromium.org/305873002/diff/220001/athena/input/input_mana...
File athena/input/input_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/220001/athena/input/input_mana...
athena/input/input_manager_impl.cc:31: virtual ui::EventTarget*
GetToplevelEventTarget() OVERRIDE { return this; }
This is also in aura::client::EventClient?

[oshima, 2014-06-04 23:35:54]

added comments. please let me know if you need more detailed description.

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:25:
NestedAcceleratorDelegate(AcceleratorManagerImpl* accelerator_manager)
On 2014/06/04 22:57:53, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:45:
AcceleratorDelegate(AcceleratorManagerImpl* accelerator_manager)
On 2014/06/04 22:57:53, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/305873002/diff/220001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:119:
nested_accelerator_controller_.get());
On 2014/06/04 22:57:53, sadrul wrote:
> Is |nested_...| supposed to be active only for nested message-loop?

Yes.

https://codereview.chromium.org/305873002/diff/220001/athena/input/input_mana...
File athena/input/input_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/220001/athena/input/input_mana...
athena/input/input_manager_impl.cc:31: virtual ui::EventTarget*
GetToplevelEventTarget() OVERRIDE { return this; }
On 2014/06/04 22:57:53, sadrul wrote:
> This is also in aura::client::EventClient?

Yes. I can rename it to something else if you want.

[oshima, 2014-06-05 18:32:07]

ping?

[sadrul, 2014-06-05 19:24:33]

A coupla nits. LGTM

https://codereview.chromium.org/305873002/diff/290001/athena/input/input_mana...
File athena/input/input_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/290001/athena/input/input_mana...
athena/input/input_manager_impl.cc:31: virtual ui::EventTarget*
GetToplevelEventTarget() OVERRIDE { return this; }
Let's call this something else to disambiguate from EventClient interface. (I
don't have any suggestions)

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
File athena/input/public/input_manager.h (right):

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
athena/input/public/input_manager.h:15: class Accelerator;
This doesn't look necessary here.

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
athena/input/public/input_manager.h:24: // Creates and deletes the singleton
object of the HomeCard
InputManager

[oshima, 2014-06-05 20:36:41]

https://codereview.chromium.org/305873002/diff/290001/athena/input/input_mana...
File athena/input/input_manager_impl.cc (right):

https://codereview.chromium.org/305873002/diff/290001/athena/input/input_mana...
athena/input/input_manager_impl.cc:31: virtual ui::EventTarget*
GetToplevelEventTarget() OVERRIDE { return this; }
On 2014/06/05 19:24:32, sadrul wrote:
> Let's call this something else to disambiguate from EventClient interface. (I
> don't have any suggestions)

Changed to TopmostEventTarget().

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
File athena/input/public/input_manager.h (right):

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
athena/input/public/input_manager.h:15: class Accelerator;
On 2014/06/05 19:24:33, sadrul wrote:
> This doesn't look necessary here.

Done.

https://codereview.chromium.org/305873002/diff/290001/athena/input/public/inp...
athena/input/public/input_manager.h:24: // Creates and deletes the singleton
object of the HomeCard
On 2014/06/05 19:24:33, sadrul wrote:
> InputManager

Done.

[oshima, 2014-06-05 20:36:58]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-05 20:39:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/305873002/310001

[oshima, 2014-06-05 21:27:26]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-06-05 21:28:31]

ben can you approve DEPS change?

ui/base
ui/wm/core
ui/wm/public

[sky, 2014-06-06 21:02:54]

LGTM

[oshima, 2014-06-06 22:25:47]

Committed patchset #9 manually as r275545 (presubmit successful).