Add ShellAcceleratorController that manages global keyboard accelerators (2nd try).

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.

The 1st attempt (http://crrev.com/110637) broke aura_shell_unittests and this fixes it.
The differences are
- moving ShellAcceleratorFilter to Shell from DesktopEventFilter, and
- adding the ShellAcceleratorFilter in Shell initialization code to DesktopEventFilter.

BUG=97255
TEST=Manual, aura_shell_unittests succeeds.


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

[mazda, 2011-11-18 11:49:59]

The previous change broke DesktopEventFilterTest because ShellAcceleratorFilter
is used without creating the Shell instance in the unit test, which caused crash
when trying to access the instance with Shell::GetInstance.
This CL fixed it. The differences are
- Allowing Shell::GetInstance() to be called before Shell::CreateInstance called
(returns NULL in such a case), and 
- Adding NULL check for Shell::GetInstance() in
ShellAcceleratorFilter::PreHandleKeyEvent. 

Please take another look.

Thanks!

[Ben Goodger (Google), 2011-11-18 15:55:25]

In this case I'd rather you not create the accelerator filter in the DEF, and
rather create it in the shell and attach it to the DEF. This is a less invasive
change than allowing GetInstance() to return NULL.

[mazda, 2011-11-21 03:57:00]

I modified the CL to create ShellAcceleratorFilter in Shell instead of
DesktopEventFilter and reverted the change of Shell::GetInstance()

Could you take another look at the change?

Here is the diff from the 1st change.
http://codereview.chromium.org/8465021/

diff --git a/ui/aura_shell/desktop_event_filter.cc
b/ui/aura_shell/desktop_event_filter.cc
index 67a75ea..dc4b4cf 100644
--- a/ui/aura_shell/desktop_event_filter.cc
+++ b/ui/aura_shell/desktop_event_filter.cc
@@ -9,7 +9,6 @@
 #include "ui/aura/focus_manager.h"
 #include "ui/aura/window_delegate.h"
 #include "ui/aura_shell/shell.h"
-#include "ui/aura_shell/shell_accelerator_filter.h"
 #include "ui/aura_shell/stacking_controller.h"
 #include "ui/base/hit_test.h"
 
@@ -44,16 +43,13 @@ gfx::NativeCursor CursorForWindowComponent(int
window_component) {
 // DesktopEventFilter, public:
 
 DesktopEventFilter::DesktopEventFilter()
-    : EventFilter(aura::Desktop::GetInstance()),
-      shell_accelerator_filter_(new ShellAcceleratorFilter) {
-  AddFilter(shell_accelerator_filter_.get());
+    : EventFilter(aura::Desktop::GetInstance()) {
 }
 
 DesktopEventFilter::~DesktopEventFilter() {
   // Additional filters are not owned by DesktopEventFilter and they
   // should all be removed when running here. |filters_| has
   // check_empty == true and will DCHECK failure if it is not empty.
-  RemoveFilter(shell_accelerator_filter_.get());
 }
 
 void DesktopEventFilter::AddFilter(aura::EventFilter* filter) {
diff --git a/ui/aura_shell/desktop_event_filter.h
b/ui/aura_shell/desktop_event_filter.h
index a9775d8..e4d1c45 100644
--- a/ui/aura_shell/desktop_event_filter.h
+++ b/ui/aura_shell/desktop_event_filter.h
@@ -7,7 +7,6 @@
 #pragma once
 
 #include "base/compiler_specific.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "ui/aura/event_filter.h"
 #include "ui/aura_shell/aura_shell_export.h"
@@ -15,8 +14,6 @@
 namespace aura_shell {
 namespace internal {
 
-class ShellAcceleratorFilter;
-
 // DesktopEventFilter gets all desktop events first and can provide actions to
 // those events. It implements desktop features such as click to activate a
 // window and cursor change when moving mouse.
@@ -59,9 +56,6 @@ class AURA_SHELL_EXPORT DesktopEventFilter : public
aura::EventFilter {
   // Additional event filters that pre-handles events.
   ObserverList<aura::EventFilter, true> filters_;
 
-  // An event filter that pre-handles global accelerators.
-  scoped_ptr<ShellAcceleratorFilter> shell_accelerator_filter_;
-
   DISALLOW_COPY_AND_ASSIGN(DesktopEventFilter);
 };
 
diff --git a/ui/aura_shell/shell.cc b/ui/aura_shell/shell.cc
index 21b04ab..4d95a25 100644
--- a/ui/aura_shell/shell.cc
+++ b/ui/aura_shell/shell.cc
@@ -19,8 +19,9 @@
 #include "ui/aura_shell/drag_drop_controller.h"
 #include "ui/aura_shell/launcher/launcher.h"
 #include "ui/aura_shell/modal_container_layout_manager.h"
-#include "ui/aura_shell/shelf_layout_controller.h"
 #include "ui/aura_shell/shell_accelerator_controller.h"
+#include "ui/aura_shell/shell_accelerator_filter.h"
+#include "ui/aura_shell/shelf_layout_controller.h"
 #include "ui/aura_shell/shell_delegate.h"
 #include "ui/aura_shell/shell_factory.h"
 #include "ui/aura_shell/shell_window_ids.h"
@@ -108,6 +109,8 @@ Shell::Shell(ShellDelegate* delegate)
 }
 
 Shell::~Shell() {
+  RemoveDesktopEventFilter(accelerator_filter_.get());
+
   // Drag drop controller needs a valid shell instance. We destroy it first.
   drag_drop_controller_.reset();
 
@@ -189,6 +192,10 @@ void Shell::Init() {
   // Force a layout.
   desktop_layout->OnWindowResized();
 
+  // Initialize ShellAcceleratorFilter
+  accelerator_filter_.reset(new internal::ShellAcceleratorFilter);
+  AddDesktopEventFilter(accelerator_filter_.get());
+
   // Initialize drag drop controller.
   drag_drop_controller_.reset(new internal::DragDropController);
   aura::Desktop::GetInstance()->SetProperty(aura::kDesktopDragDropClientKey,
diff --git a/ui/aura_shell/shell.h b/ui/aura_shell/shell.h
index 159e96b..fe386c8 100644
--- a/ui/aura_shell/shell.h
+++ b/ui/aura_shell/shell.h
@@ -32,6 +32,7 @@ class ShellDelegate;
 
 namespace internal {
 class DragDropController;
+class ShellAcceleratorFilter;
 class ShelfLayoutController;
 class WorkspaceController;
 }
@@ -95,6 +96,9 @@ class AURA_SHELL_EXPORT Shell {
   scoped_ptr<internal::WorkspaceController> workspace_controller_;
   scoped_ptr<internal::ShelfLayoutController> shelf_layout_controller_;
 
+  // An event filter that pre-handles global accelerators.
+  scoped_ptr<internal::ShellAcceleratorFilter> accelerator_filter_;
+
   DISALLOW_COPY_AND_ASSIGN(Shell);
 };

[Ben Goodger (Google), 2011-11-21 16:57:12]

LGTM for those diffs.

-Ben

On Sun, Nov 20, 2011 at 7:57 PM, <mazda@chromium.org> wrote:

> I modified the CL to create ShellAcceleratorFilter in Shell instead of
> DesktopEventFilter and reverted the change of Shell::GetInstance()
>
> Could you take another look at the change?
>
> Here is the diff from the 1st change.
>
http://codereview.chromium.**org/8465021/<http://codereview.chromium.org/8465...
>
> diff --git a/ui/aura_shell/desktop_event_**filter.cc
> b/ui/aura_shell/desktop_event_**filter.cc
> index 67a75ea..dc4b4cf 100644
> --- a/ui/aura_shell/desktop_event_**filter.cc
> +++ b/ui/aura_shell/desktop_event_**filter.cc
> @@ -9,7 +9,6 @@
>  #include "ui/aura/focus_manager.h"
>  #include "ui/aura/window_delegate.h"
>  #include "ui/aura_shell/shell.h"
> -#include "ui/aura_shell/shell_**accelerator_filter.h"
>  #include "ui/aura_shell/stacking_**controller.h"
>  #include "ui/base/hit_test.h"
>
> @@ -44,16 +43,13 @@ gfx::NativeCursor CursorForWindowComponent(int
> window_component) {
>  // DesktopEventFilter, public:
>
>  DesktopEventFilter::**DesktopEventFilter()
> -    : EventFilter(aura::Desktop::**GetInstance()),
> -      shell_accelerator_filter_(new ShellAcceleratorFilter) {
> -  AddFilter(shell_accelerator_**filter_.get());
> +    : EventFilter(aura::Desktop::**GetInstance()) {
>  }
>
>  DesktopEventFilter::~**DesktopEventFilter() {
>   // Additional filters are not owned by DesktopEventFilter and they
>   // should all be removed when running here. |filters_| has
>   // check_empty == true and will DCHECK failure if it is not empty.
> -  RemoveFilter(shell_**accelerator_filter_.get());
>  }
>
>  void DesktopEventFilter::AddFilter(**aura::EventFilter* filter) {
> diff --git a/ui/aura_shell/desktop_event_**filter.h
> b/ui/aura_shell/desktop_event_**filter.h
> index a9775d8..e4d1c45 100644
> --- a/ui/aura_shell/desktop_event_**filter.h
> +++ b/ui/aura_shell/desktop_event_**filter.h
> @@ -7,7 +7,6 @@
>  #pragma once
>
>  #include "base/compiler_specific.h"
> -#include "base/memory/scoped_ptr.h"
>  #include "base/observer_list.h"
>  #include "ui/aura/event_filter.h"
>  #include "ui/aura_shell/aura_shell_**export.h"
> @@ -15,8 +14,6 @@
>  namespace aura_shell {
>  namespace internal {
>
> -class ShellAcceleratorFilter;
> -
>  // DesktopEventFilter gets all desktop events first and can provide
> actions to
>  // those events. It implements desktop features such as click to activate
> a
>  // window and cursor change when moving mouse.
> @@ -59,9 +56,6 @@ class AURA_SHELL_EXPORT DesktopEventFilter : public
> aura::EventFilter {
>   // Additional event filters that pre-handles events.
>   ObserverList<aura::**EventFilter, true> filters_;
>
> -  // An event filter that pre-handles global accelerators.
> -  scoped_ptr<**ShellAcceleratorFilter> shell_accelerator_filter_;
> -
>   DISALLOW_COPY_AND_ASSIGN(**DesktopEventFilter);
>  };
>
> diff --git a/ui/aura_shell/shell.cc b/ui/aura_shell/shell.cc
> index 21b04ab..4d95a25 100644
> --- a/ui/aura_shell/shell.cc
> +++ b/ui/aura_shell/shell.cc
> @@ -19,8 +19,9 @@
>  #include "ui/aura_shell/drag_drop_**controller.h"
>  #include "ui/aura_shell/launcher/**launcher.h"
>  #include "ui/aura_shell/modal_**container_layout_manager.h"
> -#include "ui/aura_shell/shelf_layout_**controller.h"
>  #include "ui/aura_shell/shell_**accelerator_controller.h"
> +#include "ui/aura_shell/shell_**accelerator_filter.h"
> +#include "ui/aura_shell/shelf_layout_**controller.h"
>  #include "ui/aura_shell/shell_delegate.**h"
>  #include "ui/aura_shell/shell_factory.**h"
>  #include "ui/aura_shell/shell_window_**ids.h"
> @@ -108,6 +109,8 @@ Shell::Shell(ShellDelegate* delegate)
>  }
>
>  Shell::~Shell() {
> +  RemoveDesktopEventFilter(**accelerator_filter_.get());
> +
>   // Drag drop controller needs a valid shell instance. We destroy it
> first.
>   drag_drop_controller_.reset();
>
> @@ -189,6 +192,10 @@ void Shell::Init() {
>   // Force a layout.
>   desktop_layout->**OnWindowResized();
>
> +  // Initialize ShellAcceleratorFilter
> +  accelerator_filter_.reset(new internal::**ShellAcceleratorFilter);
> +  AddDesktopEventFilter(**accelerator_filter_.get());
> +
>   // Initialize drag drop controller.
>   drag_drop_controller_.reset(**new internal::DragDropController);
>   aura::Desktop::GetInstance()->**SetProperty(aura::**
> kDesktopDragDropClientKey,
> diff --git a/ui/aura_shell/shell.h b/ui/aura_shell/shell.h
> index 159e96b..fe386c8 100644
> --- a/ui/aura_shell/shell.h
> +++ b/ui/aura_shell/shell.h
> @@ -32,6 +32,7 @@ class ShellDelegate;
>
>  namespace internal {
>  class DragDropController;
> +class ShellAcceleratorFilter;
>  class ShelfLayoutController;
>  class WorkspaceController;
>  }
> @@ -95,6 +96,9 @@ class AURA_SHELL_EXPORT Shell {
>   scoped_ptr<internal::**WorkspaceController> workspace_controller_;
>   scoped_ptr<internal::**ShelfLayoutController> shelf_layout_controller_;
>
> +  // An event filter that pre-handles global accelerators.
> +  scoped_ptr<internal::**ShellAcceleratorFilter> accelerator_filter_;
> +
>   DISALLOW_COPY_AND_ASSIGN(**Shell);
>  };
>
>
>
>
http://codereview.chromium.**org/8561012/<http://codereview.chromium.org/8561...
>

[commit-bot: I haz the power, 2011-11-21 18:10:33]

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

[commit-bot: I haz the power, 2011-11-21 18:37:19]

Try job failure for 8561012-3001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-22 03:13:10]

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

[commit-bot: I haz the power, 2011-11-22 05:08:31]

Change committed as 111100