[Global Media Controls] Split KeyboardHook into modifier/media impls

This CL creates a new type of KeyboardHook to listen to media keys.
KeyboardHook::Create is split in two, providing access to each type of
KeyboardHook. This creates the media hook implementation for Windows,
and returns nullptr for other platforms.

Bug: 903582
Change-Id: I5eb6bbd94ec23dd54867eeb9ced585bc1b726961
Reviewed-on: https://chromium-review.googlesource.com/c/1333118
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611500}

chrome/test/BUILD.gn

@@ -4826,6 +4826,7 @@ if (!is_android) {
       "//third_party/libpng",
       "//third_party/zlib",
       "//ui/base:test_support",
+      "//ui/events:events_interactive_ui_tests",
       "//ui/resources:ui_test_pak",
       "//ui/web_dialogs:test_support",
     ]

ui/aura/window_tree_host_platform.cc

@@ -145,7 +145,7 @@ bool WindowTreeHostPlatform::CaptureSystemKeyEventsImpl(
   // problems with event routing (i.e. which Hook takes precedence) and
   // destruction ordering.
   DCHECK(!keyboard_hook_);
-  keyboard_hook_ = ui::KeyboardHook::Create(
+  keyboard_hook_ = ui::KeyboardHook::CreateModifierKeyboardHook(
       std::move(dom_codes), GetAcceleratedWidget(),
       base::BindRepeating(
           [](ui::PlatformWindowDelegate* delegate, ui::KeyEvent* event) {

ui/events/BUILD.gn

@@ -211,8 +211,10 @@ jumbo_component("events") {
     "system_input_injector.cc",
     "win/events_win.cc",
     "win/events_win_utils.cc",
-    "win/keyboard_hook_win.cc",
-    "win/keyboard_hook_win.h",
+    "win/keyboard_hook_win_base.cc",
+    "win/keyboard_hook_win_base.h",
+    "win/media_keyboard_hook_win.cc",
+    "win/modifier_keyboard_hook_win.cc",
     "win/system_event_state_lookup.cc",
   ]
 
@@ -509,7 +511,8 @@ if (!is_ios) {
       "platform/platform_event_source_unittest.cc",
       "scoped_target_handler_unittest.cc",
       "win/event_utils_win_unittest.cc",
-      "win/keyboard_hook_win_unittest.cc",
+      "win/media_keyboard_hook_win_unittest.cc",
+      "win/modifier_keyboard_hook_win_unittest.cc",
     ]
 
     deps = [
@@ -631,3 +634,21 @@ if (is_android) {
     classes = [ "android/view/KeyEvent.class" ]
   }
 }
+
+# This target is added as a dependency of browser interactive_ui_tests. It must
+# be source_set, otherwise the linker will drop the tests as dead code.
+source_set("events_interactive_ui_tests") {
+  testonly = true
+  if (is_win) {
+    sources = [
+      "win/media_keyboard_hook_win_interactive_test.cc",
+    ]
+
+    deps = [
+      ":events",
+      ":test_support",
+      "//base/test:test_support",
+      "//testing/gtest",
+    ]
+  }
+}

ui/events/android/keyboard_hook_android.cc

@@ -14,11 +14,17 @@
 namespace ui {
 
 // static
-std::unique_ptr<KeyboardHook> KeyboardHook::Create(
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     gfx::AcceleratedWidget accelerated_widget,
     KeyboardHook::KeyEventCallback callback) {
   return nullptr;
 }
 
+// static
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateMediaKeyboardHook(
+    KeyboardHook::KeyEventCallback callback) {
+  return nullptr;
+}
+
 }  // namespace ui

ui/events/keyboard_hook.h

@@ -32,11 +32,19 @@ class EVENTS_EXPORT KeyboardHook {
   // |callback| is called for each key which is intercepted.
   // Returns a valid instance if the hook was created and successfully
   // registered otherwise nullptr.
-  static std::unique_ptr<KeyboardHook> Create(
+  static std::unique_ptr<KeyboardHook> CreateModifierKeyboardHook(
       base::Optional<base::flat_set<DomCode>> dom_codes,
       gfx::AcceleratedWidget accelerated_widget,
       KeyEventCallback callback);
 
+  // Creates a platform-specific KeyboardHook implementation that captures the
+  // play/pause, stop, and next/previous track media keys.
+  // |callback| is called for each key which is intercepted.
+  // Returns a valid instance if the hook was created and successfully
+  // registered otherwise nullptr.
+  static std::unique_ptr<KeyboardHook> CreateMediaKeyboardHook(
+      KeyEventCallback callback);
+
   // True if |dom_code| is reserved for an active KeyboardLock request.
   virtual bool IsKeyLocked(DomCode dom_code) const = 0;
 };

ui/events/keyboard_hook_base.cc

@@ -34,9 +34,8 @@ bool KeyboardHookBase::ShouldCaptureKeyEvent(DomCode dom_code) const {
   return !dom_codes_ || base::ContainsKey(dom_codes_.value(), dom_code);
 }
 
-void KeyboardHookBase::ForwardCapturedKeyEvent(
-    std::unique_ptr<KeyEvent> event) {
-  key_event_callback_.Run(event.get());
+void KeyboardHookBase::ForwardCapturedKeyEvent(KeyEvent* event) {
+  key_event_callback_.Run(event);
 }
 
 }  // namespace ui

ui/events/keyboard_hook_base.h

@@ -29,7 +29,9 @@ class KeyboardHookBase : public KeyboardHook {
   bool ShouldCaptureKeyEvent(DomCode dom_code) const;
 
   // Forwards the key event using |key_event_callback_|.
-  void ForwardCapturedKeyEvent(std::unique_ptr<KeyEvent> event);
+  // |event| is owned by the calling method and will live until this method
+  // returns.
+  void ForwardCapturedKeyEvent(KeyEvent* event);
 
   const base::Optional<base::flat_set<DomCode>>& dom_codes() {
     return dom_codes_;

ui/events/mac/keyboard_hook_mac.mm

@@ -9,11 +9,17 @@
 namespace ui {
 
 // static
-std::unique_ptr<KeyboardHook> KeyboardHook::Create(
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     gfx::AcceleratedWidget accelerated_widget,
     KeyEventCallback callback) {
   return nullptr;
 }
 
+// static
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateMediaKeyboardHook(
+    KeyEventCallback callback) {
+  return nullptr;
+}
+
 }  // namespace ui

ui/events/ozone/keyboard_hook_ozone.cc

@@ -46,7 +46,7 @@ bool KeyboardHookOzone::Register() {
 }  // namespace
 
 // static
-std::unique_ptr<KeyboardHook> KeyboardHook::Create(
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     gfx::AcceleratedWidget accelerated_widget,
     KeyEventCallback callback) {
@@ -60,4 +60,10 @@ std::unique_ptr<KeyboardHook> KeyboardHook::Create(
   return keyboard_hook;
 }
 
+// static
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateMediaKeyboardHook(
+    KeyEventCallback callback) {
+  return nullptr;
+}
+
 }  // namespace ui

ui/events/win/keyboard_hook_win_base.cc

+// Copyright 2018 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/events/win/keyboard_hook_win_base.h"
+
+namespace ui {
+
+KeyboardHookWinBase::KeyboardHookWinBase(
+    base::Optional<base::flat_set<DomCode>> dom_codes,
+    KeyEventCallback callback,
+    bool enable_hook_registration)
+    : KeyboardHookBase(std::move(dom_codes), std::move(callback)),
+      enable_hook_registration_(enable_hook_registration) {}
+
+KeyboardHookWinBase::~KeyboardHookWinBase() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  if (!enable_hook_registration_)
+    return;
+
+  if (!UnhookWindowsHookEx(hook_))
+    DPLOG(ERROR) << "UnhookWindowsHookEx failed";
+}
+
+bool KeyboardHookWinBase::Register(HOOKPROC hook_proc) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  // If the hook was created for testing, |Register()| should not be called.
+  DCHECK(enable_hook_registration_);
+
+  DCHECK(!hook_);
+
+  // Don't register hooks when there is a debugger to avoid painful user input
+  // delays.
+  if (IsDebuggerPresent())
+    return false;
+
+  // Per MSDN this Hook procedure will be called in the context of the thread
+  // which installed it.
+  hook_ = SetWindowsHookEx(WH_KEYBOARD_LL, hook_proc,
+                           /*hMod=*/nullptr,
+                           /*dwThreadId=*/0);
+  DPLOG_IF(ERROR, !hook_) << "SetWindowsHookEx failed";
+
+  return hook_ != nullptr;
+}
+
+// static
+LRESULT CALLBACK
+KeyboardHookWinBase::ProcessKeyEvent(KeyboardHookWinBase* instance,
+                                     int code,
+                                     WPARAM w_param,
+                                     LPARAM l_param) {
+  // If there is an error unhooking, this method could be called with a null
+  // |instance_|.  Ensure we have a valid instance and that |code| is correct
+  // before proceeding.
+  if (!instance || code != HC_ACTION)
+    return CallNextHookEx(nullptr, code, w_param, l_param);
+
+  DCHECK_CALLED_ON_VALID_THREAD(instance->thread_checker_);
+
+  KBDLLHOOKSTRUCT* ll_hooks = reinterpret_cast<KBDLLHOOKSTRUCT*>(l_param);
+
+  // This vkey represents both a vkey and a location on the keyboard such as
+  // VK_LCONTROL or VK_RCONTROL.
+  DWORD vk = ll_hooks->vkCode;
+
+  // Apply the extended flag prior to passing |scan_code| since |instance_| does
+  // not have access to the low-level hook flags.
+  DWORD scan_code = ll_hooks->scanCode;
+  if (ll_hooks->flags & LLKHF_EXTENDED)
+    scan_code |= 0xE000;
+
+  if (instance->ProcessKeyEventMessage(w_param, vk, scan_code, ll_hooks->time))
+    return 1;
+
+  return CallNextHookEx(nullptr, code, w_param, l_param);
+}
+
+}  // namespace ui

ui/events/win/keyboard_hook_win.h → ui/events/win/keyboard_hook_win_base.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_H_
-#define UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_H_
+#ifndef UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_BASE_H_
+#define UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_BASE_H_
 
 #include <memory>
 
@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/threading/thread_checker.h"
 #include "ui/events/event.h"
 #include "ui/events/events_export.h"
 #include "ui/events/keyboard_hook_base.h"
@@ -24,17 +25,25 @@ namespace ui {
 // it key event data.  This method is used by both the low-level keyboard hook
 // and by unit tests which simulate the hooked behavior w/o actually installing
 // a hook (doing so would cause problems with test parallelization).
-class EVENTS_EXPORT KeyboardHookWin : public KeyboardHookBase {
+class EVENTS_EXPORT KeyboardHookWinBase : public KeyboardHookBase {
  public:
-  KeyboardHookWin(base::Optional<base::flat_set<DomCode>> dom_codes,
-                  KeyEventCallback callback);
-  ~KeyboardHookWin() override;
+  KeyboardHookWinBase(base::Optional<base::flat_set<DomCode>> dom_codes,
+                      KeyEventCallback callback,
+                      bool enable_hook_registration);
+  ~KeyboardHookWinBase() override;
 
-  // Create a KeyboardHookWin instance which does not register a low-level hook.
-  static std::unique_ptr<KeyboardHookWin> CreateForTesting(
+  // Create a KeyboardHookWinBase instance which does not register a
+  // low-level hook and captures modifier keys.
+  static std::unique_ptr<KeyboardHookWinBase>
+  CreateModifierKeyboardHookForTesting(
       base::Optional<base::flat_set<DomCode>> dom_codes,
       KeyEventCallback callback);
 
+  // Create a KeyboardHookWinBase instance which does not register a
+  // low-level hook and captures media keys.
+  static std::unique_ptr<KeyboardHookWinBase> CreateMediaKeyboardHookForTesting(
+      KeyEventCallback callback);
+
   // Called when a key event message is delivered via the low-level hook.
   // Exposed here to allow for testing w/o engaging the low-level hook.
   // Returns true if the message was handled.
@@ -43,10 +52,23 @@ class EVENTS_EXPORT KeyboardHookWin : public KeyboardHookBase {
                                       DWORD scan_code,
                                       DWORD time_stamp) = 0;
 
+ protected:
+  bool Register(HOOKPROC hook_proc);
+  bool enable_hook_registration() const { return enable_hook_registration_; }
+
+  static LRESULT CALLBACK ProcessKeyEvent(KeyboardHookWinBase* instance,
+                                          int code,
+                                          WPARAM w_param,
+                                          LPARAM l_param);
+
  private:
-  DISALLOW_COPY_AND_ASSIGN(KeyboardHookWin);
+  const bool enable_hook_registration_ = true;
+  HHOOK hook_ = nullptr;
+  THREAD_CHECKER(thread_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(KeyboardHookWinBase);
 };
 
 }  // namespace ui
 
-#endif  // UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_H_
+#endif  // UI_EVENTS_WIN_KEYBOARD_HOOK_WIN_BASE_H_

ui/events/win/media_keyboard_hook_win.cc

+// Copyright 2018 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/events/win/keyboard_hook_win_base.h"
+
+#include "ui/events/event.h"
+#include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/events/win/events_win_utils.h"
+
+namespace ui {
+
+namespace {
+
+bool IsMediaKey(DWORD vk) {
+  return vk == VK_MEDIA_NEXT_TRACK || vk == VK_MEDIA_PREV_TRACK ||
+         vk == VK_MEDIA_PLAY_PAUSE || vk == VK_MEDIA_STOP;
+}
+
+class MediaKeyboardHookWinImpl : public KeyboardHookWinBase {
+ public:
+  MediaKeyboardHookWinImpl(KeyEventCallback callback,
+                           bool enable_hook_registration);
+  ~MediaKeyboardHookWinImpl() override;
+
+  // KeyboardHookWinBase implementation.
+  bool ProcessKeyEventMessage(WPARAM w_param,
+                              DWORD vk,
+                              DWORD scan_code,
+                              DWORD time_stamp) override;
+
+  bool Register();
+
+ private:
+  static LRESULT CALLBACK ProcessKeyEvent(int code,
+                                          WPARAM w_param,
+                                          LPARAM l_param);
+
+  static MediaKeyboardHookWinImpl* instance_;
+
+  // Tracks the last non-located key down seen in order to determine if the
+  // current key event should be marked as a repeated key press.
+  DWORD last_key_down_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaKeyboardHookWinImpl);
+};
+
+// static
+MediaKeyboardHookWinImpl* MediaKeyboardHookWinImpl::instance_ = nullptr;
+
+MediaKeyboardHookWinImpl::MediaKeyboardHookWinImpl(
+    KeyEventCallback callback,
+    bool enable_hook_registration)
+    : KeyboardHookWinBase(
+          base::Optional<base::flat_set<DomCode>>(
+              {DomCode::MEDIA_PLAY_PAUSE, DomCode::MEDIA_STOP,
+               DomCode::MEDIA_TRACK_NEXT, DomCode::MEDIA_TRACK_PREVIOUS}),
+          std::move(callback),
+          enable_hook_registration) {}
+
+MediaKeyboardHookWinImpl::~MediaKeyboardHookWinImpl() {
+  if (!enable_hook_registration())
+    return;
+
+  DCHECK_EQ(instance_, this);
+  instance_ = nullptr;
+}
+
+bool MediaKeyboardHookWinImpl::Register() {
+  // Only one instance of this class can be registered at a time.
+  DCHECK(!instance_);
+  instance_ = this;
+
+  return KeyboardHookWinBase::Register(
+      reinterpret_cast<HOOKPROC>(&MediaKeyboardHookWinImpl::ProcessKeyEvent));
+}
+
+// static
+LRESULT CALLBACK MediaKeyboardHookWinImpl::ProcessKeyEvent(int code,
+                                                           WPARAM w_param,
+                                                           LPARAM l_param) {
+  return KeyboardHookWinBase::ProcessKeyEvent(instance_, code, w_param,
+                                              l_param);
+}
+
+bool MediaKeyboardHookWinImpl::ProcessKeyEventMessage(WPARAM w_param,
+                                                      DWORD vk,
+                                                      DWORD scan_code,
+                                                      DWORD time_stamp) {
+  if (!IsMediaKey(vk))
+    return false;
+
+  bool is_repeat = false;
+  MSG msg = {nullptr, w_param, vk, GetLParamFromScanCode(scan_code),
+             time_stamp};
+  EventType event_type = EventTypeFromMSG(msg);
+  if (event_type == ET_KEY_PRESSED) {
+    is_repeat = (last_key_down_ == vk);
+    last_key_down_ = vk;
+  } else {
+    DCHECK_EQ(event_type, ET_KEY_RELEASED);
+    last_key_down_ = 0;
+  }
+
+  std::unique_ptr<KeyEvent> key_event =
+      std::make_unique<KeyEvent>(KeyEventFromMSG(msg));
+  if (is_repeat)
+    key_event->set_flags(key_event->flags() | EF_IS_REPEAT);
+  ForwardCapturedKeyEvent(key_event.get());
+
+  // If the event is handled, don't propagate to the OS.
+  return key_event->handled();
+}
+
+}  // namespace
+
+// static
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateMediaKeyboardHook(
+    KeyEventCallback callback) {
+  std::unique_ptr<MediaKeyboardHookWinImpl> keyboard_hook =
+      std::make_unique<MediaKeyboardHookWinImpl>(
+          std::move(callback),
+          /*enable_hook_registration=*/true);
+
+  if (!keyboard_hook->Register())
+    return nullptr;
+
+  return keyboard_hook;
+}
+
+std::unique_ptr<KeyboardHookWinBase>
+KeyboardHookWinBase::CreateMediaKeyboardHookForTesting(
+    KeyEventCallback callback) {
+  return std::make_unique<MediaKeyboardHookWinImpl>(
+      std::move(callback),
+      /*enable_hook_registration=*/false);
+}
+
+}  // namespace ui

ui/events/win/media_keyboard_hook_win_interactive_test.cc

+// Copyright 2018 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/events/keyboard_hook.h"
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/event.h"
+
+namespace ui {
+
+class MediaKeyboardHookWinInteractiveTest : public testing::Test {
+ public:
+  MediaKeyboardHookWinInteractiveTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI) {}
+
+ protected:
+  void SetUp() override {
+    keyboard_hook_ = KeyboardHook::CreateMediaKeyboardHook(base::BindRepeating(
+        &MediaKeyboardHookWinInteractiveTest::HandleKeyEvent,
+        base::Unretained(this)));
+    ASSERT_NE(nullptr, keyboard_hook_);
+  }
+
+  // Loop until we've received |num_events| key events from the hook.
+  void WaitForKeyEvents(uint32_t num_events) {
+    if (key_events_.size() >= num_events)
+      return;
+
+    num_key_events_to_wait_for_ = num_events;
+    key_event_wait_loop_.Run();
+  }
+
+  void SendKeyDown(KeyboardCode code) {
+    INPUT input;
+    input.type = INPUT_KEYBOARD;
+    input.ki.wVk = code;
+    input.ki.time = time_stamp_++;
+    input.ki.dwFlags = 0;
+    SendInput(1, &input, sizeof(INPUT));
+  }
+
+  void SendKeyUp(KeyboardCode code) {
+    INPUT input;
+    input.type = INPUT_KEYBOARD;
+    input.ki.wVk = code;
+    input.ki.time = time_stamp_++;
+    input.ki.dwFlags = KEYEVENTF_KEYUP;
+    SendInput(1, &input, sizeof(INPUT));
+  }
+
+  // Expect that we have received the correct number of key events.
+  void ExpectReceivedEventsCount(uint32_t count) {
+    EXPECT_EQ(count, key_events_.size());
+  }
+
+  // Expect that the key event received at |index| has the specified key code
+  // and type.
+  void ExpectReceivedEvent(uint32_t index, KeyboardCode code, EventType type) {
+    ASSERT_LT(index, key_events_.size());
+    KeyEvent* key_event = &key_events_.at(index);
+    EXPECT_EQ(code, key_event->key_code());
+    EXPECT_EQ(type, key_event->type());
+  }
+
+ private:
+  void HandleKeyEvent(KeyEvent* key_event) {
+    key_events_.push_back(*key_event);
+    key_event->SetHandled();
+
+    // If we've received the events we're waiting for, stop waiting.
+    if (key_event_wait_loop_.running() &&
+        key_events_.size() >= num_key_events_to_wait_for_) {
+      key_event_wait_loop_.Quit();
+    }
+  }
+
+  std::vector<KeyEvent> key_events_;
+  std::unique_ptr<KeyboardHook> keyboard_hook_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  base::RunLoop key_event_wait_loop_;
+  uint32_t num_key_events_to_wait_for_ = 0;
+  DWORD time_stamp_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaKeyboardHookWinInteractiveTest);
+};
+
+// Test that we catch the different media key events.
+TEST_F(MediaKeyboardHookWinInteractiveTest, AllMediaKeysAreCaught) {
+  SendKeyDown(ui::VKEY_MEDIA_PLAY_PAUSE);
+  SendKeyUp(ui::VKEY_MEDIA_PLAY_PAUSE);
+  SendKeyDown(ui::VKEY_MEDIA_STOP);
+  SendKeyUp(ui::VKEY_MEDIA_STOP);
+  SendKeyDown(ui::VKEY_MEDIA_NEXT_TRACK);
+  SendKeyUp(ui::VKEY_MEDIA_NEXT_TRACK);
+  SendKeyDown(ui::VKEY_MEDIA_PREV_TRACK);
+  SendKeyUp(ui::VKEY_MEDIA_PREV_TRACK);
+
+  // We should receive 8 different key events.
+  WaitForKeyEvents(8);
+}
+
+// Test that the received events have the proper state.
+TEST_F(MediaKeyboardHookWinInteractiveTest, CallbackReceivesProperEvents) {
+  // Send a key down event and validate it when received through the hook.
+  SendKeyDown(ui::VKEY_MEDIA_PLAY_PAUSE);
+  WaitForKeyEvents(1);
+  ExpectReceivedEvent(/*index=*/0, ui::VKEY_MEDIA_PLAY_PAUSE, ET_KEY_PRESSED);
+
+  // Send a key up event and validate it when received through the hook.
+  SendKeyUp(ui::VKEY_MEDIA_PLAY_PAUSE);
+  WaitForKeyEvents(2);
+  ExpectReceivedEvent(/*index=*/1, ui::VKEY_MEDIA_PLAY_PAUSE, ET_KEY_RELEASED);
+}
+
+}  // namespace ui
\ No newline at end of file

ui/events/win/media_keyboard_hook_win_unittest.cc

+// Copyright 2018 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 <memory>
+#include <vector>
+
+#include "base/bind.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/event.h"
+#include "ui/events/keyboard_hook.h"
+#include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/events/keycodes/dom/keycode_converter.h"
+#include "ui/events/keycodes/keyboard_codes.h"
+#include "ui/events/win/keyboard_hook_win_base.h"
+
+namespace ui {
+
+class MediaKeyboardHookWinTest : public testing::Test {
+ public:
+  MediaKeyboardHookWinTest();
+  ~MediaKeyboardHookWinTest() override;
+
+  // testing::Test overrides.
+  void SetUp() override;
+
+  void HandleKeyPress(KeyEvent* key_event);
+
+ protected:
+  KeyboardHookWinBase* keyboard_hook() { return keyboard_hook_.get(); }
+
+  uint32_t next_time_stamp() { return time_stamp_++; }
+
+  std::vector<KeyEvent>* key_events() { return &key_events_; }
+
+  // Used for sending key events which are handled by the hook.
+  void SendMediaKeyDownEvent(KeyboardCode key_code,
+                             DomCode dom_code,
+                             int repeat_count = 1);
+  void SendMediaKeyUpEvent(KeyboardCode key_code, DomCode dom_code);
+
+  // Set the return value for the HandleKeyPress callback.
+  void StartHandlingKeys() { should_handle_keys_ = true; }
+  void StopHandlingKeys() { should_handle_keys_ = false; }
+
+ private:
+  uint32_t time_stamp_ = 0;
+  std::unique_ptr<KeyboardHookWinBase> keyboard_hook_;
+  std::vector<KeyEvent> key_events_;
+  bool should_handle_keys_ = true;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaKeyboardHookWinTest);
+};
+
+MediaKeyboardHookWinTest::MediaKeyboardHookWinTest() = default;
+
+MediaKeyboardHookWinTest::~MediaKeyboardHookWinTest() = default;
+
+void MediaKeyboardHookWinTest::SetUp() {
+  keyboard_hook_ = KeyboardHookWinBase::CreateMediaKeyboardHookForTesting(
+      base::BindRepeating(&MediaKeyboardHookWinTest::HandleKeyPress,
+                          base::Unretained(this)));
+}
+
+void MediaKeyboardHookWinTest::HandleKeyPress(KeyEvent* key_event) {
+  key_events_.push_back(*key_event);
+  if (should_handle_keys_)
+    key_event->SetHandled();
+}
+
+void MediaKeyboardHookWinTest::SendMediaKeyDownEvent(KeyboardCode key_code,
+                                                     DomCode dom_code,
+                                                     int repeat_count /*=1*/) {
+  ASSERT_GT(repeat_count, 0);
+  // This should only be used when we're handling keys.
+  ASSERT_TRUE(should_handle_keys_);
+
+  for (int i = 0; i < repeat_count; i++) {
+    ASSERT_TRUE(keyboard_hook()->ProcessKeyEventMessage(
+        WM_KEYDOWN, key_code,
+        KeycodeConverter::DomCodeToNativeKeycode(dom_code), next_time_stamp()));
+  }
+}
+
+void MediaKeyboardHookWinTest::SendMediaKeyUpEvent(KeyboardCode key_code,
+                                                   DomCode dom_code) {
+  // This should only be used when we're handling keys.
+  ASSERT_TRUE(should_handle_keys_);
+
+  ASSERT_TRUE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, key_code, KeycodeConverter::DomCodeToNativeKeycode(dom_code),
+      next_time_stamp()));
+}
+
+void VerifyKeyEvent(KeyEvent* key_event,
+                    KeyboardCode non_located_key_code,
+                    DomCode dom_code,
+                    bool key_down,
+                    bool is_repeat) {
+  if (key_down) {
+    ASSERT_EQ(key_event->type(), ET_KEY_PRESSED);
+    ASSERT_EQ(key_event->is_repeat(), is_repeat);
+  } else {
+    ASSERT_EQ(key_event->type(), ET_KEY_RELEASED);
+    ASSERT_FALSE(key_event->is_repeat());
+  }
+  ASSERT_EQ(key_event->key_code(), non_located_key_code);
+  ASSERT_EQ(key_event->code(), dom_code);
+}
+
+TEST_F(MediaKeyboardHookWinTest, SimpleKeypressTest) {
+  const KeyboardCode key_code = KeyboardCode::VKEY_MEDIA_PLAY_PAUSE;
+  const DomCode dom_code = DomCode::MEDIA_PLAY_PAUSE;
+  SendMediaKeyDownEvent(key_code, dom_code);
+  ASSERT_EQ(key_events()->size(), 1u);
+  SendMediaKeyUpEvent(key_code, dom_code);
+  ASSERT_EQ(key_events()->size(), 2u);
+
+  KeyEvent down_event = key_events()->at(0);
+  ASSERT_NO_FATAL_FAILURE(
+      VerifyKeyEvent(&down_event, key_code, dom_code, true, false));
+
+  KeyEvent up_event = key_events()->at(1);
+  ASSERT_NO_FATAL_FAILURE(
+      VerifyKeyEvent(&up_event, key_code, dom_code, false, false));
+}
+
+TEST_F(MediaKeyboardHookWinTest, RepeatingKeypressTest) {
+  const int repeat_count = 10;
+  const KeyboardCode key_code = KeyboardCode::VKEY_MEDIA_PLAY_PAUSE;
+  const DomCode dom_code = DomCode::MEDIA_PLAY_PAUSE;
+  SendMediaKeyDownEvent(key_code, dom_code, repeat_count);
+  ASSERT_EQ(static_cast<int>(key_events()->size()), repeat_count);
+  SendMediaKeyUpEvent(key_code, dom_code);
+  ASSERT_EQ(static_cast<int>(key_events()->size()), repeat_count + 1);
+
+  bool should_repeat = false;
+  for (int i = 0; i < repeat_count; i++) {
+    KeyEvent event = key_events()->at(i);
+    ASSERT_NO_FATAL_FAILURE(
+        VerifyKeyEvent(&event, key_code, dom_code, true, should_repeat));
+    should_repeat = true;
+  }
+
+  KeyEvent up_event = key_events()->at(repeat_count);
+  ASSERT_NO_FATAL_FAILURE(
+      VerifyKeyEvent(&up_event, key_code, dom_code, false, false));
+}
+
+TEST_F(MediaKeyboardHookWinTest, UnhandledKeysArePropagated) {
+  StopHandlingKeys();
+
+  // Ensure media keys are propagated to the OS.
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYDOWN, KeyboardCode::VKEY_MEDIA_STOP,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::MEDIA_STOP),
+      next_time_stamp()));
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, KeyboardCode::VKEY_MEDIA_STOP,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::MEDIA_STOP),
+      next_time_stamp()));
+
+  StartHandlingKeys();
+
+  // Ensure media keys are not propagated to the OS.
+  ASSERT_TRUE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYDOWN, KeyboardCode::VKEY_MEDIA_STOP,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::MEDIA_STOP),
+      next_time_stamp()));
+  ASSERT_TRUE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, KeyboardCode::VKEY_MEDIA_STOP,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::MEDIA_STOP),
+      next_time_stamp()));
+}
+
+TEST_F(MediaKeyboardHookWinTest, NonInterceptedKeysTest) {
+  // Here we try a few keys we do not expect to be intercepted / handled.
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYDOWN, KeyboardCode::VKEY_RSHIFT,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::SHIFT_RIGHT),
+      next_time_stamp()));
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, KeyboardCode::VKEY_RSHIFT,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::SHIFT_RIGHT),
+      next_time_stamp()));
+
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYDOWN, KeyboardCode::VKEY_ESCAPE,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::ESCAPE),
+      next_time_stamp()));
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, KeyboardCode::VKEY_ESCAPE,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::ESCAPE),
+      next_time_stamp()));
+
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYDOWN, KeyboardCode::VKEY_A,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::US_A),
+      next_time_stamp()));
+  ASSERT_FALSE(keyboard_hook()->ProcessKeyEventMessage(
+      WM_KEYUP, KeyboardCode::VKEY_A,
+      KeycodeConverter::DomCodeToNativeKeycode(DomCode::US_A),
+      next_time_stamp()));
+}
+
+}  // namespace ui

