Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
821803002:
Use XGrabPointer instead of XChangeActivatePointerGrab() to change the cursor during a pointer grab (Closed)
|
Created:
6 years ago by pkotwicz Modified:
5 years, 10 months ago Reviewers:
sadrul CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@fix_capture Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is step 1 in enabling XI2 mouse grab on Desktop Linux.
This CL replaces the use of XChangeActivePointerGrab() (of
which there is no XI2 equivalent) with XGrabPointer (of
which there is an XI2 equivalent).
BUG=426380
TEST=None
Committed: https://crrev.com/955bab609f5c16d36ad51af6b975e33c1b6367b8
Cr-Commit-Position: refs/heads/master@{#313536}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 35 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
pkotwicz@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, PTAL This CL contains the "refactoring" part of https://codereview.chromium.org/749063003/ https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:287: int GrabPointer(XID window, bool owner_events, ::Cursor cursor) { The semantics of DesktopWindowTreeHostX11::SetCapture() do not allow something like ScopedGrabPointer. By design, UngrabPointer() is not called if capture is transferred from one WindowTreeHost to another. I have added comments in DesktopWindowTreeHostX11::SetCapture() and in x11_util.h https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:230: int ret = ui::GrabPointer(grab_input_window_, false, None); Passing None for the Cursor argument seems like the right thing to do. According to http://tronche.com/gui/x/xlib/input/XGrabPointer.html "If None is specified, the normal cursor for that window is displayed when the pointer is in grab_window or one of its subwindows; otherwise, the cursor for grab_window is displayed." There is a bug where the mouse cursor unexpectedly changes to a text caret while dragging a tab from one browser window to another. This CL does not make that bug better or worse.
https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:287: int GrabPointer(XID window, bool owner_events, ::Cursor cursor) { On 2014/12/22 23:44:16, pkotwicz wrote: > The semantics of DesktopWindowTreeHostX11::SetCapture() do not allow something > like ScopedGrabPointer. > > By design, UngrabPointer() is not called if capture is transferred from one > WindowTreeHost to another. How does this transfer happen? Can it be done by transferring ownership of the scoped object? > > I have added comments in DesktopWindowTreeHostX11::SetCapture() and in > x11_util.h https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:230: int ret = ui::GrabPointer(grab_input_window_, false, None); On 2014/12/22 23:44:16, pkotwicz wrote: > Passing None for the Cursor argument seems like the right thing to do. > > According to http://tronche.com/gui/x/xlib/input/XGrabPointer.html > "If None is specified, the normal cursor for that window is displayed when the > pointer is in grab_window or one of its subwindows; otherwise, the cursor for > grab_window is displayed." This is different from what we do now though, right? So let's keep this change (i.e. using None for Cursor) as a separate CL, so revert is easier if we need to. > > There is a bug where the mouse cursor unexpectedly changes to a text caret while > dragging a tab from one browser window to another. This CL does not make that > bug better or worse.
Sadrul, can you please take another look? Please ping me if my comments are unclear. https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:287: int GrabPointer(XID window, bool owner_events, ::Cursor cursor) { The transfer occurs internally in X. The transfer is initiated via calling XGrabPointer() with a different X window. Because everything in X is asynchronous, the transfer of capture is also asynchronous. This transfer is documented in http://tronche.com/gui/x/xlib/input/XGrabPointer.html "XGrabPointer() overrides any active pointer grab by this client." No, the transfer cannot occur by transferring ownership of a scoped object. Aside: Calling XUngrabPointer() if the pointer does not have mouse grab is perfectly safe. Where do you suggest putting these methods? I am not adverse to having these methods in x11_util.cc 99% of the time it is wrong for Chrome code to call directly into X11 so we should avoid including x11_util.h if at all possible. https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:230: int ret = ui::GrabPointer(grab_input_window_, false, None); I do not understand. If X11WholeScreenMoveLoop::RunMoveLoop() does not take a cursor argument, which cursor should be passed in to GrabPointer()? WindowTreeHost::last_cursor() is an option, but it will change the behavior of Drag and Drop. Previously, we were passing in the cursor passed in to RunMoveLoop() to GrabPointer()
Ping!
https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:287: int GrabPointer(XID window, bool owner_events, ::Cursor cursor) { > By design, UngrabPointer() is not called if capture is transferred from one > WindowTreeHost to another. Where in the chrome code do we attempt to transfer the capture from one WTH to another? > Where do you suggest putting these methods? Somewhere in ui/views/widget/desktop_aura/ (e.g. perhaps x11_pointer_grab.cc|h) https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:230: int ret = ui::GrabPointer(grab_input_window_, false, None); On 2014/12/23 18:22:22, pkotwicz wrote: > I do not understand. If X11WholeScreenMoveLoop::RunMoveLoop() does not take a > cursor argument, which cursor should be passed in to GrabPointer()? > WindowTreeHost::last_cursor() is an option, but it will change the behavior of > Drag and Drop. > Previously, we were passing in the cursor passed in to RunMoveLoop() to > GrabPointer() Why not continue to do the same (i.e. why should we require two calls to ui::GrabPointer() through a call to RunMoveLoop() and another call to UpdateCursor())?
https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:287: int GrabPointer(XID window, bool owner_events, ::Cursor cursor) { We transfer capture from one WTH to another when dragging tabs https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:230: int ret = ui::GrabPointer(grab_input_window_, false, None); The problem is that if RunMoveLoop() takes a cursor argument, then we need to somehow revert to the old cursor when the move loop ends regardless of the value of |grabbed_pointer_|. In order to update the cursor of an XI2 grab, I would need to do a new grab and know the value of owner_events in the original grab.
Patchset #2 (id:80001) has been deleted
Sadrul, can you please take another look?
LGTM https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:189: NOTREACHED(); Add a comment like so: NOTREACHED() << "You must call RunMoveLoop() and check its return value first.";
https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:189: NOTREACHED(); This message would be incorrect. We reach here if the move loop is active, but the aura::Window passed in to RunMoveLoop() already had capture. RunMoveLoop() is blocking so you only get the return value when the move loop has ended...
https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:189: NOTREACHED(); On 2015/01/16 15:22:46, pkotwicz wrote: > This message would be incorrect. We reach here if the move loop is active, but > the aura::Window passed in to RunMoveLoop() already had capture. > > RunMoveLoop() is blocking so you only get the return value when the move loop > has ended... The point is, add a correct comment that explains the failure, so the developer knows what to do to fix it.
https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/821803002/diff/100001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:699: cursor_manager_->GetInitializedCursor(ui::kCursorGrabbing)); Is this going to do the right thing (i.e. would the cursor be updated to the grabbing cursor correctly before dragging)?
The CQ bit was checked by pkotwicz@chromium.org
The CQ bit was unchecked by pkotwicz@chromium.org
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821803002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:160001) has been deleted
Sadrul, can you please take another look? https://codereview.chromium.org/821803002/diff/140001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/821803002/diff/140001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:698: move_loop_->UpdateCursor( I wrote a new CL because this is wrong. The UpdateCursor() call here is performed after the move loop has ended which is wrong
Sadrul, ping?
Sadrul, ping?
You need to update the BUILD file for gn
The build on GN works now. Sadrul, can you please take another look?
lgtm
Patchset #7 (id:240001) has been deleted
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821803002/220001
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/955bab609f5c16d36ad51af6b975e33c1b6367b8 Cr-Commit-Position: refs/heads/master@{#313536} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
