Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
749063003:
Fix grabbing capture when the mouse is pressed on Desktop Linux. (Closed)
DescriptionFix grabbing capture when the mouse is pressed on Desktop Linux.
The X specification allows the OS to grab the mouse when a mouse button is
pressed. The OS grab was causing XGrabPointer() in
DesktopWindowTreeHostX11::SetCapture() to fail.
BUG=426380
TEST=Manual
Committed: https://crrev.com/890bd4742e41d386885a3bb3f5f72bac3065659a
Cr-Commit-Position: refs/heads/master@{#314501}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Messages
Total messages: 31 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
pkotwicz@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, can you please take a look? I tried always ungrabbing the pointer and then grabbing it (as opposed to ungrabbing the pointer when the grab fails) and did not notice a difference in how often tab dragging succeeds. However, I think that "Mouse grab never being released during a tab drag" makes tab dragging easier to think about. I went back and looked at https://codereview.chromium.org/455553003. That CL is a significant improvement in how often a tab drag succeeds. That being said, from a quick glance I was unable to determine why. It is not solely the change to tab_drag_controller.cc
https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:964: if (success == AlreadyGrabbed) { Can you link to some doc here? man XGrabPointer claims AlreadyGrabbed is returned when some other client has a grab, and it doesn't sound like that's what you are saying in the comment below.
https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:964: if (success == AlreadyGrabbed) { The docs are not great. However man XUngrabPointer mentions: " The XUngrabPointer function releases the pointer and any queued events if this client has actively grabbed the pointer from XGrabPointer, XGrabButton, or from a normal button press." I suspect that "if the pointer is grabbed from a normal button press", this counts as a different client. Otherwise, XGrabPointer should not fail according to the spec. In particular the spec mentions: "XGrabPointer overrides any active pointer grab by this client."
Sadrul, ping?
https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/749063003/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:964: if (success == AlreadyGrabbed) { On 2014/11/24 16:07:41, pkotwicz wrote: > The docs are not great. However man XUngrabPointer mentions: " The > XUngrabPointer function releases the pointer and any queued events > if this client has actively grabbed the pointer from XGrabPointer, XGrabButton, > or from a normal button press." > > I suspect that "if the pointer is grabbed from a normal button press", this > counts as a different client. Otherwise, XGrabPointer should not fail according > to the spec. In particular the spec mentions: "XGrabPointer overrides any active > pointer grab by this client." Do we have example of any other clients doing similar things? (from a quick glance, GTK+ doesn't seem to do this)
I looked into this some more and the problem is that XI2 capture is not compatible with XI1 capture. So I need to figure out how to do XI2 capture. Commenting out XISetMask(mask, XI_ButtonPress) makes the bug go away.
Patchset #2 (id:60001) has been deleted
Sadrul, can you please take another look? https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:313: } else { Should I try XGrabPointer() if XIGrabDevice() fails?
Ping!
Sadrul, Ping?
https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:301: evmask.deviceid = XIAllDevices; Should you use master_pointers[i] here? https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:313: } else { On 2014/12/01 22:42:48, pkotwicz wrote: > Should I try XGrabPointer() if XIGrabDevice() fails? Sure. Let's try that. Also, remove else. https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.h#new... ui/base/x/x11_util.h:52: UI_BASE_EXPORT void UngrabPointer(); Would it be possible to have something like ScopedGrabPointer? So that the caller of GrabPointer() receives a ScopedGrabPointer object, and resets it to ungrab. This makes sure some code doesn't accidentally forget to ungrab, and doesn't try to ungrab when it doesn't have a grab. https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... ui/events/devices/x11/device_data_manager_x11.cc:243: // We currently handle only slave, non-keyboard devices Can you update |master_pointers_| before this instead of a separate loop? https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... ui/events/devices/x11/device_data_manager_x11.h:281: // List of the master pointer devices. Used for grabbing mouse capture. Remove the second sentence. (let's not require future users of this to have to update this comment). https://codereview.chromium.org/749063003/diff/80001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:252: int ret = ui::GrabPointer(grab_input_window_, false, None); Should you send cursor.platform() for cursor?
https://codereview.chromium.org/749063003/diff/80001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_move_loop.h (right): https://codereview.chromium.org/749063003/diff/80001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_move_loop.h:22: virtual bool RunMoveLoop(aura::Window* window) = 0; Would it be possible to split this part of the change into a separate CL?
One more nit to avoid another round-trip: let's have this ScopedGrabPointer internally inside views, instead of in ui/base/ (the rest of the code shouldn't need to use this, and so this shouldn't need to be exposed in the API)
Sadrul, can you please take another look? As requested, I have split this CL into two. Part 1 which does most of the refactoring is at https://codereview.chromium.org/821803002/ https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:301: evmask.deviceid = XIAllDevices; It seems to work either way. I haven't seen any documentation as to which way is "right". https://codereview.chromium.org/749063003/diff/80001/ui/base/x/x11_util.cc#ne... ui/base/x/x11_util.cc:313: } else { On 2014/12/22 21:08:59, sadrul wrote: > On 2014/12/01 22:42:48, pkotwicz wrote: > > Should I try XGrabPointer() if XIGrabDevice() fails? > > Sure. Let's try that. > > Also, remove else. Done. https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... ui/events/devices/x11/device_data_manager_x11.cc:243: // We currently handle only slave, non-keyboard devices On 2014/12/22 21:08:59, sadrul wrote: > Can you update |master_pointers_| before this instead of a separate loop? Done. https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... File ui/events/devices/x11/device_data_manager_x11.h (right): https://codereview.chromium.org/749063003/diff/80001/ui/events/devices/x11/de... ui/events/devices/x11/device_data_manager_x11.h:281: // List of the master pointer devices. Used for grabbing mouse capture. On 2014/12/22 21:08:59, sadrul wrote: > Remove the second sentence. (let's not require future users of this to have to > update this comment). Done.
On 2014/12/23 00:29:49, pkotwicz wrote: > Sadrul, can you please take another look? As requested, I have split this CL > into two. Part 1 which does most of the refactoring is at > https://codereview.chromium.org/821803002/ I didn't really mean this split. I was suggesting splitting out the change where X11MoveLoop::RunMoveLoop() doesn't take a cursor. Also, we shouldn't expose this API in ui/base/, and keep it in views.
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Sadrul, can you please take another look now that part 1 (https://codereview.chromium.org/821803002/) has landed?
Sadrul, Ping?
https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_pointer_grab.cc (right): https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:32: for (auto master_pointer : master_pointers) { int https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:41: evmask.mask = mask; Can you move initialization of |mask|/|evmask| out of the loop, and just change |deviceid| inside before calling XIGrabDevice()? https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:78: for (auto master_pointer : master_pointers) int https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:79: XIUngrabDevice(gfx::GetXDisplay(), master_pointer, CurrentTime); What happens if GrabPointer() ended up calling XGrabPointer? Would calling XIUgrabDevice() do the right thing in that case? Or would it be necessary to call XUngragPointer() instead?
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
Sadrul, can you please take another look? https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_pointer_grab.cc (right): https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:32: for (auto master_pointer : master_pointers) { On 2015/02/03 01:22:49, sadrul wrote: > int Done. https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:41: evmask.mask = mask; On 2015/02/03 01:22:48, sadrul wrote: > Can you move initialization of |mask|/|evmask| out of the loop, and just change > |deviceid| inside before calling XIGrabDevice()? Done. https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:78: for (auto master_pointer : master_pointers) On 2015/02/03 01:22:49, sadrul wrote: > int Done. https://codereview.chromium.org/749063003/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_pointer_grab.cc:79: XIUngrabDevice(gfx::GetXDisplay(), master_pointer, CurrentTime); Thanks for catching this. We need to call XUngrabPointer() regardless of whether XI2 is available.
LGTM
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/749063003/220001
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/890bd4742e41d386885a3bb3f5f72bac3065659a Cr-Commit-Position: refs/heads/master@{#314501} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
