Add ShellAcceleratorController that managers global keyboard accelerators.

Add ShellAcceleratorController that manages global keyboard accelerators.

- Create ShellAcceleratorController that manages global keyboard accelerators and also processes several accelerators as a target.
- Create ShellAcceleratorFilter, which is used by DesktopEventFilter to handle accelerators.
- Add a function to Shell for accessing ShellAcceleratorController.

BUG=97255
TEST=Manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110637

[mazda, 2011-11-15 13:51:15]

I submitted the CLs on which this CL depended and rebased.
I modified the CL to move the role of registering global keyboard shortcuts to
aura_shell::Shell from aura::Desktop.
I also introduced GlobalAcceleratorFilter, which inherits from EventFilter and
used in DesktopEventFilter for handling accelerator keys.

Could you take another look at the change?

Thanks,

[tfarina, 2011-11-15 15:46:22]

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
File ui/aura_shell/global_accelerator_filter.cc (right):

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:17: }
nit: }  // namespace

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:38:
aura_shell::Shell::GetInstance()->accelerator_manager()->Process(
can you omit aura_shell:: here, since it's already in the aura_shell namespace?

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:57:
////////////////////////////////////////////////////////////////////////////////
nit: Remove this?

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
File ui/aura_shell/global_accelerator_filter.h (right):

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:10: #include "base/observer_list.h"
nit: remove, unused in this header file.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:17: 
nit: remove, extra blank line.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:23: // Overridden from EventFilter:
nit: from aura::EventFilter:

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:32:
DISALLOW_COPY_AND_ASSIGN(GlobalAcceleratorFilter);
nit: basictypes.h

[mazda, 2011-11-15 16:16:29]

Thank you for the review.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
File ui/aura_shell/global_accelerator_filter.cc (right):

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:17: }
On 2011/11/15 15:46:23, tfarina wrote:
> nit: }  // namespace

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:38:
aura_shell::Shell::GetInstance()->accelerator_manager()->Process(
On 2011/11/15 15:46:23, tfarina wrote:
> can you omit aura_shell:: here, since it's already in the aura_shell
namespace?

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.cc:57:
////////////////////////////////////////////////////////////////////////////////
On 2011/11/15 15:46:23, tfarina wrote:
> nit: Remove this?

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
File ui/aura_shell/global_accelerator_filter.h (right):

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:10: #include "base/observer_list.h"
On 2011/11/15 15:46:23, tfarina wrote:
> nit: remove, unused in this header file.

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:17: 
On 2011/11/15 15:46:23, tfarina wrote:
> nit: remove, extra blank line.

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:23: // Overridden from EventFilter:
On 2011/11/15 15:46:23, tfarina wrote:
> nit: from aura::EventFilter:

Done.

http://codereview.chromium.org/8465021/diff/3011/ui/aura_shell/global_acceler...
ui/aura_shell/global_accelerator_filter.h:32:
DISALLOW_COPY_AND_ASSIGN(GlobalAcceleratorFilter);
On 2011/11/15 15:46:23, tfarina wrote:
> nit: basictypes.h

Done.

[Ben Goodger (Google), 2011-11-15 16:28:09]

Excellent change!

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.cc
File ui/aura/desktop.cc (right):

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.cc#newcode411
ui/aura/desktop.cc:411: bool Desktop::AcceleratorPressed(const ui::Accelerator&
accelerator) {
We should just move this to the shell now that you're adding a good API to add
global accelerators.

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.h#newcode134
ui/aura/desktop.h:134: // Overridden from ui::AcceleratorTarget.
: on the end, rather than .

[mazda, 2011-11-16 05:40:08]

Thank you for the review.

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.cc
File ui/aura/desktop.cc (right):

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.cc#newcode411
ui/aura/desktop.cc:411: bool Desktop::AcceleratorPressed(const ui::Accelerator&
accelerator) {
On 2011/11/15 16:28:09, Ben Goodger (Google) wrote:
> We should just move this to the shell now that you're adding a good API to add
> global accelerators.

Done.

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8465021/diff/8002/ui/aura/desktop.h#newcode134
ui/aura/desktop.h:134: // Overridden from ui::AcceleratorTarget.
On 2011/11/15 16:28:09, Ben Goodger (Google) wrote:
> : on the end, rather than .

Done.

[Ben Goodger (Google), 2011-11-16 16:16:10]

Almost there...

http://codereview.chromium.org/8465021/diff/3013/ui/aura_shell/shell.h
File ui/aura_shell/shell.h (right):

http://codereview.chromium.org/8465021/diff/3013/ui/aura_shell/shell.h#newcode42
ui/aura_shell/shell.h:42: class AURA_SHELL_EXPORT Shell : public
ui::AcceleratorTarget {
Rather than making the Shell an accelerator target, create a new class
ShellAcceleratorController to be the target. This class will be where we add any
other global shortcuts.

Once you have this class, move the impls of rotate/fullscreen from aura::Desktop
onto it, so we can pull that code out of aura completely and into aura_shell.

[mazda, 2011-11-16 16:47:48]

On 2011/11/16 16:16:10, Ben Goodger (Google) wrote:
> Almost there...
> 
> http://codereview.chromium.org/8465021/diff/3013/ui/aura_shell/shell.h
> File ui/aura_shell/shell.h (right):
> 
>
http://codereview.chromium.org/8465021/diff/3013/ui/aura_shell/shell.h#newcode42
> ui/aura_shell/shell.h:42: class AURA_SHELL_EXPORT Shell : public
> ui::AcceleratorTarget {
> Rather than making the Shell an accelerator target, create a new class
> ShellAcceleratorController to be the target. This class will be where we add
any
> other global shortcuts.
> 
> Once you have this class, move the impls of rotate/fullscreen from
aura::Desktop
> onto it, so we can pull that code out of aura completely and into aura_shell.

Thanks. I'll make a new class.

One quick question.
ToggleFullscreen uses DesktopHost, which is private to Desktop.
Is it OK to add an accessor function that returns DesktopHost*?

[Ben Goodger (Google), 2011-11-16 17:08:42]

Maybe just add a ToggleFullscreen method to Desktop rather than exposing the
host?

[mazda, 2011-11-16 17:51:25]

On 2011/11/16 17:08:42, Ben Goodger (Google) wrote:
> Maybe just add a ToggleFullscreen method to Desktop rather than exposing the
> host?

Thanks. I'll implement that way.

[mazda, 2011-11-17 12:34:35]

I added ShellAcceleratorController, and renamed GlobalAcceleratorFilter to
ShellAcceleratorFilter for consistency.
Please take another look.

Thanks,

[tfarina, 2011-11-17 12:49:55]

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell.h
File ui/aura_shell/shell.h (right):

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell.h#newco...
ui/aura_shell/shell.h:31: class Launcher;
Launcher should be first ;) sort up!

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
File ui/aura_shell/shell_accelerator_controller.h (right):

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:13: #include
"ui/base/accelerator_manager.h"
nit: you can forward declare this;

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:14: #include
"ui/base/models/accelerator.h"
nit: I wish we could centralize all classes related to accelerator into an
ui/base/accelerators/ or something like that.

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:53:
DISALLOW_COPY_AND_ASSIGN(ShellAcceleratorController);
nit: newline between 52-53

[mazda, 2011-11-17 13:10:16]

Thank you for the comments.

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell.h
File ui/aura_shell/shell.h (right):

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell.h#newco...
ui/aura_shell/shell.h:31: class Launcher;
On 2011/11/17 12:49:55, tfarina wrote:
> Launcher should be first ;) sort up!

Done.

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
File ui/aura_shell/shell_accelerator_controller.h (right):

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:13: #include
"ui/base/accelerator_manager.h"
On 2011/11/17 12:49:55, tfarina wrote:
> nit: you can forward declare this;

Done.

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:14: #include
"ui/base/models/accelerator.h"
On 2011/11/17 12:49:55, tfarina wrote:
> nit: I wish we could centralize all classes related to accelerator into an
> ui/base/accelerators/ or something like that.

OK. I filed an issue for that (http://crbug.com/104599).
I'll do it in another CL.

http://codereview.chromium.org/8465021/diff/14001/ui/aura_shell/shell_acceler...
ui/aura_shell/shell_accelerator_controller.h:53:
DISALLOW_COPY_AND_ASSIGN(ShellAcceleratorController);
On 2011/11/17 12:49:55, tfarina wrote:
> nit: newline between 52-53

Done.

[Ben Goodger (Google), 2011-11-17 20:53:37]

LGTM

[commit-bot: I haz the power, 2011-11-18 01:03:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8465021/17002

[commit-bot: I haz the power, 2011-11-18 02:09:45]

Change committed as 110637