Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_event_filter.h

Index: ui/views/controls/menu/menu_event_filter.h
diff --git a/ui/views/controls/menu/menu_event_filter.h b/ui/views/controls/menu/menu_event_filter.h
new file mode 100644
index 0000000000000000000000000000000000000000..3e1839a6fec9b2adfc6988f3bd084fe4920bab1b
--- /dev/null
+++ b/ui/views/controls/menu/menu_event_filter.h
@@ -0,0 +1,86 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_VIEWS_CONTROLS_MENU_MENU_EVENT_FILTER_H_
+#define UI_VIEWS_CONTROLS_MENU_MENU_EVENT_FILTER_H_
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "ui/events/event_handler.h"
+#include "ui/views/views_export.h"
+
+namespace aura {
+class Window;
+}  // namespace aura

[pkotwicz, 2015/08/28 23:44:25]

Nit: We don't add comments for forward declaration namespaces.

[afakhry, 2015/08/29 01:31:24]

Done.

+
+namespace ui {
+class Accelerator;
+}  // namespace ui

[pkotwicz, 2015/08/28 23:44:26]

Nit: remove namespace comment

[afakhry, 2015/08/29 01:31:24]

Done.

+
+namespace views {
+
+class MenuController;
+
+// This will be used as a pre-target-handler whenever a menu is present. It is
+// needed to handle the interactions between accelerators and menus.

[pkotwicz, 2015/08/28 23:44:26]

How about: "Handles key events when a menu is open."

I am assuming that the code in OnTouchEvent() is no longer necessary

+class VIEWS_EXPORT MenuEventFilter : public ui::EventHandler {
+ public:
+  // Defines a delegate interface for the MenuEventFilter that can handle
+  // accelerators on its behalf.
+  class Delegate {
+   public:
+    enum Result {

[pkotwicz, 2015/08/28 23:44:25]

Nit: Remove because it is no longer used

[afakhry, 2015/08/29 01:31:24]

Oops forgot to do that. Done!

+      RESULT_PROCESSED = 0,  // Accelerator was processed while menu is open.
+      RESULT_PROCESS_LATER,  // Accelerator should be processed after menu
+                             // closes.
+    };
+
+    Delegate() {}
+    virtual ~Delegate() {}
+
+    // When a KeyEvent is received, the corresponding |accelerator| must be
+    // stored in the |AcceleratorHistory| so that accelerator processing can
+    // be done properly.

[pkotwicz, 2015/08/28 23:44:25]

How about: "Stores |accelerator| in AcceleratorHistory."

I have not seen the "vertical bar" character used for class names.

[afakhry, 2015/08/29 01:31:24]

Done.

+    virtual void StoreInHistory(const ui::Accelerator& accelerator) = 0;
+
+    // Returns false if the |accelerator| can be performed while the menu is
+    // open, or true if the menu should be closed an the accelerator should be
+    // reposted after the menu exists to be performed later.

[pkotwicz, 2015/08/28 23:44:26]

How about: "Returns true if there is an action associated with |accelerator| and
the menu should be closed before performing the action."

I do not think it is worth mentioning what false means because it can either
mean:
- There is an action associated with the accelerator and it can be performed
while the menu is open
- There is no action associated with the accelerator

[afakhry, 2015/08/29 01:31:24]

Makes sense. Done!

+    virtual bool ShouldCloseMenuAndRepostAccelerator(
+        const ui::Accelerator& accelerator) const = 0;
+
+    // Processes the given |accelerator| immediately regardless of the action
+    // associated with it.

[pkotwicz, 2015/08/28 23:44:26]

How about: "Processes |accelerator| immediately."

[afakhry, 2015/08/29 01:31:24]

Done.

+    virtual void ProcessAcceleratorNow(const ui::Accelerator& accelerator) = 0;
+  };
+
+  // |menu_controller| should be of the active menu in the currently running
+  // menu message loop, it can't be nullptr. If |delegate| is nullptr, the
+  // default delegate will be used.

[pkotwicz, 2015/08/28 23:44:26]

Nit: "active menu in the currently running menu message loop" -> "active
MenuController"

[afakhry, 2015/08/29 01:31:24]

Done.

+  MenuEventFilter(MenuController* menu_controller,
+                  MenuEventFilter::Delegate* delegate);
+  ~MenuEventFilter() override;
+
+  static void SetMenuEventFilterDelegate(aura::Window* root_window,
+                                         MenuEventFilter::Delegate* delegate);
+  static MenuEventFilter::Delegate* GetMenuEventFilterDelegate(
+      aura::Window* root_window);
+
+  // ui::EventHandler:
+  void OnKeyEvent(ui::KeyEvent* event) override;
+  void OnTouchEvent(ui::TouchEvent* event) override;
+
+ private:
+  MenuController* menu_controller_;  // Not owned.
+  scoped_ptr<MenuEventFilter::Delegate> default_delegate_;
+  MenuEventFilter::Delegate* filter_delegate_;  // Not owned.
+
+  DISALLOW_COPY_AND_ASSIGN(MenuEventFilter);
+};
+
+}  // namespace views
+
+
+#endif  // UI_VIEWS_CONTROLS_MENU_MENU_EVENT_FILTER_H_