[Windows] Finish global and non-global media keys support on Windows.

chrome/browser/extensions/extension_commands_global_registry_apitest.cc

Index: chrome/browser/extensions/extension_commands_global_registry_apitest.cc
diff --git a/chrome/browser/extensions/extension_commands_global_registry_apitest.cc b/chrome/browser/extensions/extension_commands_global_registry_apitest.cc
index b9b5f6d7c77b83a684ffc192f8a59295f93ba3b4..c18e8482fdaf0e50ed3b336cb883c1ea6740fb73 100644
--- a/chrome/browser/extensions/extension_commands_global_registry_apitest.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry_apitest.cc
@@ -17,6 +17,8 @@
 
 #include "ui/events/keycodes/keyboard_code_conversion_x.h"
 #include "ui/gfx/x/x11_types.h"
+#elif defined(OS_WIN)
+#include "ui/events/keycodes/keyboard_code_conversion_win.h"
 #endif
 
 namespace extensions {
@@ -66,6 +68,91 @@ void SendNativeKeyEventToXDisplay(ui::KeyboardCode key,
 }
 #endif  // OS_LINUX
 
+#if defined(OS_WIN)
+bool FillKeyboardInput(ui::KeyboardCode key, INPUT* input, bool key_up) {
+  memset(input, 0, sizeof(INPUT));
+  input->type = INPUT_KEYBOARD;
+  input->ki.wVk = ui::WindowsKeyCodeForKeyboardCode(key);
+  input->ki.dwFlags = key_up ? KEYEVENTF_EXTENDEDKEY | KEYEVENTF_KEYUP :
+                               KEYEVENTF_EXTENDEDKEY;
+
+  return true;
+}

[Finnur, 2013/11/12 16:27:54]

Why do we need to copy this function?

+
+// This function is modified from |SendKeyPressImpl| of
+// ui_controls_internal_win.cc. Use ui_test_utils::SendKeyPressSync when testing
+// global media keys will fail, this is because we have register the single key
+// using RegisterHotKey, so the keyboard hook of current thread will never be
+// called when we only send the single key (For example: "MediaNextTrack" down
+// -> "MediaNextTrack" up). The difference when testing global key combinations:
+// the key event are sent one by one. For example, sending "Ctrl+Shift+1", we
+// will send "Ctrl" down -> "Shift" down -> "1" down, (here the hot key is
+// activated.), then 1 "up" (here InputDispatcher will receive an key up event,
+// then the message loop in |SendKeyPressToWindowSync| could stop running
+// normally) -> "Shift" up -> "Ctrl" up.
+//
+// Note: The above comment is only when I have digged out when investigate why
+// using ui_test_utils::SendKeyPressSync to test global media keys will fail.
+// I need some help about whether we can use this custom function?
+// TODO(zhchbin): Clear up the comment.

[Finnur, 2013/11/12 16:27:54]

I think this is not the right approach.
1) I'm not entirely sure why this function works but the one in ui_test_utils
does not. I'd like to understand that better.
2) If we absolutely need to do this, then we should split the function up into
two (like you have done) but have them both live there (and one call the other)
and not have one live here (and be almost an exact copy of the other one).

[zhchbin, 2013/11/13 05:01:57]

You can do an experiment: 
1. Change the SendNativeKeyEvent in my test case to ui_test_utils one. Then you
can find it fail.
2. Now add a little trick patch to /ui/base/test/ui_controls_internal_win.cc

diff --git a/ui/base/test/ui_controls_internal_win.cc
b/ui/base/test/ui_controls_internal_win.cc
index 45b8f45..9e264bd 100644
--- a/ui/base/test/ui_controls_internal_win.cc
+++ b/ui/base/test/ui_controls_internal_win.cc
@@ -246,6 +246,11 @@ bool SendKeyPressImpl(HWND window,
     i++;
   }

+  if (!control && !shift && !alt) {
+    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], false);
+    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], true);
+  }
+
   if (::SendInput(i, input, sizeof(INPUT)) != i)
     return false;

With this trick way, you can find it can work now.

The difference between my custom function and the one in ui_test_utils is:
ui_test_utils one will wait the WM_KEYUP event in its message loop.

The purpose I copied the function here is want to show you the difference and
asking for advice.

[zhchbin, 2013/11/13 16:05:10]

Maybe you ignore this comment?

[Finnur, 2013/11/18 11:16:14]

I didn't have a good advice for you because I don't feel like I understand the
problem well. I'd like to take a look at the message loop that handles it as it
seems like that loop is wrong. Where is it exactly? Specifically, there is a
keyup message sent (when the non-modifier key is set in the input stream) -- but
I presume the loop assume the keyup must be present on a non-modifier key)? 

Maybe we should change the messageloop (and the test code) to send a custom
WM_USER message (or something) to let it know that no further processing is to
be expected?

[zhchbin, 2013/11/18 13:10:13]

The message loop is:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...
And the detail of windows:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/ui_co....

I have no idea how we can fix it yet.

[Finnur, 2013/11/18 14:14:53]

