Linux Aura: Use managed tab dragging when possible (Reland)

Linux Aura: Use managed tab dragging when possible (Reland)

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119, 617806, 635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}

[Tom (Use chromium acct), 2016-10-12 00:47:51]

Description was changed from

==========
Linux Aura: Make tab dragging great again

BUG=
==========

to

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org,pkasting@chromium.org
==========

[Tom (Use chromium acct), 2016-10-12 00:54:12]

Description was changed from

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org,pkasting@chromium.org
==========

to

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org
==========

[Tom (Use chromium acct), 2016-10-12 00:54:12]

thomasanderson@google.com changed reviewers:
+ erg@chromium.org, sky@chromium.org

[Tom (Use chromium acct), 2016-10-12 00:55:19]

erg + sky PTAL

erg:
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
  ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc
  ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.h
  ui/views/widget/desktop_aura/x11_window_event_filter.cc
  ui/views/widget/desktop_aura/x11_window_event_filter.h
sky:
  chrome/browser/ui/views/tabs/tab_drag_controller.cc
  ui/base/x/x11_util.cc
  ui/base/x/x11_util.h
  ui/views/BUILD.gn
  ui/wm/public/window_move_client.h

https://codereview.chromium.org/2410773002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc
(right):

https://codereview.chromium.org/2410773002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:76:
aura::client::WindowMoveResult X11DesktopWindowMoveClientManaged::RunMoveLoop(
erg: There's duplicated code between this class and X11WholeScreenMoveLoop.  I'm
not sure what the best way to partition this is.  Maybe add X11MoveLoopManaged :
public X11MoveLoop and then pass an X11MoveLoop to X11DesktopWindowMoveClient?

https://codereview.chromium.org/2410773002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:85:
window->ReleaseCapture();
Without this ReleaseCapture, aura stops receiving events after the drag ends
until you click in the window.  But I'm not sure if this is right in 100% of
cases.

https://codereview.chromium.org/2410773002/diff/20001/ui/wm/public/window_mov...
File ui/wm/public/window_move_client.h (left):

https://codereview.chromium.org/2410773002/diff/20001/ui/wm/public/window_mov...
ui/wm/public/window_move_client.h:44: protected:
I needed to do this because DesktopWindowTreeHostX11 now keeps a unique_pointer
to a WindowMoveClient

[sky, 2016-10-12 15:58:32]

sky@chromium.org changed reviewers:
+ derat@chromium.org

[sky, 2016-10-12 15:58:33]

+derat for ui/base/x/*

https://codereview.chromium.org/2410773002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/2410773002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:420: drag_point_in_screen =
start_point_in_screen_;
I'm confused by this. If you use the original starting location, then don't you
miss the kMinimumDragDistance?

Also, in this context 'hack' is rather negative and the above code isn't a hack.
It's there to ensure specific behavior.

[Daniel Erat, 2016-10-12 16:13:10]

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.cc#n...
ui/base/x/x11_util.cc:515: event.xclient.data.l[4] = 1;
nit: add something like "// Requested by normal application" at the end of this
line

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:40: enum class NetWmMoveResize {
nit: add a comment saying that these must match the hardcoded values from ewmh's
_NET_WM_MOVERESIZE spec, and consider explicitly numbering them (or at least
explicitly setting TOPLEFT to 0)

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:103: // Clients MUST release all grabs prior to calling.
maybe document that this isn't implemented by all window managers (perhaps
explicitly calling out tiling ones) and that the caller should check for support
first.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:104: UI_BASE_X_EXPORT void MoveResizeWindowManaged(XID
window,
nit: change "WindowManaged" to "ManagedWindow"?

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:105: gfx::Point origin,
per the spec, this is actually the pointer position relative to the root
window's origin at the start of the operation, right? if that's correct, please
name it something like |event_root| and document it in the comment.

[Tom (Use chromium acct), 2016-10-12 18:08:28]

https://codereview.chromium.org/2410773002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/2410773002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:420: drag_point_in_screen =
start_point_in_screen_;
On 2016/10/12 15:58:33, sky wrote:
> I'm confused by this. If you use the original starting location, then don't
you
> miss the kMinimumDragDistance?
> 

No, because I removed the widget->SetBounds above.

The reason I needed to do this is the SetBounds eventually results in a request
to move the window sent to the window manager.  The WM would then snap the
window to within the screen bounds, and we would then request the WM move the
window in RunMoveLoop with a stale starting point.

> Also, in this context 'hack' is rather negative and the above code isn't a
hack.
> It's there to ensure specific behavior.

Sorry, removed.  I said that because that code was added to overcome a
limitation only on Windows platforms.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.cc#n...
ui/base/x/x11_util.cc:515: event.xclient.data.l[4] = 1;
On 2016/10/12 16:13:10, Daniel Erat wrote:
> nit: add something like "// Requested by normal application" at the end of
this
> line

Done.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:40: enum class NetWmMoveResize {
On 2016/10/12 16:13:10, Daniel Erat wrote:
> nit: add a comment saying that these must match the hardcoded values from
ewmh's
> _NET_WM_MOVERESIZE spec, and consider explicitly numbering them (or at least
> explicitly setting TOPLEFT to 0)

Done.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:103: // Clients MUST release all grabs prior to calling.
On 2016/10/12 16:13:10, Daniel Erat wrote:
> maybe document that this isn't implemented by all window managers (perhaps
> explicitly calling out tiling ones) and that the caller should check for
support
> first.

Done.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:104: UI_BASE_X_EXPORT void MoveResizeWindowManaged(XID
window,
On 2016/10/12 16:13:10, Daniel Erat wrote:
> nit: change "WindowManaged" to "ManagedWindow"?

Done.

https://codereview.chromium.org/2410773002/diff/20001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:105: gfx::Point origin,
On 2016/10/12 16:13:10, Daniel Erat wrote:
> per the spec, this is actually the pointer position relative to the root
> window's origin at the start of the operation, right? if that's correct,
please
> name it something like |event_root| and document it in the comment.

Done.

[sky, 2016-10-12 19:28:12]

Ok, makes sense. LGTM

[Daniel Erat, 2016-10-12 19:48:02]

thanks, lgtm for ui/base/x11

[Elliot Glaysher, 2016-10-12 19:51:29]

lgtm

[Tom (Use chromium acct), 2016-10-12 20:38:08]

erg@ I left some comments in x11_desktop_window_move_client_managed.cc
Can you take a look at those?

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc
(right):

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:70:
return ui::POST_DISPATCH_PERFORM_DEFAULT;
I wasn't sure whether to use POST_DISPATCH_PERFORM_DEFAULT or POST_DISPATCH_NONE
here.  Do you know which to use?

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:85:
window->ReleaseCapture();
If I don't ReleaseCapture() here, when the drag ends, I stop receiving mouse
events until I click in the window, but only on some window managers (doesn't
happen on Compiz, happens on Metacity).

I'm not confident that this is right.  Can you verify?

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:97:
in_move_loop_ = true;
There's duplicate code between here and x11_whole_screen_move_loop.  Should we
try to extract this into a base class?

[Elliot Glaysher, 2016-10-12 20:48:06]

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc
(right):

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:70:
return ui::POST_DISPATCH_PERFORM_DEFAULT;
On 2016/10/12 20:38:08, Tom Anderson wrote:
> I wasn't sure whether to use POST_DISPATCH_PERFORM_DEFAULT or
POST_DISPATCH_NONE
> here.  Do you know which to use?

I assume POST_DISPATCH_NONE since you're handling the event; the button release
corresponds as a concept has been handled and you don't want the default
dispatcher to also interpret it.

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:85:
window->ReleaseCapture();
On 2016/10/12 20:38:08, Tom Anderson wrote:
> If I don't ReleaseCapture() here, when the drag ends, I stop receiving mouse
> events until I click in the window, but only on some window managers (doesn't
> happen on Compiz, happens on Metacity).
> 
> I'm not confident that this is right.  Can you verify?

I vaguely remember doing the same thing for the same reasons. (And a quick check
shows that we release the current global capture window in the aurax11 drag and
drop code before we start our nested move loop.)

(Just because I did it doesn't mean it's correct to do. But since this is X11,
what does correctness really mean, anyway?)

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:97:
in_move_loop_ = true;
On 2016/10/12 20:38:08, Tom Anderson wrote:
> There's duplicate code between here and x11_whole_screen_move_loop.  Should we
> try to extract this into a base class?

Eyeballing it, it's less than 20 lines and is only duplicated once. Extract a
base class if you have to make a third implementation, otherwise don't bother.

[Tom (Use chromium acct), 2016-10-12 22:35:11]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[Tom (Use chromium acct), 2016-10-12 22:35:18]

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc
(right):

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:70:
return ui::POST_DISPATCH_PERFORM_DEFAULT;
On 2016/10/12 20:48:06, Elliot Glaysher wrote:
> On 2016/10/12 20:38:08, Tom Anderson wrote:
> > I wasn't sure whether to use POST_DISPATCH_PERFORM_DEFAULT or
> POST_DISPATCH_NONE
> > here.  Do you know which to use?
> 
> I assume POST_DISPATCH_NONE since you're handling the event; the button
release
> corresponds as a concept has been handled and you don't want the default
> dispatcher to also interpret it.

Done.

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:85:
window->ReleaseCapture();
On 2016/10/12 20:48:06, Elliot Glaysher wrote:
> On 2016/10/12 20:38:08, Tom Anderson wrote:
> > If I don't ReleaseCapture() here, when the drag ends, I stop receiving mouse
> > events until I click in the window, but only on some window managers
(doesn't
> > happen on Compiz, happens on Metacity).
> > 
> > I'm not confident that this is right.  Can you verify?
> 
> I vaguely remember doing the same thing for the same reasons. (And a quick
check
> shows that we release the current global capture window in the aurax11 drag
and
> drop code before we start our nested move loop.)
> 
> (Just because I did it doesn't mean it's correct to do. But since this is X11,
> what does correctness really mean, anyway?)

Acknowledged.

https://codereview.chromium.org/2410773002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_window_move_client_managed.cc:97:
in_move_loop_ = true;
On 2016/10/12 20:48:06, Elliot Glaysher wrote:
> On 2016/10/12 20:38:08, Tom Anderson wrote:
> > There's duplicate code between here and x11_whole_screen_move_loop.  Should
we
> > try to extract this into a base class?
> 
> Eyeballing it, it's less than 20 lines and is only duplicated once. Extract a
> base class if you have to make a third implementation, otherwise don't bother.

Acknowledged.

[commit-bot: I haz the power, 2016-10-12 22:35:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 23:29:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 23:29:22]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-10-12 23:41:02]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-10-12 23:41:02]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
derat@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2410773002/#ps60001
(title: "use POST_DISPATCH_NONE")

[commit-bot: I haz the power, 2016-10-12 23:41:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 23:47:59]

Description was changed from

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org
==========

to

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-10-12 23:48:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-12 23:49:45]

Description was changed from

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org
==========

to

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}
==========

[commit-bot: I haz the power, 2016-10-12 23:49:45]

Patchset 4 (id:??) landed as
https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}

[Tom Anderson, 2016-11-01 19:32:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2467053002/ by thomasanderson@chromium.org.

The reason for reverting is: Suspecting this is causing 661248, so
conservatively reverting..

[Tom (Use chromium acct), 2016-11-04 21:26:41]

Description was changed from

==========
Linux Aura: Make tab dragging great again

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}
==========

to

==========
Linux Aura: Make tab dragging great again (Reland)

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}
==========

[Tom (Use chromium acct), 2016-11-11 02:03:04]

Description was changed from

==========
Linux Aura: Make tab dragging great again (Reland)

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}
==========

to

==========
Linux Aura: Use managed tab dragging when possible (Reland)

This CL uses _NET_WM_MOVERESIZE to handle tab dragging for managed windows on
select window managers.

This fixes various issues I've seen with tab dragging including:
* Window movement is no longer constrained by screen bounds
* Windows animating in during tab dragging is minimized
* Tab dragging is improved on tiling WMs
* Better integration with desktop environment (you can now drag to the top of
  the screen to maximize if the DE supports it, or drag left/right between
  workspaces)

BUG=416119,617806,635195
R=erg@chromium.org,sky@chromium.org

Committed: https://crrev.com/fbb8975f1693f4fb33d54f62bbef19a70a4bc724
Cr-Commit-Position: refs/heads/master@{#424909}
==========