Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_message_loop_aura.cc

Index: ui/views/controls/menu/menu_message_loop_aura.cc
diff --git a/ui/views/controls/menu/menu_message_loop_aura.cc b/ui/views/controls/menu/menu_message_loop_aura.cc
index 3ed33281aa6f43e8b99a301acb33af9aef39515f..d2788a3ffa38b2fc71755f01e61ff8067d2a9dfd 100644
--- a/ui/views/controls/menu/menu_message_loop_aura.cc
+++ b/ui/views/controls/menu/menu_message_loop_aura.cc
@@ -18,6 +18,7 @@
 #include "ui/events/platform/platform_event_source.h"
 #include "ui/events/platform/scoped_event_dispatcher.h"
 #include "ui/views/controls/menu/menu_controller.h"
+#include "ui/views/controls/menu/menu_event_filter.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/public/activation_change_observer.h"
 #include "ui/wm/public/activation_client.h"
@@ -103,7 +104,7 @@ MenuMessageLoop* MenuMessageLoop::Create() {
   return new MenuMessageLoopAura;
 }
 
-MenuMessageLoopAura::MenuMessageLoopAura() : owner_(NULL) {
+MenuMessageLoopAura::MenuMessageLoopAura() : owner_(nullptr) {
 }
 
 MenuMessageLoopAura::~MenuMessageLoopAura() {
@@ -157,29 +158,28 @@ void MenuMessageLoopAura::Run(MenuController* controller,
     run_loop.Run();
   }
 #else
-  internal::MenuEventDispatcher event_dispatcher(controller);
-  scoped_ptr<ui::ScopedEventDispatcher> dispatcher_override;
-  if (ui::PlatformEventSource::GetInstance()) {
-    dispatcher_override =
-        ui::PlatformEventSource::GetInstance()->OverrideDispatcher(
-            &event_dispatcher);
-  }
+  scoped_ptr<ActivationChangeObserverImpl> observer;
+  MenuEventFilter::Delegate* filter_delegate = nullptr;
   if (root) {
-    scoped_ptr<ActivationChangeObserverImpl> observer;
     if (!nested_menu)
       observer.reset(new ActivationChangeObserverImpl(controller, root));
-    aura::client::DispatcherRunLoop run_loop(
-        aura::client::GetDispatcherClient(root), NULL);
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
-  } else {
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    base::RunLoop run_loop;
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
+    filter_delegate = MenuEventFilter::GetMenuEventFilterDelegate(root);
   }
-#endif
+
+  MenuEventFilter menu_event_filter(controller, filter_delegate);

[sky, 2015/09/03 17:23:21]

AFAICT the functionality you need is the ability to install an EventHandlerwhen
the menu is shown. If this is the case, why do you need all this logic and not a
more straightforward way to do that? Say a function on ViewsDelegate that is
called when the menu is shown allowing you to add an EventHandler?

[afakhry, 2015/09/04 01:49:42]

I'm not sure I understand your suggestion. What's the problem with using what we
already have in aura::Env to add and remove the EventHandler? Currently
ViewsDelegate doesn't have this functionality, which will make this change an
even bigger refactor. Could you please explain a bit more?

[sky, 2015/09/04 14:38:54]

I'm trying to understand why you are doing the change you're doing.

I agree with you adding/removing an EventHandler is the way to go. What I don't
understand is why we need MenuEventFilter in views side of code. I'm suggesting
the logic of MenuEventFilter be moved into chrome/ash by way of ViewsDelegate.
ViewsDelegate gets a method that is called at the right time to allow you to
install the EventHandler you need to. Does that make sense?

[oshima, 2015/09/04 18:27:01]

My understanding of the role of ViewsDelegate is to customize views behavior for
views client such as ash/chrome, right?
I think the issue is that current MenuEventFilter has a mix of platform and
client behavior. How about
removing the accelerator handling part from MenuEventFilter and move it to the
implementation of MenuEventFilter::Delegate, then
let ViewsDelegate return the MenuEventFilter::Delegate instead of
MenuEventFilter::GetMenuEventFilterDelegate. What do you think?

I also think adding MenuEventFilter for every message loop is overkill though.
Can't this be moved to MenuController so that it gets created only once when
menu is open?

[oshima, 2015/09/04 18:32:10]

Or just have ViewsDelegate::HandleKeyEventOnMenu

(we probably don't need separate Delegate instance)

[afakhry, 2015/09/08 21:31:46]

Done.

+  aura::Env::GetInstanceDontCreate()->PrependPreTargetHandler(
+      &menu_event_filter);
+
+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+  base::RunLoop run_loop;
+  message_loop_quit_ = run_loop.QuitClosure();
+
+  run_loop.Run();
+
+  aura::Env::GetInstanceDontCreate()->RemovePreTargetHandler(
+      &menu_event_filter);
+#endif  // defined(OS_WIN)
 }
 
 void MenuMessageLoopAura::QuitNow() {