I think this is the one remaining issue before you can check this in (apart from
the DCHECK). There's still some confusion on this issue; might be with me -- but
we need to get to the bottom of this. :)

How did you figure out that KeyDown+KeyUp of Ctrl was enough to get the loop to
stop processing? I was hoping to see where that decision is made (to stop on
KeyUp) but I didn't see it from the links. Unfortunately, I don't have the time
right now to patch in your CL and start debugging, as I'm a few days behind on
the schedule I promised for another task, so I'll need to try to understand this
based on the information you give me.

One thing that I don't understand is that the code below does not send
KeyDown+KeyUp of Ctrl, yet it works better than the helper function provided in
the util class. Why is that?

[zhchbin, 2013/11/18 14:54:44]

Em... first of all, sorry for my poor English that couldn't help you understand
what's happening.

I had tried my best to explain the problem in the comment of the code. Let me
try to explain again.

1. |SendKeyPressSync| is a wrapper of |SendKeyPressToWindowSync|.
2. |SendKeyPressToWindowSync| is an synchronized function, it will wait the
result whether the specified window has receive the keys. (If timeout happen,
the test case will fail.)
3. On windows, the media keys has been registered by our API, so the
InputDispatcher didn't get notified when only media keys are sent.
InputDispatcher is using Keyboard Hook to listen for a keyboard event
(WM_KEYUP). So adding a irrelevant keyup/down, like what I have shown you, can
help in this situation. Also take a look again my comments: 

A single key: "MediaNextTrack" down -> "MediaNextTrack" up (for example).

Key Combination: Sending "Ctrl+Shift+1", we will send "Ctrl" down -> "Shift"
down -> "1" down, (here the hot key is activated.), then 1 "up" (here
InputDispatcher will receive an key up event, then the message loop in
|SendKeyPressToWindowSync| could stop running normally) -> "Shift" up -> "Ctrl"
up.

4. Last question: Because the bellow code just send the keys out and ignore who
will receive the keys. We didn't wait for any result.

Maybe you can read the links I have gave first of all. Anyway, deal with this
this issue only when you are available for it. I'm OK all the times. Thanks! :)

+bool SendNativeKeyEvent(ui::KeyboardCode key,
+                        bool control,
+                        bool shift,
+                        bool alt) {
+  INPUT input[8] = { 0 };  // 8, assuming all the modifiers are activated.
+
+  UINT i = 0;
+  if (control) {
+    if (!FillKeyboardInput(ui::VKEY_CONTROL, &input[i], false))
+      return false;
+    i++;
+  }
+
+  if (shift) {
+    if (!FillKeyboardInput(ui::VKEY_SHIFT, &input[i], false))
+      return false;
+    i++;
+  }
+
+  if (alt) {
+    if (!FillKeyboardInput(ui::VKEY_MENU, &input[i], false))
+      return false;
+    i++;
+  }
+
+  if (!FillKeyboardInput(key, &input[i], false))
+    return false;
+  i++;
+
+  if (!FillKeyboardInput(key, &input[i], true))
+    return false;
+  i++;
+
+  if (alt) {
+    if (!FillKeyboardInput(ui::VKEY_MENU, &input[i], true))
+      return false;
+    i++;
+  }
+
+  if (shift) {
+    if (!FillKeyboardInput(ui::VKEY_SHIFT, &input[i], true))
+      return false;
+    i++;
+  }
+
+  if (control) {
+    if (!FillKeyboardInput(ui::VKEY_CONTROL, &input[i], true))
+      return false;
+    i++;
+  }
+
+  if (::SendInput(i, input, sizeof(INPUT)) != i)
+    return false;
+
+  return true;
+}
+#endif  // OS_WIN
+
 #if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 // The feature is only fully implemented on Windows and Linux, other platforms
 // coming.
@@ -128,4 +215,31 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
+#if defined(OS_WIN)
+// The feature is only fully implemented on Windows, other platforms coming.
+#define MAYBE_GlobalDuplicatedMediaKey GlobalDuplicatedMediaKey
+#else
+#define MAYBE_GlobalDuplicatedMediaKey DISABLED_GlobalDuplicatedMediaKey
+#endif
+
+IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalDuplicatedMediaKey) {
+  FeatureSwitch::ScopedOverride enable_global_commands(
+      FeatureSwitch::global_commands(), true);
+
+  ResultCatcher catcher;
+  ASSERT_TRUE(RunExtensionTest("keybinding/global_media_keys_0")) << message_;
+  ASSERT_TRUE(catcher.GetNextResult());
+  ASSERT_TRUE(RunExtensionTest("keybinding/global_media_keys_1")) << message_;
+  ASSERT_TRUE(catcher.GetNextResult());
+
+  WindowController* controller = browser()->extension_window_controller();
+  controller->window()->Minimize();
+
+  SendNativeKeyEvent(ui::VKEY_MEDIA_NEXT_TRACK, false, false, false);
+
+  // We should get two success result.
+  ASSERT_TRUE(catcher.GetNextResult());
+  ASSERT_TRUE(catcher.GetNextResult());
+}
+
 }  // namespace extensions