Suppress 0x0 MotionNotify events between button presses and releases.

remoting/host/event_executor_linux.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/event_executor.h"
 
 #include <set>
 
 #include <X11/Xlib.h>
 #include <X11/XF86keysym.h>
 #include <X11/keysym.h>
 #include <X11/extensions/XTest.h>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "remoting/proto/internal.pb.h"
+#include "third_party/skia/include/core/SkPoint.h"
 
 namespace remoting {
 
 namespace {
 
 using protocol::ClipboardEvent;
 using protocol::KeyEvent;
 using protocol::MouseEvent;
 
 // USB to XKB keycode map table.
@@ skipping 25 matching lines @@
  private:
   // |mode| is one of the AutoRepeatModeOn, AutoRepeatModeOff,
   // AutoRepeatModeDefault constants defined by the XChangeKeyboardControl()
   // API.
   void SetAutoRepeatForKey(int keycode, int mode);
   void InjectScrollWheelClicks(int button, int count);
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   std::set<int> pressed_keys_;
+  SkIPoint latest_mouse_position_;

[simonmorris, 2012/07/23 16:45:01]

Drive-by: Do you need to initialise this member?

[Wez, 2012/07/23 17:30:16]

Yes! Good spot. I wonder how the Skia folks would feel about having these types
initialized to something...

[Wez, 2012/07/23 17:30:16]

Done.

 
   // X11 graphics context.
   Display* display_;
   Window root_window_;
 
   int test_event_base_;
   int test_error_base_;
 
   DISALLOW_COPY_AND_ASSIGN(EventExecutorLinux);
 };
@@ skipping 303 matching lines @@
 void EventExecutorLinux::InjectMouseEvent(const MouseEvent& event) {
   if (!task_runner_->BelongsToCurrentThread()) {
     task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&EventExecutorLinux::InjectMouseEvent,
                    base::Unretained(this), event));
     return;
   }
 
   if (event.has_x() && event.has_y()) {
-    VLOG(3) << "Moving mouse to " << event.x()
-            << "," << event.y();
-    XTestFakeMotionEvent(display_, DefaultScreen(display_),
-                         event.x(), event.y(),
-                         CurrentTime);
+    // Injecting a motion event immediately before a button release results in
+    // a MotionNotify even if the mouse position hasn't changed, which confuses
+    // apps which assume MotionNotify implies movement. See crbug.com/138075.
+    bool inject_motion = true;
+    if (event.has_button() && event.has_button_down() && !event.button_down()) {

[simonmorris, 2012/07/23 16:45:01]

Drive-by: Is there any need for these button tests? Should we ever inject a
motion event if the mouse position hasn't changed?

[Wez, 2012/07/23 17:30:16]

Yes; if a local user moves the cursor and after waiting for the block on remote
input to expire, the remote user clicks, then the position in
last_mouse_position_ will cause us not to move the mouse to the right position
before clicking.

+      if (SkIPoint::Make(event.x(), event.y()) == latest_mouse_position_)
+        inject_motion = false;
+    }
+
+    latest_mouse_position_ = SkIPoint::Make(event.x(), event.y());

[Jamie, 2012/07/23 16:44:16]

Nit: It might be cleaner to declare a variable, new_mouse_position, rather than
calling Make twice.

[Wez, 2012/07/23 17:30:16]

Done.

+
+    if (inject_motion) {
+      VLOG(3) << "Moving mouse to " << event.x()
+              << "," << event.y();
+      XTestFakeMotionEvent(display_, DefaultScreen(display_),
+                           event.x(), event.y(),
+                           CurrentTime);
+    }
   }
 
   if (event.has_button() && event.has_button_down()) {
     int button_number = MouseButtonToX11ButtonNumber(event.button());
 
     if (button_number < 0) {
       LOG(WARNING) << "Ignoring unknown button type: " << event.button();
       return;
     }
 
@@ skipping 34 matching lines @@
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
     VideoFrameCapturer* capturer) {
   scoped_ptr<EventExecutorLinux> executor(
       new EventExecutorLinux(main_task_runner));
   if (!executor->Init())
     return scoped_ptr<EventExecutor>(NULL);
   return executor.PassAs<EventExecutor>();
 }
 
 }  // namespace remoting