More accelerator code leanup

* Remove null check
* Simplify NestedAcceleratorDelegate interface.

BUG=None
R=sky@chromium.org

Review URL: https://codereview.chromium.org/312483002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274560 0039d316-1c4b-4281-b951-d872f2087c98

ash/accelerators/accelerator_delegate.cc

@@ -49,8 +49,7 @@ bool AcceleratorDelegate::ProcessAccelerator(const ui::KeyEvent& key_event,
 // containing parent window will be checked for the property.
 bool AcceleratorDelegate::CanConsumeSystemKeys(const ui::KeyEvent& event) {
   aura::Window* target = static_cast<aura::Window*>(event.target());
-  if (!target)  // Can be NULL in tests.
-    return false;
+  DCHECK(target);
   aura::Window* top_level = ::wm::GetToplevelWindow(target);
   return top_level && wm::GetWindowState(top_level)->can_consume_system_keys();
 }
@@ -61,8 +60,7 @@ bool AcceleratorDelegate::ShouldProcessAcceleratorNow(
     const ui::KeyEvent& event,
     const ui::Accelerator& accelerator) {
   aura::Window* target = static_cast<aura::Window*>(event.target());
-  if (!target)
-    return true;
+  DCHECK(target);
 
   aura::Window::Windows root_windows = Shell::GetAllRootWindows();
   if (std::find(root_windows.begin(), root_windows.end(), target) !=

ash/accelerators/accelerator_filter_unittest.cc

@@ -89,15 +89,24 @@ TEST_F(AcceleratorFilterTest, TestCapsLockMask) {
 TEST_F(AcceleratorFilterTest, CanConsumeSystemKeys) {
   ::wm::AcceleratorFilter filter(
       scoped_ptr< ::wm::AcceleratorDelegate>(new AcceleratorDelegate).Pass());
+  aura::Window* root_window = Shell::GetPrimaryRootWindow();
 
   // Normal keys are not consumed.
   ui::KeyEvent press_a(ui::ET_KEY_PRESSED, ui::VKEY_A, ui::EF_NONE, false);
+  {
+    ui::Event::DispatcherApi dispatch_helper(&press_a);
+    dispatch_helper.set_target(root_window);
+  }
   filter.OnKeyEvent(&press_a);
   EXPECT_FALSE(press_a.stopped_propagation());
 
   // System keys are directly consumed.
   ui::KeyEvent press_mute(
       ui::ET_KEY_PRESSED, ui::VKEY_VOLUME_MUTE, ui::EF_NONE, false);
+  {
+    ui::Event::DispatcherApi dispatch_helper(&press_mute);
+    dispatch_helper.set_target(root_window);
+  }
   filter.OnKeyEvent(&press_mute);
   EXPECT_TRUE(press_mute.stopped_propagation());
 

ash/accelerators/nested_accelerator_delegate.cc

@@ -16,14 +16,14 @@
 namespace ash {
 namespace {
 
-bool IsPossibleAcceleratorNotForMenu(const ui::KeyEvent& key_event) {
+bool IsPossibleAcceleratorNotForMenu(const ui::Accelerator& accelerator) {
   // For shortcuts generated by Ctrl or Alt plus a letter, number or
   // the tab key, we want to exit the context menu first and then
   // repost the event. That allows for the shortcut execution after
   // the context menu has exited.
-  if (key_event.type() == ui::ET_KEY_PRESSED &&
-      (key_event.flags() & (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN))) {
-    const ui::KeyboardCode key_code = key_event.key_code();
+  if (accelerator.type() == ui::ET_KEY_PRESSED &&
+      (accelerator.modifiers() & (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN))) {
+    const ui::KeyboardCode key_code = accelerator.key_code();
     if ((key_code >= ui::VKEY_A && key_code <= ui::VKEY_Z) ||
         (key_code >= ui::VKEY_0 && key_code <= ui::VKEY_9) ||
         (key_code == ui::VKEY_TAB)) {
@@ -33,17 +33,8 @@ bool IsPossibleAcceleratorNotForMenu(const ui::KeyEvent& key_event) {
   return false;
 }
 
-}  // namespace
-
-NestedAcceleratorDelegate::NestedAcceleratorDelegate() {
-}
-
-NestedAcceleratorDelegate::~NestedAcceleratorDelegate() {
-}
-
-bool NestedAcceleratorDelegate::ShouldProcessEventNow(
-    const ui::KeyEvent& key_event) {
-  if (!IsPossibleAcceleratorNotForMenu(key_event))
+bool ShouldProcessAcceleratorNow(const ui::Accelerator& accelerator) {
+  if (!IsPossibleAcceleratorNotForMenu(accelerator))
     return true;
 
   if (views::MenuController* menu_controller =
@@ -54,22 +45,30 @@ bool NestedAcceleratorDelegate::ShouldProcessEventNow(
   return true;
 }
 
-bool NestedAcceleratorDelegate::ProcessEvent(const ui::KeyEvent& key_event) {
+}  // namespace
+
+NestedAcceleratorDelegate::NestedAcceleratorDelegate() {
+}
+
+NestedAcceleratorDelegate::~NestedAcceleratorDelegate() {
+}
+
+NestedAcceleratorDelegate::Result NestedAcceleratorDelegate::ProcessAccelerator(
+    const ui::Accelerator& accelerator) {
+  if (!ShouldProcessAcceleratorNow(accelerator))
+    return RESULT_PROCESS_LATER;
+
   ash::AcceleratorController* accelerator_controller =
       ash::Shell::GetInstance()->accelerator_controller();
   if (!accelerator_controller)
-    return false;
-  const int kModifierMask =
-      (ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN);
-  ui::Accelerator accelerator(key_event.key_code(),
-                              key_event.flags() & kModifierMask);
-  if (key_event.type() == ui::ET_KEY_RELEASED)
-    accelerator.set_type(ui::ET_KEY_RELEASED);
+    return RESULT_NOT_PROCESSED;
+
   // Fill out context object so AcceleratorController will know what
   // was the previous accelerator or if the current accelerator is repeated.
   Shell::GetInstance()->accelerator_controller()->context()->UpdateContext(
       accelerator);
-  return accelerator_controller->Process(accelerator);
+  return accelerator_controller->Process(accelerator) ? RESULT_PROCESSED
+                                                      : RESULT_NOT_PROCESSED;
 }
 
 }  // namespace ash

ash/accelerators/nested_accelerator_delegate.h

@@ -16,8 +16,8 @@ class NestedAcceleratorDelegate : public wm::NestedAcceleratorDelegate {
   virtual ~NestedAcceleratorDelegate();
 
   // wm::AcceleratorDispatcher::Delegate
-  virtual bool ShouldProcessEventNow(const ui::KeyEvent& key_event) OVERRIDE;
-  virtual bool ProcessEvent(const ui::KeyEvent& key_event) OVERRIDE;
+  virtual Result ProcessAccelerator(
+      const ui::Accelerator& accelerator) OVERRIDE;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NestedAcceleratorDelegate);

ui/wm/core/accelerator_filter.cc

@@ -11,9 +11,6 @@
 namespace wm {
 namespace {
 
-const int kModifierFlagMask =
-    (ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN);
-
 // Returns true if |key_code| is a key usually handled directly by the shell.
 bool IsSystemKey(ui::KeyboardCode key_code) {
 #if defined(OS_CHROMEOS)
@@ -52,14 +49,13 @@ AcceleratorFilter::~AcceleratorFilter() {
 
 void AcceleratorFilter::OnKeyEvent(ui::KeyEvent* event) {
   const ui::EventType type = event->type();
+  DCHECK(event->target());
   if ((type != ui::ET_KEY_PRESSED && type != ui::ET_KEY_RELEASED) ||
-      event->is_char()) {
+      event->is_char() || !event->target()) {
     return;
   }
 
-  ui::Accelerator accelerator(event->key_code(),
-                              event->flags() & kModifierFlagMask);
-  accelerator.set_type(event->type());
+  ui::Accelerator accelerator = CreateAcceleratorFromKeyEvent(*event);
 
   delegate_->PreProcessAccelerator(accelerator);
 
@@ -71,4 +67,15 @@ void AcceleratorFilter::OnKeyEvent(ui::KeyEvent* event) {
     event->StopPropagation();
 }
 
+ui::Accelerator CreateAcceleratorFromKeyEvent(const ui::KeyEvent& key_event) {
+  const int kModifierFlagMask =
+      (ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN);
+
+  ui::Accelerator accelerator(key_event.key_code(),
+                              key_event.flags() & kModifierFlagMask);
+  if (key_event.type() == ui::ET_KEY_RELEASED)
+    accelerator.set_type(ui::ET_KEY_RELEASED);
+  return accelerator;
+}
+
 }  // namespace wm

ui/wm/core/accelerator_filter.h

@@ -10,6 +10,10 @@
 #include "ui/events/event_handler.h"
 #include "ui/wm/wm_export.h"
 
+namespace ui {
+class Accelerator;
+}
+
 namespace wm {
 class AcceleratorDelegate;
 
@@ -29,6 +33,8 @@ class WM_EXPORT AcceleratorFilter : public ui::EventHandler {
   DISALLOW_COPY_AND_ASSIGN(AcceleratorFilter);
 };
 
+ui::Accelerator CreateAcceleratorFromKeyEvent(const ui::KeyEvent& key_event);
+
 }  // namespace wm
 
 #endif  // UI_WM_CORE_ACCELERATOR_FILTER_H_

ui/wm/core/nested_accelerator_controller_unittest.cc

@@ -104,17 +104,10 @@ class MockNestedAcceleratorDelegate : public NestedAcceleratorDelegate {
   virtual ~MockNestedAcceleratorDelegate() {}
 
   // NestedAcceleratorDelegate:
-  virtual bool ShouldProcessEventNow(const ui::KeyEvent& key_event) OVERRIDE {
-    return true;
-  }
-  virtual bool ProcessEvent(const ui::KeyEvent& key_event) OVERRIDE {
-    const int kModifierMask =
-        (ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN);
-    ui::Accelerator accelerator(key_event.key_code(),
-                                key_event.flags() & kModifierMask);
-    if (key_event.type() == ui::ET_KEY_RELEASED)
-      accelerator.set_type(ui::ET_KEY_RELEASED);
-    return accelerator_manager_->Process(accelerator);
+  virtual Result ProcessAccelerator(
+      const ui::Accelerator& accelerator) OVERRIDE {
+    return accelerator_manager_->Process(accelerator) ?
+        RESULT_PROCESSED : RESULT_NOT_PROCESSED;
   }
 
   void Register(const ui::Accelerator& accelerator,

ui/wm/core/nested_accelerator_delegate.h

@@ -6,7 +6,7 @@
 #define UI_WM_CORE_NESTED_ACCELERATOR_DELEGATE_H_
 
 namespace ui {
-class KeyEvent;
+class Accelerator;
 }
 
 namespace wm {
@@ -15,15 +15,18 @@ namespace wm {
 // handling.
 class NestedAcceleratorDelegate {
  public:
-  virtual ~NestedAcceleratorDelegate() {}
+  enum Result {
+    RESULT_PROCESSED,
+    RESULT_NOT_PROCESSED,
+    // The key event should be ignored now and instead be reposted so that
+    // next event loop.
+    RESULT_PROCESS_LATER,
+  };
 
-  // If the key event should be ignored now and instead be reposted so that next
-  // event loop.
-  virtual bool ShouldProcessEventNow(const ui::KeyEvent& key_event) = 0;
+  virtual ~NestedAcceleratorDelegate() {}
 
-  // Attempts to process an accelerator for the key-event.
-  // Returns whether an accelerator was triggered and processed.
-  virtual bool ProcessEvent(const ui::KeyEvent& key_event) = 0;
+  // Attempts to process the |accelerator|.
+  virtual Result ProcessAccelerator(const ui::Accelerator& accelerator) = 0;
 };
 
 }  // namespace wm

ui/wm/core/nested_accelerator_dispatcher_linux.cc

@@ -6,10 +6,12 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
+#include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event.h"
 #include "ui/events/platform/platform_event_dispatcher.h"
 #include "ui/events/platform/platform_event_source.h"
 #include "ui/events/platform/scoped_event_dispatcher.h"
+#include "ui/wm/core/accelerator_filter.h"
 #include "ui/wm/core/nested_accelerator_delegate.h"
 
 #if defined(USE_X11)
@@ -65,17 +67,21 @@ class NestedAcceleratorDispatcherLinux : public NestedAcceleratorDispatcher,
   virtual uint32_t DispatchEvent(const ui::PlatformEvent& event) OVERRIDE {
     if (IsKeyEvent(event)) {
       ui::KeyEvent key_event(event, false);
-      if (!delegate_->ShouldProcessEventNow(key_event)) {
+      ui::Accelerator accelerator = CreateAcceleratorFromKeyEvent(key_event);
+
+      switch (delegate_->ProcessAccelerator(accelerator)) {
+        case NestedAcceleratorDelegate::RESULT_PROCESS_LATER:
 #if defined(USE_X11)
-        XPutBackEvent(event->xany.display, event);
+          XPutBackEvent(event->xany.display, event);
 #else
-        NOTIMPLEMENTED();
+          NOTIMPLEMENTED();
 #endif
-        return ui::POST_DISPATCH_NONE;
+          return ui::POST_DISPATCH_NONE;
+        case NestedAcceleratorDelegate::RESULT_PROCESSED:
+          return ui::POST_DISPATCH_NONE;
+        case NestedAcceleratorDelegate::RESULT_NOT_PROCESSED:
+          break;
       }
-
-      if (delegate_->ProcessEvent(key_event))
-        return ui::POST_DISPATCH_NONE;
     }
     ui::PlatformEventDispatcher* prev = *restore_dispatcher_;
 

ui/wm/core/nested_accelerator_dispatcher_win.cc

@@ -7,7 +7,9 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_pump_dispatcher.h"
 #include "base/run_loop.h"
+#include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event.h"
+#include "ui/wm/core/accelerator_filter.h"
 #include "ui/wm/core/nested_accelerator_delegate.h"
 
 using base::MessagePumpDispatcher;
@@ -41,11 +43,16 @@ class NestedAcceleratorDispatcherWin : public NestedAcceleratorDispatcher,
   virtual uint32_t Dispatch(const MSG& event) OVERRIDE {
     if (IsKeyEvent(event)) {
       ui::KeyEvent key_event(event, false);
-      if (!delegate_->ShouldProcessEventNow(key_event))
-        return POST_DISPATCH_QUIT_LOOP;
-
-      if (delegate_->ProcessEvent(key_event))
-        return POST_DISPATCH_NONE;
+      ui::Accelerator accelerator = CreateAcceleratorFromKeyEvent(key_event);
+
+      switch (delegate_->ProcessAccelerator(accelerator)) {
+        case NestedAcceleratorDelegate::RESULT_PROCESS_LATER:
+          return POST_DISPATCH_QUIT_LOOP;
+        case NestedAcceleratorDelegate::RESULT_PROCESSED:
+          return POST_DISPATCH_NONE;
+        case NestedAcceleratorDelegate::RESULT_NOT_PROCESSED:
+          break;
+      }
     }
 
     return nested_dispatcher_ ? nested_dispatcher_->Dispatch(event)