Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255,566019

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

Cr-Commit-Position: refs/heads/master@{#371894}

chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc

@@ -53,6 +53,10 @@
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/widget/widget.h"
 
+#if defined(OS_WIN)
+#include "ui/aura/window_tree_host.h"
+#endif
+
 using base::ASCIIToUTF16;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
@@ -2243,3 +2247,90 @@ class BookmarkBarViewTest24 : public BookmarkBarViewEventTestBase {
 #endif
 VIEW_TEST(BookmarkBarViewTest24, MAYBE_ContextMenusKeyboardEscape)
 
+#if defined(OS_WIN)
+// Tests that pressing the key KEYCODE closes the menu.
+template <ui::KeyboardCode KEYCODE>
+class BookmarkBarViewTest25 : public BookmarkBarViewEventTestBase {
+ protected:
+  void DoTestOnMessageLoop() override {
+    // Move the mouse to the first folder on the bookmark bar and press the
+    // mouse.
+    views::LabelButton* button = GetBookmarkButton(0);
+    ui_test_utils::MoveMouseToCenterAndPress(
+        button, ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest25::Step2));
+    base::MessageLoop::current()->RunUntilIdle();
+  }
+
+ private:
+  void Step2() {
+    // Menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != nullptr);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    // Send KEYCODE key event, which should close the menu.
+    ASSERT_TRUE(ui_controls::SendKeyPressNotifyWhenDone(
+        window_->GetNativeWindow(), KEYCODE, false, false, false, false,
+        CreateEventTask(this, &BookmarkBarViewTest25::Step3)));
+  }
+
+  void Step3() {
+    // Make sure menu is not showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu == nullptr);
+
+    Done();
+  }
+};
+
+// Tests that pressing F10 system key closes the menu.
+using BookmarkBarViewTest25F10 = BookmarkBarViewTest25<ui::VKEY_F10>;
+VIEW_TEST(BookmarkBarViewTest25F10, F10ClosesMenu);
+
+// Tests that pressing Alt system key closes the menu.
+using BookmarkBarViewTest25Alt = BookmarkBarViewTest25<ui::VKEY_MENU>;
+VIEW_TEST(BookmarkBarViewTest25Alt, AltClosesMenu);
+
+// Tests that WM_CANCELMODE closes the menu.
+class BookmarkBarViewTest26 : public BookmarkBarViewEventTestBase {
+ protected:
+  void DoTestOnMessageLoop() override {
+    // Move the mouse to the first folder on the bookmark bar and press the
+    // mouse.
+    views::LabelButton* button = GetBookmarkButton(0);
+    ui_test_utils::MoveMouseToCenterAndPress(
+        button, ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest26::Step2));
+    base::MessageLoop::current()->RunUntilIdle();
+  }
+
+ private:
+  void Step2() {
+    // Menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != nullptr);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    // Send WM_CANCELMODE, which should close the menu. The message is sent
+    // synchronously, however, we post a task to make sure that the message is
+    // processed completely before finishing the test.
+    ::SendMessage(
+        GetWidget()->GetNativeView()->GetHost()->GetAcceleratedWidget(),
+        WM_CANCELMODE, 0, 0);
+
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(&BookmarkBarViewTest26::Step3, this));
+  }
+
+  void Step3() {
+    // Menu should not be showing anymore.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu == nullptr);
+
+    Done();
+  }
+};
+
+VIEW_TEST(BookmarkBarViewTest26, CancelModeClosesMenu);
+#endif

ui/views/controls/menu/menu_controller.cc

@@ -1149,6 +1149,15 @@ void MenuController::OnKeyDown(ui::KeyboardCode key_code) {
       break;
     }
 
+#if defined(OS_WIN)
+    // On Windows, pressing Alt and F10 keys should hide the menu to match the
+    // OS behavior.
+    case ui::VKEY_MENU:
+    case ui::VKEY_F10:
+      Cancel(EXIT_ALL);
+      break;
+#endif
+
     default:
       break;
   }

