Run event executor on the ui thread to remove the need to explicitly XFlush() the XTest calls.

remoting/host/event_executor_linux.cc

 // Copyright (c) 2010 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_linux.h"
 
 #include <X11/Xlib.h>
 #include <X11/keysym.h>
 #include <X11/extensions/XTest.h>
+#include <gdk/gdkx.h>
 
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/task.h"
 #include "remoting/proto/internal.pb.h"
 
-// TODO(jamiewalch): Class to ensure that XFlush is called regardless of what
-// fields are set in the MouseEvent. It's not worth refactoring HandleMouse to
-// avoid the early returns because calling XFlush is not really the correct
-// way of flushing the XTest requests; instead we should dispatch the requests
-// to a suitable UI thread.
-namespace {
-class ScopedXFlusher {
- public:
-  explicit ScopedXFlusher(Display* display)
-      : display_(display), needs_flush_(false) {
-  }
-
-  ~ScopedXFlusher() {
-    if (needs_flush_)
-      XFlush(display_);
-  }
-
-  void SignalFlush() {
-    needs_flush_ = true;
-  }
-
- private:
-  Display* display_;
-  bool needs_flush_;
-};
-}
-
 namespace remoting {
 
 using protocol::MouseEvent;
 using protocol::KeyEvent;
 
 static int MouseButtonToX11ButtonNumber(
     protocol::MouseEvent::MouseButton button) {
   switch (button) {
     case MouseEvent::BUTTON_LEFT:
       return 1;
@@ skipping 207 matching lines @@
       root_window_(BadValue),
       width_(0),
       height_(0) {
 }
 
 EventExecutorLinuxPimpl::~EventExecutorLinuxPimpl() {
   DeinitXlib();
 }
 
 bool EventExecutorLinuxPimpl::Init() {
-  // TODO(ajwong): We should specify the display string we are attaching to
-  // in the constructor.
-  display_ = XOpenDisplay(NULL);
+  display_ = GDK_DISPLAY_XDISPLAY(gdk_display_get_default());

[awong, 2011/02/14 17:27:29]

hmm...can we poke a hole into the message loop such that it gives us the
associated display back? I'm leery of leaking an implicit GDK dependency here.

In the contenxt, we'd want to store a MessageLoopUI instead of just MessageLoop.

[Jamie, 2011/02/14 19:12:38]

I agree, but I couldn't see any clean way of doing this. Passing a MessageLoopUI
into the context means also passing one into ServiceProcess::Initialize, which
made me nervous. Even if we do do that, it's not clear how we would get at a
platform-specific Display pointer from a cross-platform interface.

   if (!display_) {
     LOG(ERROR) << "Unable to open display";
     return false;
   }
 
   root_window_ = RootWindow(display_, DefaultScreen(display_));
   if (root_window_ == BadValue) {
     LOG(ERROR) << "Unable to get the root window";
     DeinitXlib();
     return false;
@@ skipping 39 matching lines @@
   if (keycode == 0) {
     LOG(WARNING) << "Ignoring undefined keysym: " << keysym
                  << " for key: " << key_event->keycode();
     return;
   }
 
   VLOG(3) << "Got pepper key: " << key_event->keycode()
           << " sending keysym: " << keysym
           << " to keycode: " << keycode;
   XTestFakeKeyEvent(display_, keycode, key_event->pressed(), CurrentTime);
-
-  // TODO(jamiewalch): Get rid of this once we're dispatching to the UI thread.
-  XFlush(display_);
 }
 
 void EventExecutorLinuxPimpl::HandleMouse(const MouseEvent* event) {
-  ScopedXFlusher flusher(display_);
   if (event->has_x() && event->has_y()) {
     if (event->x() < 0 || event->y() < 0 ||
         event->x() > width_ || event->y() > height_) {
       // A misbehaving client may send these. Drop events that are out of range.
       // TODO(ajwong): How can we log this sanely? We don't want to DOS the
       // server with a misbehaving client by logging like crazy.
       return;
     }
 
     VLOG(3) << "Moving mouse to " << event->x()
             << "," << event->y();
-    flusher.SignalFlush();
     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;
     }
 
     VLOG(3) << "Button " << event->button()
             << " received, sending down " << button_number;
-    flusher.SignalFlush();
     XTestFakeButtonEvent(display_, button_number, event->button_down(),
                          CurrentTime);
   }
 
   if (event->has_wheel_offset_x() && event->has_wheel_offset_y()) {
     NOTIMPLEMENTED() << "No scroll wheel support yet.";
   }
 }
 
 void EventExecutorLinuxPimpl::DeinitXlib() {
@@ skipping 43 matching lines @@
   done->Run();
   delete done;
 }
 
 protocol::InputStub* CreateEventExecutor(MessageLoop* message_loop,
                                          Capturer* capturer) {
   return new EventExecutorLinux(message_loop, capturer);
 }
 
 }  // namespace remoting