Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262893002:
Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop
|
Created:
6 years, 7 months ago by varkha Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org, tdanderson+views_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemoves grab input window and extra grab and ungrab in X11WholeScreenMoveLoop.
It is made possible by the change https://codereview.chromium.org/254573002/ which allows all events to be dispatched first to X11WholeScreenMoveLoop without extra grab using OverrideDispatcher.
It also effectively reverts CL https://codereview.chromium.org/140663013/ which should be no longer necessary.
This is an alternative to https://codereview.chromium.org/265843004/
We used to have a separate drag input window that would have capture during tab drag or drag and drop. This is now replaced with the drag widget for drag and drop (which gets the capture) or by the dragged browser widget for the tab dragging (which gets the capture). Doing this allowed removing a lot of code (capture management and additional input window) and more importantly avoids changing or releasing capture during the drag.
With this refactoring X11WholeScreenMoveLoop does not grab pointer or release the grab and hence there should be less side effects when using it.
X11WholeScreenMoveLoop contained code that was specific to just drag and drop or to just tab dragging. This code got moved to X11DesktopWindowMoveClient (setting capture on the drag widget), DesktopDragDropClientAuraX11 (creating, moving and destroying the drag widget, updating cursor) and TabDragController (making sure that the capture is set to the attached tabstrip after it gets shown).
The only visible behavior change is fixing symptoms described in bug http://crbug.com/363503 - i.e. not cancelling drag when exiting one move loop before going into another one.
BUG=363503
TEST=Follow the exact steps in the bug description.
TEST=interactive_ui_tests --gtest_filter=*TabDragging*
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (review) #Patch Set 3 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (refactored) #
Total comments: 11
Patch Set 4 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (review) #Patch Set 5 : More refactoring of CreateDragImageWindow into DesktopDragDropClientAuraX11 #
Total comments: 7
Patch Set 6 : Additional SetCapture after Attach and Show in TabDragController #
Total comments: 12
Patch Set 7 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (null image case) #
Total comments: 30
Patch Set 8 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (nits and comments) #
Total comments: 5
Patch Set 9 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (nits) #
Total comments: 2
Patch Set 10 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (order in TabDragCont… #
Total comments: 2
Patch Set 11 : Removes grab input window and extra grab and ungrab in X11WholeScreenMoveLoop (no more changes in T… #
Messages
Total messages: 29 (0 generated)
pkotwicz@ what do you think of this?
I think it might be worthwhile to post a hack for review. I am going to be pessimistic and suggest that I am not a fast enough reviewer to get this CL in at a safe time for M-36. (i.e. not Friday) As a disclaimer, changing things in order to be able to drag the tab offscreen may change what needs to be done for tab dragging significantly. In particular, we need to release capture before sending the _NET_WM_MOVERESIZE event. I still think that this CL is useful though because at the very least it is simplifying the drag and drop code path. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:179: ScopedCapturer capturer(source->GetHost()); This is not really right. X11WholeScreenMoveLoop::RunMoveLoop() is called when |in_move_loop_| is true if you call set source->SetCapture() instead. It looks like for drag and drop specifically there is an expectation that capture must be lost at the beginning of the drag and drop operation. There seems to be also no expectation for capture to be restored at the end of the drag. I wonder whether it would be possible to: - Set capture to the drag widget for drag and drop only. This could be done in DesktopDragDropClientAuraX11::StartDragAndDrop() You could change things around such that |source| is the drag window when RunMoveLoop() is called as a result of drag and drop - Set capture on |source| otherwise. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:209: if (in_move_loop_) { I wonder whether we could do a grab on the window returned by aura::client::CaptureClient::GetGlobalCaptureWindow() and no-op if the result is NULL
One last thing. I am not yet sure if it is possible to use _NET_WM_MOVERESIZE for tab dragging
I've posted the other CL for review as well. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:179: ScopedCapturer capturer(source->GetHost()); On 2014/05/02 19:50:39, pkotwicz wrote: > This is not really right. X11WholeScreenMoveLoop::RunMoveLoop() is called when > |in_move_loop_| is true if you call set source->SetCapture() instead. > > It looks like for drag and drop specifically there is an expectation that > capture must be lost at the beginning of the drag and drop operation. There > seems to be also no expectation for capture to be restored at the end of the > drag. > I wonder whether it would be possible to: > - Set capture to the drag widget for drag and drop only. > This could be done in DesktopDragDropClientAuraX11::StartDragAndDrop() > You could change things around such that |source| is the drag window when > RunMoveLoop() is called as a result of drag and drop > - Set capture on |source| otherwise. I have changed it so that the capture is only kept for the tab dragging and not for drag an drop. The capture needs to be restored at the end of the tab dragging (or we need an extra click when a tab is dragged out). https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:209: if (in_move_loop_) { On 2014/05/02 19:50:39, pkotwicz wrote: > I wonder whether we could do a grab on the window returned by > aura::client::CaptureClient::GetGlobalCaptureWindow() and no-op if the result is > NULL I don't think it will work. GetGlobalCaptureWindow() returns NULL while in the drag since the drag widget loses focus.
https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:162: return ui::POST_DISPATCH_NONE; This should be: "return DispatchEvent(&xevent);" https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:179: ScopedCapturer capturer(source->GetHost()); On CrOS the capture is released as a result of views::Widget::OnMouseReleased(). Can we do the same here? This "just works" if ui::POST_DISPATCH_NONE is replaced with ui::POST_DISPATCH_PERFORM_DEFAULT when handling ButtonRelease. Would it be possible to move as much of the logic as possible which is specific to drag and drop into DesktopDragDropClientAuraX11? e.g. UpdateCursor() could potentially be moved out of this class. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:209: if (in_move_loop_) { I tried this quickly and GetGlobalCaptureWindow() seemed to do the trick. In which situation ddoes it not work for you?
If I were you I would look into crbug.com/358504 to figure out what fixing that bug will entail (i.e. what code in this class will need to be ripped out in order to fix that bug and whether any of the work done for this CL is unnecessary)
I'll look into http://crbug.com/358504 but I think as long as this CL simplifies the capture during the drag I think it will be still usefull. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:162: return ui::POST_DISPATCH_NONE; On 2014/05/05 00:09:05, pkotwicz wrote: > This should be: "return DispatchEvent(&xevent);" Done. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:179: ScopedCapturer capturer(source->GetHost()); On 2014/05/05 00:09:05, pkotwicz wrote: > On CrOS the capture is released as a result of views::Widget::OnMouseReleased(). > Can we do the same here? Yes we can! Done. > This "just works" if ui::POST_DISPATCH_NONE is replaced with > ui::POST_DISPATCH_PERFORM_DEFAULT when handling ButtonRelease. Done. > Would it be possible to move as much of the logic as possible which is specific > to drag and drop into DesktopDragDropClientAuraX11? e.g. UpdateCursor() could > potentially be moved out of this class. Done. https://codereview.chromium.org/262893002/diff/1/ui/views/widget/desktop_aura... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:209: if (in_move_loop_) { On 2014/05/05 00:09:05, pkotwicz wrote: > I tried this quickly and GetGlobalCaptureWindow() seemed to do the trick. In > which situation does it not work for you? It did not work because I was still using ScopedCapturer. Works now. Done.
Looks really good. A few comments. https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:899: // EndMoveLoop() was called, but before we return from the nested RunLoop. I do not completely understand this comment. move_loop_.in_move_loop() is set to false before the quit closure is run asynchronously. OnMoveLoopEnded() sets |source_current_window_| to None so UpdateCursor() should hopefully not be invoked after OnMoveLoopEnded() was called. Did you find a case where UpdateCursor() can be called after OnMoveLoopEnded() is invoked? https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:919: Would XChangeActivePointerGrab() be more appropriate here? http://tronche.com/gui/x/xlib/input/XChangeActivePointerGrab.html https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:68: return in_move_loop_; returning true would probably be more appropriate here https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:108: // default dispatch will release capture in Widget::OnMouseEvent. Nit: Widget::OnMouseEvent() https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:165: source = drag_widget_->GetNativeWindow(); 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|.
https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:899: // EndMoveLoop() was called, but before we return from the nested RunLoop. On 2014/05/05 18:14:37, pkotwicz wrote: > I do not completely understand this comment. > move_loop_.in_move_loop() is set to false before the quit closure is run > asynchronously. > > OnMoveLoopEnded() sets |source_current_window_| to None so UpdateCursor() should > hopefully not be invoked after OnMoveLoopEnded() was called. Did you find a case > where UpdateCursor() can be called after OnMoveLoopEnded() is invoked? > Yes, with the source_current_window_ set to None in OnMoveLoopEnded() I don't expect this to be an issue. I've removed the check. https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:919: On 2014/05/05 18:14:37, pkotwicz wrote: > Would XChangeActivePointerGrab() be more appropriate here? > http://tronche.com/gui/x/xlib/input/XChangeActivePointerGrab.html Done. This also eliminates the need to get the current capture since this call is a non-nop only when "pointer is actively grabbed by the client". https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:68: return in_move_loop_; On 2014/05/05 18:14:37, pkotwicz wrote: > returning true would probably be more appropriate here Done. https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:108: // default dispatch will release capture in Widget::OnMouseEvent. On 2014/05/05 18:14:37, pkotwicz wrote: > Nit: Widget::OnMouseEvent() Done. https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:165: source = drag_widget_->GetNativeWindow(); On 2014/05/05 18:14:37, pkotwicz wrote: > 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|. I'll try that. I have uploaded a patch with the rest addressed in the meanwhile.
https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:165: source = drag_widget_->GetNativeWindow(); On 2014/05/05 18:14:37, pkotwicz wrote: > 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|. Done.
https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc:51: host_->SetCapture(); Did you end up figuring out why this is necessary?
https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc:51: host_->SetCapture(); On 2014/05/07 02:22:24, pkotwicz wrote: > Did you end up figuring out why this is necessary? No, but since this is largely a refactoring CL this code just repeats existing flow. My guess is that something necessary happens when capture is released and restored.
Note that using the override dispatcher to receive all events instead of doing a grab will only work for X11 windows managed by the chrome-browser process. https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc:54: source->GetHost()->ReleaseCapture(); host_->ReleaseCapture()? (or is it possible for |this| to be destroyed from the run-loop?) https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:86: // default dispatch will release capture in Widget::OnMouseEvent(). I assume you mean the dispatcher for the browser etc. windows in chrome? For those windows, we request XI2 events. So we should not be receiving ButtonRelease events?
https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc:51: host_->SetCapture(); On 2014/05/07 02:22:24, pkotwicz wrote: > Did you end up figuring out why this is necessary? Yes, since the TabDragController releases capture just before running move loop. I should be however setting capture to the source window and not just the host. https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_window_move_client.cc:54: source->GetHost()->ReleaseCapture(); On 2014/05/07 14:18:16, sadrul wrote: > host_->ReleaseCapture()? (or is it possible for |this| to be destroyed from the > run-loop?) host_ gets reset in OnMoveLoopEnded() in this class, but we don't really need this release anyway if the capture is properly set and released in TabDragController and in this class. This makes the logic same as in NativeWidgetAura::RunMoveLoop (i.e. setting capture just before the move loop runs). https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/120001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:86: // default dispatch will release capture in Widget::OnMouseEvent(). On 2014/05/07 14:18:16, sadrul wrote: > I assume you mean the dispatcher for the browser etc. windows in chrome? For > those windows, we request XI2 events. So we should not be receiving > ButtonRelease events? Hopefully this is a bit clearer. We get here re-enter here from handling XI2 event and return the value returned which in the end ends up calling the Widget::OnMouseEvent() and releases the capture. This allows not having an additional ReleaseCapture in X11DesktopWindowMoveClient.
https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:643: // track all cursor movement and reroute events to a specific handler. Nits: - The comment above should be right above the call to RunMoveLoop() - Does SetDragImage() need to be its own function? It just does assignment of the two parameters which are passed to it. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:647: CreateDragImageWindow(); We may need to move |grab_input_window_| to this class too. We rely on |drag_widget_| having capture for the duration of the drag. However, |drag_widget_| may not be created. e.g. |drag_widget_| is currently not created if a fully transparent image from a website is dragged. I am unsure when |drag_image_| can have 0 size. I am sorry for giving bad advice :( https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:890: // Update the pointer with the updated cursor if a grab is active. Nit: updated -> new https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:44: class DesktopNativeCursorManager; Nit: forward declare views::Widget https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:92: EndMoveLoop(); Nit: It would be helpful to comment as to when capture is released in this case. (I believe that it is Widget::OnMouseEvent() too) https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:44: // If the pointer-grab fails, or the move-loop is canceled by the user (e.g. Nit: Can you please update this comment?
PTAL. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:643: // track all cursor movement and reroute events to a specific handler. On 2014/05/08 00:03:49, pkotwicz wrote: > Nits: > - The comment above should be right above the call to RunMoveLoop() Done. > - Does SetDragImage() need to be its own function? It just does assignment of > the two parameters which are passed to it. I think it is clearer to isolate it in a method, primarily because it has a piece of logic about keeping Y offset zero. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:647: CreateDragImageWindow(); On 2014/05/08 00:03:49, pkotwicz wrote: > We may need to move |grab_input_window_| to this class too. We rely on > |drag_widget_| having capture for the duration of the drag. However, > |drag_widget_| may not be created. > e.g. |drag_widget_| is currently not created if a fully transparent image from a > website is dragged. I am unsure when |drag_image_| can have 0 size. > > I am sorry for giving bad advice :( We talked about this offline. I've set up the drag widget to be off screen in this case. This seems simpler and prevents cases of reentrant move loops. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:890: // Update the pointer with the updated cursor if a grab is active. On 2014/05/08 00:03:49, pkotwicz wrote: > Nit: updated -> new Done. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:44: class DesktopNativeCursorManager; On 2014/05/08 00:03:49, pkotwicz wrote: > Nit: forward declare views::Widget Done (is also forward declared in ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h which is why this still compiled). https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:92: EndMoveLoop(); On 2014/05/08 00:03:49, pkotwicz wrote: > Nit: It would be helpful to comment as to when capture is released in this case. > (I believe that it is Widget::OnMouseEvent() too) Done. This was actually more complicated and educational. https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h (right): https://codereview.chromium.org/262893002/diff/160001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:44: // If the pointer-grab fails, or the move-loop is canceled by the user (e.g. On 2014/05/08 00:03:49, pkotwicz wrote: > Nit: Can you please update this comment? Done.
I think you can get sadrul@ to do the next review. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:43: const gfx::Point kNullDragWidgetLocation = gfx::Point(-100, -100); I do not know if there is much precedent for non primitive constants. Can you double check? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:45: bool CheckIfIconValid(const gfx::ImageSkia& drag_image) { Is CheckIfIconValid() still needed in a world where we always construct a |drag_widget_|? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:763: if (drag_widget_.get()) { If we intentionally moved |drag_widget_| offscreen, won't this put it back onscreen? I suspect we want to check |drag_image_| or perhaps have a dedicated bool member variable to keep track of whether we want to keep |drag_widget_| offscreen https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1088: /*if (drag_image_.isNull() || !CheckIfIconValid(drag_image_))*/ { Remove the if block entirely. It is not needed. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1107: gfx::Point location = drag_image_.isNull() ? I will defer to sadrul@/erg@ as to whether they like moving the widget offscreen. The alternative would be to make X11ScopedCapture smarter. i.e. X11ScopedCapture would track which XID has capture This might be slightly nicer: if (drag_image_.isNull()) { params.bounds = gfx::Rect(kNullDragWidgetLocation, kNullDragWidgetSize); } else { gfx::Point cursor_screen_point = gfx::Screen::GetNativeScreen()->GetCursorScreenPoint(); params.bounds = gfx::Rect( gfx::ToFlooredPoint(cursor_screen_point - drag_offset_), drag_image_.size()); } https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1129: widget->GetNativeWindow()->layer()->SetFillsBoundsOpaquely(false); We should have a comment as to why we set capture here. Setting capture is important enough that it should probably occur outside of this function. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:259: // used during the drag. This comment is obsolete. Can you please update it? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:86: // default will end up releasing capture in Widget::OnMouseEvent(). Nit: default -> Default https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:92: EndMoveLoop(); Maybe comment that how capture is released is different based on what type of move loop we are in (drag and drop, tab dragging etc) Thanks for looking into the specifics. This is educational. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:148: ui::PlatformEventSource::GetInstance()->OverrideDispatcher(this); Nit: might as well fix the alignment here https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:14: #include "ui/gfx/vector2d_f.h" Nit: The include for native_widget_types.h and vector2d_f.h are unnecessary? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:17: typedef struct _XDisplay XDisplay; The typedef is unnecessary? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:29: class Widget; The Window and Widget forward declarations are unnecessary https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:32: // dragging. Nit: Update this comment https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:43: // successfully. Returns false if the pointer-grab fails or the move-loop is Nit: Remove the portion about the pointer-grab failing. That portion of the comment is stale.
sadrul@, can you please take another look? https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:43: const gfx::Point kNullDragWidgetLocation = gfx::Point(-100, -100); On 2014/05/12 16:04:29, pkotwicz wrote: > I do not know if there is much precedent for non primitive constants. Can you > double check? Good catch. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... bans class objects with static storage duration, including global variables, static variables, static class member variables, and function static variables; they must be Plain Old Data. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:45: bool CheckIfIconValid(const gfx::ImageSkia& drag_image) { On 2014/05/12 16:04:29, pkotwicz wrote: > Is CheckIfIconValid() still needed in a world where we always construct a > |drag_widget_|? Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:763: if (drag_widget_.get()) { On 2014/05/12 16:04:29, pkotwicz wrote: > If we intentionally moved |drag_widget_| offscreen, won't this put it back > onscreen? I suspect we want to check |drag_image_| or perhaps have a dedicated > bool member variable to keep track of whether we want to keep |drag_widget_| > offscreen I am using drag_image_.isNull() for that (and resetting drag_image_ in case when the |drag_widget_| is offscreen. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1088: /*if (drag_image_.isNull() || !CheckIfIconValid(drag_image_))*/ { On 2014/05/12 16:04:29, pkotwicz wrote: > Remove the if block entirely. It is not needed. Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1107: gfx::Point location = drag_image_.isNull() ? On 2014/05/12 16:04:29, pkotwicz wrote: > I will defer to sadrul@/erg@ as to whether they like moving the widget > offscreen. The alternative would be to make X11ScopedCapture smarter. i.e. > X11ScopedCapture would track which XID has capture > > This might be slightly nicer: > if (drag_image_.isNull()) { > params.bounds = gfx::Rect(kNullDragWidgetLocation, kNullDragWidgetSize); > } else { > gfx::Point cursor_screen_point = > gfx::Screen::GetNativeScreen()->GetCursorScreenPoint(); > params.bounds = gfx::Rect( > gfx::ToFlooredPoint(cursor_screen_point - drag_offset_), > drag_image_.size()); > } Right. Keeping the drag widget offscreen matches the behavior of the code prior to this CL where an input window was created offscreen. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1129: widget->GetNativeWindow()->layer()->SetFillsBoundsOpaquely(false); On 2014/05/12 16:04:29, pkotwicz wrote: > We should have a comment as to why we set capture here. Setting capture is > important enough that it should probably occur outside of this function. Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:259: // used during the drag. On 2014/05/12 16:04:29, pkotwicz wrote: > This comment is obsolete. Can you please update it? Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:86: // default will end up releasing capture in Widget::OnMouseEvent(). On 2014/05/12 16:04:29, pkotwicz wrote: > Nit: default -> Default Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:92: EndMoveLoop(); On 2014/05/12 16:04:29, pkotwicz wrote: > Maybe comment that how capture is released is different based on what type of > move loop we are in (drag and drop, tab dragging etc) > > Thanks for looking into the specifics. This is educational. Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:148: ui::PlatformEventSource::GetInstance()->OverrideDispatcher(this); On 2014/05/12 16:04:29, pkotwicz wrote: > Nit: might as well fix the alignment here Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h (right): https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:14: #include "ui/gfx/vector2d_f.h" On 2014/05/12 16:04:29, pkotwicz wrote: > Nit: The include for native_widget_types.h and vector2d_f.h are unnecessary? Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:17: typedef struct _XDisplay XDisplay; On 2014/05/12 16:04:29, pkotwicz wrote: > The typedef is unnecessary? Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:29: class Widget; On 2014/05/12 16:04:29, pkotwicz wrote: > The Window and Widget forward declarations are unnecessary Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:32: // dragging. On 2014/05/12 16:04:29, pkotwicz wrote: > Nit: Update this comment Done. https://codereview.chromium.org/262893002/diff/180001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_whole_screen_move_loop.h:43: // successfully. Returns false if the pointer-grab fails or the move-loop is On 2014/05/12 16:04:29, pkotwicz wrote: > Nit: Remove the portion about the pointer-grab failing. That portion of the > comment is stale. Done.
sadrul@, ping?
Can you explain more clearly what's happening in this CL? I see that some things are moving out of X11WholeScreenMoveLoop, into DesktopDragDropClientAuraX11, but it's not clear what the changes in behaviour are. I am also nervous about continuing to fix bugs without having test coverage, because drag-and-drop code here has been considerably brittle. +sky@ for the change in tab-strip. I don't know enough about that to know if this can break the various edge cases.
I have added a few points about motivation for this CL (other than fixing the bug).
Explaining the change in TabDragController::DetachIntoNewBrowserAndRunMoveLoop(). https://codereview.chromium.org/262893002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/262893002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1181: attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_); This makes sure that capture is really where we think it is after calling Attach(). Attach() would normally set capture on attached widget but with Aura fails to do so because of IsVisible() check in Window::SetCapture(). This was wrong before but worked because of additional capture in X11WholeScreenMoveLoop. Simplifying X11WholeScreenMoveLoop exposed this.
views changes lgtm https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1082: params.opacity = Widget::InitParams::OPAQUE_WINDOW; Should this be TRANSLUCENT_WINDOW? https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:179: void SetDragImage(const gfx::ImageSkia& image, gfx::Vector2dF offset); const gfx::Vector2dF&
https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:1082: params.opacity = Widget::InitParams::OPAQUE_WINDOW; On 2014/05/23 14:39:53, sadrul wrote: > Should this be TRANSLUCENT_WINDOW? Done. https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/262893002/diff/200001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:179: void SetDragImage(const gfx::ImageSkia& image, gfx::Vector2dF offset); On 2014/05/23 14:39:53, sadrul wrote: > const gfx::Vector2dF& Done.
+sky@, for OWNERS in chrome/browser/ui/views/tabs/tab_drag_controller.cc
https://codereview.chromium.org/262893002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/262893002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1181: attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_); Did you verify this doesn't cause any issues on windows? I would feel marginally better if this was moved down two lines (after SetVisibilityChangedAnimationsEnabled, but especially the RemoveObserver bit).
I'll verify (takes time to sync and build on Windows). I have changed the order in the meanwhile and checked that this does not regress on Linux or Chrome OS. I have also restored CheckIfIconValid (which I previously thought was not useful anymore). Well, removing it regressed dragging icons in chrome://apps page by showing the unnecessary drag widget. https://codereview.chromium.org/262893002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/262893002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1181: attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_); On 2014/05/23 15:56:13, sky wrote: > Did you verify this doesn't cause any issues on windows? I would feel marginally > better if this was moved down two lines (after > SetVisibilityChangedAnimationsEnabled, but especially the RemoveObserver bit). Done.
https://codereview.chromium.org/262893002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/262893002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1183: attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_); Couple of questions. Why is this needed here since Attach should set capture? Since this seems X11 specific, can it be moved to DesktopWindowTreeHostX11::RunMoveLoop?
I've reverted the part in TabDragController, making the changes linux-only but unfortunately this CL regresses on Linux in a scenario when a tab is repeatedly dragged out and back in. In this scenario previous code path would do multiple captures and releases (more than in the new) and has more chance to cancel the drag when a focus loss notification (which releases the capture and kills the TabDragController) arrives asynchronously after the new drag has yet to get started (but the mouse button has not been released). I am still thinking how to handle this so this is not yet ready. https://codereview.chromium.org/262893002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/262893002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1183: attached_tabstrip_->GetWidget()->SetCapture(attached_tabstrip_); On 2014/05/27 13:27:04, sky wrote: > Couple of questions. Why is this needed here since Attach should set capture? I had this comment here before, but it is higher up the review chain: This makes sure that capture is really where we think it is after calling Attach(). Attach() would normally set capture on attached widget but with Aura fails to do so because of IsVisible() check in Window::SetCapture(). This was wrong before but worked because of additional capture in X11WholeScreenMoveLoop. Simplifying X11WholeScreenMoveLoop exposed this. It worked on other Aura platforms because NativeWidgetAura::RunMoveLoop calls SetCapture. > Since this seems X11 specific, can it be moved to > DesktopWindowTreeHostX11::RunMoveLoop? There is already SetCapture in X11DesktopWindowMoveClient::RunMoveLoop which is called from DesktopWindowTreeHostX11::RunMoveLoop so this should not be necessary and I reverted this part of the change. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