ui/events/win/keyboard_hook_win.cc → ui/events/win/modifier_keyboard_hook_win.cc

@@ -2,14 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ui/events/win/keyboard_hook_win.h"
+#include "ui/events/win/keyboard_hook_win_base.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/optional.h"
-#include "base/threading/thread_checker.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/dom_code.h"
@@ -108,14 +107,14 @@ bool IsModifierKey(DWORD vk) {
   return IsAltKey(vk) || IsControlKey(vk) || IsWindowsKey(vk);
 }
 
-class KeyboardHookWinImpl : public KeyboardHookWin {
+class ModifierKeyboardHookWinImpl : public KeyboardHookWinBase {
  public:
-  KeyboardHookWinImpl(base::Optional<base::flat_set<DomCode>> dom_codes,
-                      KeyEventCallback callback,
-                      bool enable_hook_registration);
-  ~KeyboardHookWinImpl() override;
+  ModifierKeyboardHookWinImpl(base::Optional<base::flat_set<DomCode>> dom_codes,
+                              KeyEventCallback callback,
+                              bool enable_hook_registration);
+  ~ModifierKeyboardHookWinImpl() override;
 
-  // KeyboardHookWin implementation.
+  // KeyboardHookWinBase implementation.
   bool ProcessKeyEventMessage(WPARAM w_param,
                               DWORD vk,
                               DWORD scan_code,
@@ -132,11 +131,7 @@ class KeyboardHookWinImpl : public KeyboardHookWin {
 
   void ClearModifierStates();
 
-  static KeyboardHookWinImpl* instance_;
-
-  THREAD_CHECKER(thread_checker_);
-
-  HHOOK hook_ = nullptr;
+  static ModifierKeyboardHookWinImpl* instance_;
 
   // Tracks the last non-located key down seen in order to determine if the
   // current key event should be marked as a repeated key press.
@@ -148,59 +143,40 @@ class KeyboardHookWinImpl : public KeyboardHookWin {
   // This sequence occurs on the initial keypress and every repeat.
   int altgr_sequence_count_ = 0;
 
-  const bool enable_hook_registration_ = true;
-
-  DISALLOW_COPY_AND_ASSIGN(KeyboardHookWinImpl);
+  DISALLOW_COPY_AND_ASSIGN(ModifierKeyboardHookWinImpl);
 };
 
 // static
-KeyboardHookWinImpl* KeyboardHookWinImpl::instance_ = nullptr;
+ModifierKeyboardHookWinImpl* ModifierKeyboardHookWinImpl::instance_ = nullptr;
 
-KeyboardHookWinImpl::KeyboardHookWinImpl(
+ModifierKeyboardHookWinImpl::ModifierKeyboardHookWinImpl(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     KeyEventCallback callback,
     bool enable_hook_registration)
-    : KeyboardHookWin(std::move(dom_codes), std::move(callback)),
-      enable_hook_registration_(enable_hook_registration) {}
-
-KeyboardHookWinImpl::~KeyboardHookWinImpl() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    : KeyboardHookWinBase(std::move(dom_codes),
+                          std::move(callback),
+                          enable_hook_registration) {}
 
+ModifierKeyboardHookWinImpl::~ModifierKeyboardHookWinImpl() {
   ClearModifierStates();
 
-  if (!enable_hook_registration_)
+  if (!enable_hook_registration())
     return;
 
   DCHECK_EQ(instance_, this);
   instance_ = nullptr;
-
-  if (!UnhookWindowsHookEx(hook_))
-    DPLOG(ERROR) << "UnhookWindowsHookEx failed";
 }
 
-bool KeyboardHookWinImpl::Register() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-
-  // If the hook was created for testing, |Register()| should not be called.
-  DCHECK(enable_hook_registration_);
-
+bool ModifierKeyboardHookWinImpl::Register() {
   // Only one instance of this class can be registered at a time.
   DCHECK(!instance_);
   instance_ = this;
 
-  // Per MSDN this Hook procedure will be called in the context of the thread
-  // which installed it.
-  hook_ = SetWindowsHookEx(
-      WH_KEYBOARD_LL,
-      reinterpret_cast<HOOKPROC>(&KeyboardHookWinImpl::ProcessKeyEvent),
-      /*hMod=*/nullptr,
-      /*dwThreadId=*/0);
-  DPLOG_IF(ERROR, !hook_) << "SetWindowsHookEx failed";
-
-  return hook_ != nullptr;
+  return KeyboardHookWinBase::Register(reinterpret_cast<HOOKPROC>(
+      &ModifierKeyboardHookWinImpl::ProcessKeyEvent));
 }
 
-void KeyboardHookWinImpl::ClearModifierStates() {
+void ModifierKeyboardHookWinImpl::ClearModifierStates() {
   BYTE keyboard_state[kKeyboardStateArraySize] = {0};
   if (!GetKeyboardState(keyboard_state)) {
     DPLOG(ERROR) << "GetKeyboardState() failed: ";
@@ -221,10 +197,10 @@ void KeyboardHookWinImpl::ClearModifierStates() {
     DPLOG(ERROR) << "SetKeyboardState() failed: ";
 }
 
-bool KeyboardHookWinImpl::ProcessKeyEventMessage(WPARAM w_param,
-                                                 DWORD vk,
-                                                 DWORD scan_code,
-                                                 DWORD time_stamp) {
+bool ModifierKeyboardHookWinImpl::ProcessKeyEventMessage(WPARAM w_param,
+                                                         DWORD vk,
+                                                         DWORD scan_code,
+                                                         DWORD time_stamp) {
   // The |vk| delivered to the low-level hook includes a location which is
   // needed to track individual keystates such as when both left and right
   // control keys are pressed.  Make sure that location information was retained
@@ -288,12 +264,13 @@ bool KeyboardHookWinImpl::ProcessKeyEventMessage(WPARAM w_param,
       std::make_unique<KeyEvent>(KeyEventFromMSG(msg));
   if (is_repeat)
     key_event->set_flags(key_event->flags() | EF_IS_REPEAT);
-  ForwardCapturedKeyEvent(std::move(key_event));
+  ForwardCapturedKeyEvent(key_event.get());
 
   return true;
 }
 
-void KeyboardHookWinImpl::UpdateModifierState(DWORD vk, bool is_key_down) {
+void ModifierKeyboardHookWinImpl::UpdateModifierState(DWORD vk,
+                                                      bool is_key_down) {
   BYTE keyboard_state[kKeyboardStateArraySize] = {0};
   if (!GetKeyboardState(keyboard_state)) {
     DPLOG(ERROR) << "GetKeyboardState() failed: ";
@@ -318,46 +295,24 @@ void KeyboardHookWinImpl::UpdateModifierState(DWORD vk, bool is_key_down) {
 }
 
 // static
-LRESULT CALLBACK KeyboardHookWinImpl::ProcessKeyEvent(int code,
-                                                      WPARAM w_param,
-                                                      LPARAM l_param) {
-  // If there is an error unhooking, this method could be called with a null
-  // |instance_|.  Ensure we have a valid instance and that |code| is correct
-  // before proceeding.
-  if (!instance_ || code != HC_ACTION)
-    return CallNextHookEx(nullptr, code, w_param, l_param);
-
-  DCHECK_CALLED_ON_VALID_THREAD(instance_->thread_checker_);
-
-  KBDLLHOOKSTRUCT* ll_hooks = reinterpret_cast<KBDLLHOOKSTRUCT*>(l_param);
-
-  // This vkey represents both a vkey and a location on the keyboard such as
-  // VK_LCONTROL or VK_RCONTROL.
-  DWORD vk = ll_hooks->vkCode;
-
-  // Apply the extended flag prior to passing |scan_code| since |instance_| does
-  // not have access to the low-level hook flags.
-  DWORD scan_code = ll_hooks->scanCode;
-  if (ll_hooks->flags & LLKHF_EXTENDED)
-    scan_code |= 0xE000;
-
-  if (instance_->ProcessKeyEventMessage(w_param, vk, scan_code, ll_hooks->time))
-    return 1;
-
-  return CallNextHookEx(nullptr, code, w_param, l_param);
+LRESULT CALLBACK ModifierKeyboardHookWinImpl::ProcessKeyEvent(int code,
+                                                              WPARAM w_param,
+                                                              LPARAM l_param) {
+  return KeyboardHookWinBase::ProcessKeyEvent(instance_, code, w_param,
+                                              l_param);
 }
 
 }  // namespace
 
 // static
-std::unique_ptr<KeyboardHook> KeyboardHook::Create(
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     gfx::AcceleratedWidget accelerated_widget,
     KeyEventCallback callback) {
-  std::unique_ptr<KeyboardHookWinImpl> keyboard_hook =
-      std::make_unique<KeyboardHookWinImpl>(std::move(dom_codes),
-                                            std::move(callback),
-                                            /*enable_hook_registration=*/true);
+  std::unique_ptr<ModifierKeyboardHookWinImpl> keyboard_hook =
+      std::make_unique<ModifierKeyboardHookWinImpl>(
+          std::move(dom_codes), std::move(callback),
+          /*enable_hook_registration=*/true);
 
   if (!keyboard_hook->Register())
     return nullptr;
@@ -365,19 +320,13 @@ std::unique_ptr<KeyboardHook> KeyboardHook::Create(
   return keyboard_hook;
 }
 
-std::unique_ptr<KeyboardHookWin> KeyboardHookWin::CreateForTesting(
+std::unique_ptr<KeyboardHookWinBase>
+KeyboardHookWinBase::CreateModifierKeyboardHookForTesting(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     KeyEventCallback callback) {
-  return std::make_unique<KeyboardHookWinImpl>(
+  return std::make_unique<ModifierKeyboardHookWinImpl>(
       std::move(dom_codes), std::move(callback),
       /*enable_hook_registration=*/false);
 }
 
-KeyboardHookWin::KeyboardHookWin(
-    base::Optional<base::flat_set<DomCode>> dom_codes,
-    KeyEventCallback callback)
-    : KeyboardHookBase(std::move(dom_codes), std::move(callback)) {}
-
-KeyboardHookWin::~KeyboardHookWin() = default;
-
 }  // namespace ui

ui/events/x/keyboard_hook_x11.cc

@@ -156,7 +156,7 @@ void KeyboardHookX11::CaptureKeyForDomCode(DomCode dom_code) {
 }  // namespace
 
 // static
-std::unique_ptr<KeyboardHook> KeyboardHook::Create(
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
     base::Optional<base::flat_set<DomCode>> dom_codes,
     gfx::AcceleratedWidget accelerated_widget,
     KeyboardHook::KeyEventCallback callback) {
@@ -169,4 +169,10 @@ std::unique_ptr<KeyboardHook> KeyboardHook::Create(
   return keyboard_hook;
 }
 
+// static
+std::unique_ptr<KeyboardHook> KeyboardHook::CreateMediaKeyboardHook(
+    KeyEventCallback callback) {
+  return nullptr;
+}
+
 }  // namespace ui

ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc

@@ -578,7 +578,7 @@ bool DesktopWindowTreeHostWin::CaptureSystemKeyEventsImpl(
   // problems with event routing (i.e. which Hook takes precedence) and
   // destruction ordering.
   DCHECK(!keyboard_hook_);
-  keyboard_hook_ = ui::KeyboardHook::Create(
+  keyboard_hook_ = ui::KeyboardHook::CreateModifierKeyboardHook(
       std::move(dom_codes), GetAcceleratedWidget(),
       base::BindRepeating(&DesktopWindowTreeHostWin::HandleKeyEvent,
                           base::Unretained(this)));

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

@@ -1318,7 +1318,7 @@ bool DesktopWindowTreeHostX11::CaptureSystemKeyEventsImpl(
   // problems with event routing (i.e. which Hook takes precedence) and
   // destruction ordering.
   DCHECK(!keyboard_hook_);
-  keyboard_hook_ = ui::KeyboardHook::Create(
+  keyboard_hook_ = ui::KeyboardHook::CreateModifierKeyboardHook(
       std::move(dom_codes), GetAcceleratedWidget(),
       base::BindRepeating(&DesktopWindowTreeHostX11::DispatchKeyEvent,
                           base::Unretained(this)));