Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop

ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc

Index: ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc
diff --git a/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc b/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc
index 1db4a2226ef5bbe561958d839092029211a053d2..f77dfe89c04211ab4f2aa30d7e0c68d4abad1595 100644
--- a/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc
+++ b/ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc
@@ -4,6 +4,7 @@
 
 #include "ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h"
 
+#include <X11/extensions/XInput2.h>
 #include <X11/Xlib.h>
 // Get rid of a macro from Xlib.h that conflicts with Aura's RootWindow class.
 #undef RootWindow
@@ -36,23 +37,6 @@ namespace {
 const uint32 kMinAlpha = 32;
 const unsigned char kDragWidgetOpacity = 0xc0;
 
-class ScopedCapturer {
- public:
-  explicit ScopedCapturer(aura::WindowTreeHost* host)
-      : host_(host) {
-    host_->SetCapture();
-  }
-
-  ~ScopedCapturer() {
-    host_->ReleaseCapture();
-  }
-
- private:
-  aura::WindowTreeHost* host_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedCapturer);
-};
-
 }  // namespace
 
 X11WholeScreenMoveLoop::X11WholeScreenMoveLoop(
@@ -60,7 +44,6 @@ X11WholeScreenMoveLoop::X11WholeScreenMoveLoop(
     : delegate_(delegate),
       in_move_loop_(false),
       should_reset_mouse_flags_(false),
-      grab_input_window_(None),
       canceled_(false),
       weak_factory_(this) {
   last_xmotion_.type = LASTEvent;
@@ -81,17 +64,13 @@ void X11WholeScreenMoveLoop::DispatchMouseMovement() {
 // DesktopWindowTreeHostLinux, ui::PlatformEventDispatcher implementation:
 
 bool X11WholeScreenMoveLoop::CanDispatchEvent(const ui::PlatformEvent& event) {
+  NOTREACHED();
   return in_move_loop_;

[pkotwicz, 2014/05/05 18:14:37]

returning true would probably be more appropriate here

[varkha, 2014/05/05 19:06:04]

Done.

 }
 
 uint32_t X11WholeScreenMoveLoop::DispatchEvent(const ui::PlatformEvent& event) {
-  // This method processes all events for the grab_input_window_ as well as
-  // mouse events for all windows while the move loop is active - even before
-  // the grab is granted by X. This allows mouse notification events that were
-  // sent after the capture was requested but before the capture was granted
-  // to be dispatched. It is especially important to process the mouse release
-  // event that should have stopped the drag even if that mouse release happened
-  // before the grab was granted.
+  // This method processes all mouse events for all windows while the move loop
+  // is active.
   if (!in_move_loop_)
     return ui::POST_DISPATCH_PERFORM_DEFAULT;
   XEvent* xev = event;
@@ -126,7 +105,8 @@ uint32_t X11WholeScreenMoveLoop::DispatchEvent(const ui::PlatformEvent& event) {
         DispatchMouseMovement();
         delegate_->OnMouseReleased();
       }
-      return ui::POST_DISPATCH_NONE;
+      // default dispatch will release capture in Widget::OnMouseEvent.

[pkotwicz, 2014/05/05 18:14:37]

Nit: Widget::OnMouseEvent()

[varkha, 2014/05/05 19:06:04]

Done.

+      break;
     }
     case KeyPress: {
       if (ui::KeyboardCodeFromXKeyEvent(xev) == ui::VKEY_ESCAPE) {
@@ -150,7 +130,9 @@ uint32_t X11WholeScreenMoveLoop::DispatchEvent(const ui::PlatformEvent& event) {
             xevent.type = MotionNotify;
           }
           xevent.xany.display = xev->xgeneric.display;
-          xevent.xany.window = grab_input_window_;
+          XIDeviceEvent* xievent =
+              static_cast<XIDeviceEvent*>(xev->xcookie.data);
+          xevent.xany.window = xievent->event;
           // The fields used below are in the same place for all of events
           // above. Using xmotion from XEvent's unions to avoid repeating
           // the code.
@@ -159,8 +141,7 @@ uint32_t X11WholeScreenMoveLoop::DispatchEvent(const ui::PlatformEvent& event) {
           gfx::Point point(ui::EventSystemLocationFromNative(xev));
           xevent.xmotion.x_root = point.x();
           xevent.xmotion.y_root = point.y();
-          DispatchEvent(&xevent);
-          return ui::POST_DISPATCH_NONE;
+          return DispatchEvent(&xevent);
         }
         default:
           break;
@@ -168,41 +149,24 @@ uint32_t X11WholeScreenMoveLoop::DispatchEvent(const ui::PlatformEvent& event) {
     }
   }
 
-  return (event->xany.window == grab_input_window_) ?
-      ui::POST_DISPATCH_NONE : ui::POST_DISPATCH_PERFORM_DEFAULT;
+  return ui::POST_DISPATCH_PERFORM_DEFAULT;
 }
 
 bool X11WholeScreenMoveLoop::RunMoveLoop(aura::Window* source,
                                          gfx::NativeCursor cursor) {
   DCHECK(!in_move_loop_);  // Can only handle one nested loop at a time.
 
-  // Start a capture on the host, so that it continues to receive events during
-  // the drag. This may be second time we are capturing the mouse events - the
-  // first being when a mouse is first pressed. That first capture needs to be
-  // released before the call to GrabPointerAndKeyboard below, otherwise it may
-  // get released while we still need the pointer grab, which is why we restrict
-  // the scope here.
-  {
-    ScopedCapturer capturer(source->GetHost());
-
-    grab_input_window_ = CreateDragInputWindow(gfx::GetXDisplay());
-    // Releasing ScopedCapturer ensures that any other instance of
-    // X11ScopedCapture will not prematurely release grab that will be acquired
-    // below.
-  }
-  // TODO(varkha): Consider integrating GrabPointerAndKeyboard with
-  // ScopedCapturer to avoid possibility of logically keeping multiple grabs.
-  if (!GrabPointerAndKeyboard(cursor)) {
-    XDestroyWindow(gfx::GetXDisplay(), grab_input_window_);
-    return false;
-  }
-
   scoped_ptr<ui::ScopedEventDispatcher> old_dispatcher =
       nested_dispatcher_.Pass();
   nested_dispatcher_ =
          ui::PlatformEventSource::GetInstance()->OverrideDispatcher(this);
-  if (!drag_image_.isNull() && CheckIfIconValid())
+  if (!drag_image_.isNull() && CheckIfIconValid()) {
     CreateDragImageWindow();
+    source = drag_widget_->GetNativeWindow();

[pkotwicz, 2014/05/05 18:14:37]

I wonder whether we can move this logic to DesktopDragDropClientAuraX11 too
This would involve moving SetDragImage(), CreateDragImageWindow() and
CheckIfIconValid() to DesktopDragDropClientAuraX11.

RunMoveLoop() would just do a CHECK() to make sure that |source| has capture.
This matches CrOS behavior as ToplevelWindowEventHandler::RunMoveLoop() does not
set capture as far as I can tell but assumes that |source| has capture.

I suspect that RunMoveLoop() will need to be changed to take in a third
parameter |drag_widget|.

[varkha, 2014/05/05 19:06:04]

I'll try that. I have uploaded a patch with the rest addressed in the meanwhile.

[varkha, 2014/05/06 22:02:41]

Done.

+  }
+  // Start a capture on the source window (drag widget for drag and drop),
+  // so that it continues to receive events during the drag.
+  source->SetCapture();
 
   // We are handling a mouse drag outside of the aura::RootWindow system. We
   // must manually make aura think that the mouse button is pressed so that we
@@ -224,15 +188,6 @@ bool X11WholeScreenMoveLoop::RunMoveLoop(aura::Window* source,
   return !canceled_;
 }
 
-void X11WholeScreenMoveLoop::UpdateCursor(gfx::NativeCursor cursor) {
-  if (in_move_loop_) {
-    // If we're still in the move loop, regrab the pointer with the updated
-    // cursor. Note: we can be called from handling an XdndStatus message after
-    // EndMoveLoop() was called, but before we return from the nested RunLoop.
-    GrabPointerAndKeyboard(cursor);
-  }
-}
-
 void X11WholeScreenMoveLoop::EndMoveLoop() {
   if (!in_move_loop_)
     return;
@@ -247,21 +202,10 @@ void X11WholeScreenMoveLoop::EndMoveLoop() {
     should_reset_mouse_flags_ = false;
   }
 
-  // TODO(erg): Is this ungrab the cause of having to click to give input focus
-  // on drawn out windows? Not ungrabbing here screws the X server until I kill
-  // the chrome process.
-
-  // Ungrab before we let go of the window.
-  XDisplay* display = gfx::GetXDisplay();
-  XUngrabPointer(display, CurrentTime);
-  XUngrabKeyboard(display, CurrentTime);
-
   // Restore the previous dispatcher.
   nested_dispatcher_.reset();
   drag_widget_.reset();
   delegate_->OnMoveLoopEnded();
-  XDestroyWindow(display, grab_input_window_);
-  grab_input_window_ = None;
 
   in_move_loop_ = false;
   quit_closure_.Run();
@@ -276,66 +220,6 @@ void X11WholeScreenMoveLoop::SetDragImage(const gfx::ImageSkia& image,
   drag_offset_.set_y(0.f);
 }
 
-bool X11WholeScreenMoveLoop::GrabPointerAndKeyboard(gfx::NativeCursor cursor) {
-  XDisplay* display = gfx::GetXDisplay();
-  XGrabServer(display);
-
-  XUngrabPointer(display, CurrentTime);
-  int ret = XGrabPointer(
-      display,
-      grab_input_window_,
-      False,
-      ButtonPressMask | ButtonReleaseMask | PointerMotionMask,
-      GrabModeAsync,
-      GrabModeAsync,
-      None,
-      cursor.platform(),
-      CurrentTime);
-  if (ret != GrabSuccess) {
-    DLOG(ERROR) << "Grabbing pointer for dragging failed: "
-                << ui::GetX11ErrorString(display, ret);
-  } else {
-    XUngrabKeyboard(display, CurrentTime);
-    ret = XGrabKeyboard(
-        display,
-        grab_input_window_,
-        False,
-        GrabModeAsync,
-        GrabModeAsync,
-        CurrentTime);
-    if (ret != GrabSuccess) {
-      DLOG(ERROR) << "Grabbing keyboard for dragging failed: "
-                  << ui::GetX11ErrorString(display, ret);
-    }
-  }
-
-  XUngrabServer(display);
-  XFlush(display);
-  return ret == GrabSuccess;
-}
-
-Window X11WholeScreenMoveLoop::CreateDragInputWindow(XDisplay* display) {
-  // Creates an invisible, InputOnly toplevel window. This window will receive
-  // all mouse movement for drags. It turns out that normal windows doing a
-  // grab doesn't redirect pointer motion events if the pointer isn't over the
-  // grabbing window. But InputOnly windows are able to grab everything. This
-  // is what GTK+ does, and I found a patch to KDE that did something similar.
-  unsigned long attribute_mask = CWEventMask | CWOverrideRedirect;
-  XSetWindowAttributes swa;
-  memset(&swa, 0, sizeof(swa));
-  swa.event_mask = ButtonPressMask | ButtonReleaseMask | PointerMotionMask |
-                   KeyPressMask | KeyReleaseMask | StructureNotifyMask;
-  swa.override_redirect = True;
-  Window window = XCreateWindow(display,
-                                DefaultRootWindow(display),
-                                -100, -100, 10, 10,
-                                0, CopyFromParent, InputOnly, CopyFromParent,
-                                attribute_mask, &swa);
-  XMapRaised(display, window);
-  ui::X11EventSource::GetInstance()->BlockUntilWindowMapped(window);
-  return window;
-}
-
 void X11WholeScreenMoveLoop::CreateDragImageWindow() {
   Widget* widget = new Widget;
   Widget::InitParams params(Widget::InitParams::TYPE_DRAG);
@@ -358,7 +242,6 @@ void X11WholeScreenMoveLoop::CreateDragImageWindow() {
   widget->SetContentsView(image);
   widget->Show();
   widget->GetNativeWindow()->layer()->SetFillsBoundsOpaquely(false);
-
   drag_widget_.reset(widget);
 }