Update input_injector_win.cc to support injecting wheel-x and wheel-y events together

remoting/host/input_injector_win.cc

 // Copyright (c) 2012 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 "remoting/host/input_injector.h"
 
 #include <stdint.h>
 #include <windows.h>
 
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "remoting/base/util.h"
 #include "remoting/host/clipboard.h"
 #include "remoting/host/touch_injector_win.h"
 #include "remoting/proto/event.pb.h"
 #include "ui/events/keycodes/dom/keycode_converter.h"
 
 namespace remoting {
 
+using protocol::ClipboardEvent;
+using protocol::KeyEvent;
+using protocol::TextEvent;
+using protocol::MouseEvent;
+using protocol::TouchEvent;
+using std::vector;

[Sergey Ulanov, 2016/04/06 18:25:31]

Please just refer to std::vector<> everywhere below instead of adding this using
statement. 

[Hzj_jie, 2016/04/06 19:56:34]

Yes, I can do it, but why do we prefer removing this using statement?

[Sergey Ulanov, 2016/04/06 20:42:20]

We don't use using statement for STL types anywhere else. In general using
statements have some costs, they are not free. They make the code harder to read
(one have to scroll to find that using statement). The code is also harder to
move, e.g. to a file that doesn't have the same using statement. In this
particular case std::vector is used in 4 places in this file, so you don't save
much by having this statement. 
Ideally I would prefer removing the other using statements above for protocol::
types as well, but I don't mind keeping them as they were there before.

[Hzj_jie, 2016/04/06 21:41:05]

Done.

+
 namespace {
 
 // Helper used to call SendInput() API.
 void SendKeyboardInput(uint32_t flags, uint16_t scancode) {
   // Populate a Windows INPUT structure for the event.
   INPUT input;
   memset(&input, 0, sizeof(input));
   input.type = INPUT_KEYBOARD;
   input.ki.time = 0;
   input.ki.dwFlags = flags;
   input.ki.wScan = scancode;
 
   if ((flags & KEYEVENTF_UNICODE) == 0) {
     // Windows scancodes are only 8-bit, so store the low-order byte into the
     // event and set the extended flag if any high-order bits are set. The only
     // high-order values we should see are 0xE0 or 0xE1. The extended bit
     // usually distinguishes keys with the same meaning, e.g. left & right
     // shift.
     input.ki.wScan &= 0xFF;
     if ((scancode & 0xFF00) != 0x0000)
       input.ki.dwFlags |= KEYEVENTF_EXTENDEDKEY;
   }
 
   if (SendInput(1, &input, sizeof(INPUT)) == 0)
     PLOG(ERROR) << "Failed to inject a key event";
 }
 
-using protocol::ClipboardEvent;
-using protocol::KeyEvent;
-using protocol::TextEvent;
-using protocol::MouseEvent;
-using protocol::TouchEvent;
+// Parse move related operations from the input MouseEvent, and insert the
+// result into output.
+void ParseMouseMoveEvent(const MouseEvent& event, vector<INPUT>* output) {
+  DCHECK(output != nullptr);

[Sergey Ulanov, 2016/04/06 18:25:31]

This should be DCHECK(output), but I don't think this DCHECK is useful. Remove
it?

[Hzj_jie, 2016/04/06 19:56:34]

Similar as above, why do we prefer to use, say,
'if (a_pointer)' instead of 'if (a_pointer != nullptr)'?

[Sergey Ulanov, 2016/04/06 20:42:20]

Mainly for consistency with all other code. In this case it makes the code
shorter and just as readable, and there are no downsides.

[Hzj_jie, 2016/04/06 21:41:05]

Done.

+  INPUT input{};

[Sergey Ulanov, 2016/04/06 18:25:31]

C++11 initialization syntax is currently banned, see
http://chromium-cpp.appspot.com/
This can be "input = {0}"

[Hzj_jie, 2016/04/06 19:56:34]

Done.

+  input.type = INPUT_MOUSE;
+
+  if (event.has_delta_x() && event.has_delta_y()) {
+    input.mi.dx = event.delta_x();
+    input.mi.dy = event.delta_y();
+    input.mi.dwFlags = MOUSEEVENTF_MOVE | MOUSEEVENTF_VIRTUALDESK;
+  } else if (event.has_x() && event.has_y()) {
+    int width = GetSystemMetrics(SM_CXVIRTUALSCREEN);
+    int height = GetSystemMetrics(SM_CYVIRTUALSCREEN);
+    if (width > 1 && height > 1) {
+      int x = std::max(0, std::min(width, event.x()));
+      int y = std::max(0, std::min(height, event.y()));
+      input.mi.dx = static_cast<int>((x * 65535) / (width - 1));
+      input.mi.dy = static_cast<int>((y * 65535) / (height - 1));
+      input.mi.dwFlags =
+          MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_VIRTUALDESK;
+    }
+  } else {
+    return;
+  }
+
+  output->push_back(std::move(input));

[Sergey Ulanov, 2016/04/06 18:25:31]

INPUT is a just a struct. I don't think you need std::move() here.

[Hzj_jie, 2016/04/06 19:56:34]

Since it's a trivial type, a move constructor is automatically added by
compiler. So an std::move can eventually trigger the optimized move constructor.

[Sergey Ulanov, 2016/04/06 20:42:20]

Interesting. How can it be optimized? push_back() is normally inlined, so I
think the compiler will generate code that just copies data to the new location
and there is not much room to optimize it.
Anyway, I think it's ok to keep std::move() here. The style guide says this:
 "You may use std::move to express moving a value from one object to another
rather than copying it."

[Hzj_jie, 2016/04/06 21:41:05]

Sorry, I did not express my opinion clear. I do not focus on this specific case,
but just a very common opinion. To any types we do not want to pass via value in
function parameters, using std::move is not a bad idea.

+}
+
+// Parse click related operations from the input MouseEvent, and insert the
+// result into output.
+void ParseMouseClickEvent(const MouseEvent& event, vector<INPUT>* output) {
+  DCHECK(output != nullptr);
+
+  if (event.has_button() && event.has_button_down()) {
+    INPUT input{};
+    input.type = INPUT_MOUSE;
+
+    MouseEvent::MouseButton button = event.button();
+    bool down = event.button_down();
+
+    // If the host is configured to swap left & right buttons, inject swapped
+    // events to un-do that re-mapping.
+    if (GetSystemMetrics(SM_SWAPBUTTON)) {
+      if (button == MouseEvent::BUTTON_LEFT) {
+        button = MouseEvent::BUTTON_RIGHT;
+      } else if (button == MouseEvent::BUTTON_RIGHT) {
+        button = MouseEvent::BUTTON_LEFT;
+      }
+    }
+
+    if (button == MouseEvent::BUTTON_MIDDLE) {
+      input.mi.dwFlags = down ? MOUSEEVENTF_MIDDLEDOWN : MOUSEEVENTF_MIDDLEUP;
+    } else if (button == MouseEvent::BUTTON_RIGHT) {
+      input.mi.dwFlags = down ? MOUSEEVENTF_RIGHTDOWN : MOUSEEVENTF_RIGHTUP;
+    } else {
+      input.mi.dwFlags = down ? MOUSEEVENTF_LEFTDOWN : MOUSEEVENTF_LEFTUP;
+    }
+
+    output->push_back(std::move(input));
+  }
+}
+
+// Parse wheel related operations from the input MouseEvent, and insert the
+// result into output.
+void ParseMouseWheelEvent(const MouseEvent& event, vector<INPUT>* output) {
+  DCHECK(output != nullptr);
+
+  if (event.has_wheel_delta_x()) {
+    int delta = static_cast<int>(event.wheel_delta_x());
+    if (delta != 0) {
+      INPUT input{};
+      input.type = INPUT_MOUSE;
+      input.mi.mouseData = delta;
+      // According to MSDN, if dwFlags does not contain MOUSEEVENTF_WHEEL, the
+      // event should have mouseData = 0. Without MOUSEDEVENTF_WHEEL, mouseData
+      // > 0 will cause this INPUT to be invalid.

[Sergey Ulanov, 2016/04/06 18:25:31]

This comment is confusing. Maybe just say that both flags need to be set for
horizontal wheel?
Does this mean that horizontal wheel movement were broken before this change?

[Hzj_jie, 2016/04/06 19:56:34]

Updated.

The original implementation has two issues,
1. Even with only horizontal wheel movement, we need to set both
MOUSEEVENTF_WHEEL and MOUSEEVENTF_HWHELL for dwFlags.
2. If one MouseEvent contains both horizontal wheel movement and vertical wheel
movement, vertical one will be ignored, but the delta value is still coming from
vertical one. Well, it was totally wrong.

+      input.mi.dwFlags = MOUSEEVENTF_HWHEEL | MOUSEEVENTF_WHEEL;
+      output->push_back(std::move(input));
+    }
+  }
+
+  if (event.has_wheel_delta_y()) {
+    int delta = static_cast<int>(event.wheel_delta_y());
+    if (delta != 0) {
+      INPUT input{};
+      input.type = INPUT_MOUSE;
+      input.mi.mouseData = delta;
+      input.mi.dwFlags = MOUSEEVENTF_WHEEL;
+      output->push_back(std::move(input));
+    }
+  }
+}
 
 // A class to generate events on Windows.
 class InputInjectorWin : public InputInjector {
  public:
   InputInjectorWin(scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
                    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
   ~InputInjectorWin() override;
 
   // ClipboardStub interface.
   void InjectClipboardEvent(const ClipboardEvent& event) override;
@@ skipping 199 matching lines @@
        it != text.end(); ++it)  {
     SendKeyboardInput(KEYEVENTF_UNICODE, *it);
     SendKeyboardInput(KEYEVENTF_UNICODE | KEYEVENTF_KEYUP, *it);
   }
 }
 
 void InputInjectorWin::Core::HandleMouse(const MouseEvent& event) {
   // Reset the system idle suspend timeout.
   SetThreadExecutionState(ES_SYSTEM_REQUIRED);
 
-  INPUT input;
-  memset(&input, 0, sizeof(input));
-  input.type = INPUT_MOUSE;
+  vector<INPUT> inputs;
+  ParseMouseMoveEvent(event, &inputs);
+  ParseMouseClickEvent(event, &inputs);
+  ParseMouseWheelEvent(event, &inputs);
 
-  if (event.has_delta_x() && event.has_delta_y()) {
-    input.mi.dx = event.delta_x();
-    input.mi.dy = event.delta_y();
-    input.mi.dwFlags |= MOUSEEVENTF_MOVE | MOUSEEVENTF_VIRTUALDESK;
-  } else if (event.has_x() && event.has_y()) {
-    int width = GetSystemMetrics(SM_CXVIRTUALSCREEN);
-    int height = GetSystemMetrics(SM_CYVIRTUALSCREEN);
-    if (width > 1 && height > 1) {
-      int x = std::max(0, std::min(width, event.x()));
-      int y = std::max(0, std::min(height, event.y()));
-      input.mi.dx = static_cast<int>((x * 65535) / (width - 1));
-      input.mi.dy = static_cast<int>((y * 65535) / (height - 1));
-      input.mi.dwFlags |=
-          MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_VIRTUALDESK;
-    }
-  }
-
-  int wheel_delta_x = 0;
-  int wheel_delta_y = 0;
-  if (event.has_wheel_delta_x() && event.has_wheel_delta_y()) {
-    wheel_delta_x = static_cast<int>(event.wheel_delta_x());
-    wheel_delta_y = static_cast<int>(event.wheel_delta_y());
-  }
-
-  if (wheel_delta_x != 0 || wheel_delta_y != 0) {
-    if (wheel_delta_x != 0) {
-      input.mi.mouseData = wheel_delta_x;
-      input.mi.dwFlags |= MOUSEEVENTF_HWHEEL;
-    }
-    if (wheel_delta_y != 0) {
-      input.mi.mouseData = wheel_delta_y;
-      input.mi.dwFlags |= MOUSEEVENTF_WHEEL;
-    }
-  }
-
-  if (event.has_button() && event.has_button_down()) {
-    MouseEvent::MouseButton button = event.button();
-    bool down = event.button_down();
-
-    // If the host is configured to swap left & right buttons, inject swapped
-    // events to un-do that re-mapping.
-    if (GetSystemMetrics(SM_SWAPBUTTON)) {
-      if (button == MouseEvent::BUTTON_LEFT) {
-        button = MouseEvent::BUTTON_RIGHT;
-      } else if (button == MouseEvent::BUTTON_RIGHT) {
-        button = MouseEvent::BUTTON_LEFT;
-      }
-    }
-
-    if (button == MouseEvent::BUTTON_LEFT) {
-      input.mi.dwFlags |= down ? MOUSEEVENTF_LEFTDOWN : MOUSEEVENTF_LEFTUP;
-    } else if (button == MouseEvent::BUTTON_MIDDLE) {
-      input.mi.dwFlags |= down ? MOUSEEVENTF_MIDDLEDOWN : MOUSEEVENTF_MIDDLEUP;
-    } else if (button == MouseEvent::BUTTON_RIGHT) {
-      input.mi.dwFlags |= down ? MOUSEEVENTF_RIGHTDOWN : MOUSEEVENTF_RIGHTUP;
-    } else {
-      input.mi.dwFlags |= down ? MOUSEEVENTF_LEFTDOWN : MOUSEEVENTF_LEFTUP;
-    }
-  }
-
-  if (input.mi.dwFlags) {
-    if (SendInput(1, &input, sizeof(INPUT)) == 0)
+  if (!inputs.empty()) {
+    if (SendInput(inputs.size(), inputs.data(), sizeof(INPUT)) != inputs.size())
       PLOG(ERROR) << "Failed to inject a mouse event";
   }
 }
 
 void InputInjectorWin::Core::HandleTouch(const TouchEvent& event) {
   touch_injector_.InjectTouchEvent(event);
 }
 
 }  // namespace
 
 // static
 scoped_ptr<InputInjector> InputInjector::Create(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
   return make_scoped_ptr(
       new InputInjectorWin(main_task_runner, ui_task_runner));
 }
 
 // static
 bool InputInjector::SupportsTouchEvents() {
   return TouchInjectorWinDelegate::Create() != nullptr;
 }
 
 }  // namespace remoting