ui/views/controls/menu/menu_controller.h

@@ -49,7 +49,6 @@ class View;
 namespace internal {
 class MenuControllerDelegate;
 class MenuEventDispatcher;
-class MenuMessagePumpDispatcher;
 class MenuRunnerImpl;
 }
 
@@ -200,7 +199,6 @@ class VIEWS_EXPORT MenuController : public WidgetObserver {
 
  private:
   friend class internal::MenuEventDispatcher;
-  friend class internal::MenuMessagePumpDispatcher;
   friend class internal::MenuRunnerImpl;
   friend class test::MenuControllerTest;
   friend class MenuKeyEventHandler;

ui/views/controls/menu/menu_key_event_handler.cc

@@ -5,7 +5,6 @@
 #include "ui/views/controls/menu/menu_key_event_handler.h"
 
 #include "ui/aura/env.h"
-#include "ui/events/keycodes/keyboard_code_conversion.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/views_delegate.h"
 
@@ -53,7 +52,7 @@ void MenuKeyEventHandler::OnKeyEvent(ui::KeyEvent* event) {
     const int flags = event->flags();
     if (menu_controller->exit_type() == MenuController::EXIT_NONE &&
         (flags & kKeyFlagsMask) == 0) {
-      char c = ui::DomCodeToUsLayoutCharacter(event->code(), flags);
+      char c = event->GetCharacter();
       menu_controller->SelectByChar(c);
     }
   }

ui/views/controls/menu/menu_message_loop_aura.cc

@@ -15,19 +15,13 @@
 #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_key_event_handler.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/public/activation_change_observer.h"
 #include "ui/wm/public/activation_client.h"
 #include "ui/wm/public/dispatcher_client.h"
 #include "ui/wm/public/drag_drop_client.h"
 
-#if defined(OS_WIN)
-#include "ui/base/win/internal_constants.h"
-#include "ui/views/controls/menu/menu_message_pump_dispatcher_win.h"
-#include "ui/views/win/hwnd_util.h"
-#else
-#include "ui/views/controls/menu/menu_key_event_handler.h"
-#endif
 
 using aura::client::ScreenPositionClient;
 
@@ -139,24 +133,6 @@ void MenuMessageLoopAura::Run(MenuController* controller,
   base::AutoReset<base::Closure> reset_quit_closure(&message_loop_quit_,
                                                     base::Closure());
 
-#if defined(OS_WIN)
-  internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
-  if (root) {
-    scoped_ptr<ActivationChangeObserverImpl> observer;
-    if (!nested_menu)
-      observer.reset(new ActivationChangeObserverImpl(controller, root));
-    aura::client::DispatcherRunLoop run_loop(
-        aura::client::GetDispatcherClient(root), &nested_dispatcher);
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
-  } else {
-    base::MessageLoop* loop = base::MessageLoop::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    base::RunLoop run_loop(&nested_dispatcher);
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
-  }
-#else
   scoped_ptr<ActivationChangeObserverImpl> observer;
   if (root) {
     if (!nested_menu)
@@ -176,7 +152,6 @@ void MenuMessageLoopAura::Run(MenuController* controller,
   message_loop_quit_ = run_loop.QuitClosure();
 
   run_loop.Run();
-#endif  // defined(OS_WIN)
 }
 
 void MenuMessageLoopAura::QuitNow() {

ui/views/controls/menu/menu_message_pump_dispatcher_win.cc

-// Copyright 2014 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.
-
-#include "ui/views/controls/menu/menu_message_pump_dispatcher_win.h"
-
-#include <windowsx.h>
-
-#include "ui/events/event_utils.h"
-#include "ui/events/keycodes/keyboard_code_conversion.h"
-#include "ui/events/keycodes/keyboard_codes.h"
-#include "ui/views/controls/menu/menu_controller.h"
-#include "ui/views/controls/menu/menu_item_view.h"
-
-namespace views {
-namespace internal {
-
-MenuMessagePumpDispatcher::MenuMessagePumpDispatcher(MenuController* controller)
-    : menu_controller_(controller) {}
-
-MenuMessagePumpDispatcher::~MenuMessagePumpDispatcher() {}
-
-uint32_t MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
-  DCHECK(menu_controller_->IsBlockingRun());
-
-  bool should_perform_default = true;
-  if (menu_controller_->exit_type() != MenuController::EXIT_ALL &&
-      menu_controller_->exit_type() != MenuController::EXIT_DESTROYED) {
-    // NOTE: we don't get WM_ACTIVATE or anything else interesting in here.
-    switch (msg.message) {
-      // NOTE: focus wasn't changed when the menu was shown. As such, don't
-      // dispatch key events otherwise the focused window will get the events.
-      case WM_KEYDOWN: {
-        menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(msg));
-        TranslateMessage(&msg);
-        should_perform_default = false;
-        break;
-      }
-      case WM_CHAR: {
-        menu_controller_->SelectByChar(static_cast<base::char16>(msg.wParam));
-        should_perform_default = false;
-        break;
-      }
-      case WM_KEYUP:
-      case WM_SYSKEYUP:
-        // We may have been shown on a system key, as such don't do anything
-        // here. If another system key is pushed we'll get a WM_SYSKEYDOWN and
-        // close the menu.
-        should_perform_default = false;
-        break;
-
-      case WM_CANCELMODE:
-      case WM_SYSKEYDOWN:
-        // Exit immediately on system keys.
-        menu_controller_->Cancel(MenuController::EXIT_ALL);
-        should_perform_default = false;
-        break;
-
-      default:
-        break;
-    }
-  }
-
-  menu_controller_->TerminateNestedMessageLoopIfNecessary();
-  return should_perform_default ? POST_DISPATCH_PERFORM_DEFAULT
-                                : POST_DISPATCH_NONE;
-}
-
-}  // namespace internal
-}  // namespace views

ui/views/controls/menu/menu_message_pump_dispatcher_win.h

-// Copyright 2014 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_MESSAGE_PUMP_DISPATCHER_WIN_H_
-#define UI_VIEWS_CONTROLS_MENU_MENU_MESSAGE_PUMP_DISPATCHER_WIN_H_
-
-#include <stdint.h>
-
-#include "base/macros.h"
-#include "base/message_loop/message_pump_dispatcher.h"
-
-namespace views {
-
-class MenuController;
-
-namespace internal {
-
-// A message-pump dispatcher object used to dispatch events from the nested
-// message-loop initiated by the MenuController.
-class MenuMessagePumpDispatcher : public base::MessagePumpDispatcher {
- public:
-  explicit MenuMessagePumpDispatcher(MenuController* menu_controller);
-  ~MenuMessagePumpDispatcher() override;
-
- private:
-  // base::MessagePumpDispatcher:
-  uint32_t Dispatch(const base::NativeEvent& event) override;
-
-  MenuController* menu_controller_;
-
-  DISALLOW_COPY_AND_ASSIGN(MenuMessagePumpDispatcher);
-};
-
-}  // namespace internal
-}  // namespace views
-
-#endif  // UI_VIEWS_CONTROLS_MENU_MENU_MESSAGE_PUMP_DISPATCHER_WIN_H_

ui/views/views.gyp

@@ -128,8 +128,6 @@
       'controls/menu/menu_message_loop.h',
       'controls/menu/menu_message_loop_mac.cc',
       'controls/menu/menu_message_loop_mac.h',
-      'controls/menu/menu_message_pump_dispatcher_win.cc',
-      'controls/menu/menu_message_pump_dispatcher_win.h',
       'controls/menu/menu_model_adapter.cc',
       'controls/menu/menu_model_adapter.h',
       'controls/menu/menu_runner.cc',