Desktop Aura: Allow tab drags out of window.

Desktop Aura: Allow tab drags out of window.

You can now drag tabs in and out of windows in linux_aura builds. This patch has a lot of moving parts and caveats though.

Moving parts in the patch:
- Added system location to aura::Events.
- Forwarding move events from RootWindowHostLinux to the TabDragController through an observer.
- MessagePumpAuraX11 now can block waiting for a window to be mapped. If we don't do this, we can't perform a grab on the new window.
- The drag offset is threaded from TabDragController through the WindowMoveController interface. (There's no other non-racey way of getting this data locally from X.)
- RootWindowHostLinux now has working Show()/Hide().

TODOs:
- After releasing a new window, the window looks focused, but isn't. You have to click on it for it to receive input focus.

BUG=133059
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152180

[Elliot Glaysher, 2012-08-14 21:49:53]

This patch is big; and I'm going to ask for some advice in the comments.

sky: tab drag controller and related
derat: x11 stuff
ben: rest; event stuff

https://chromiumcodereview.appspot.com/10828133/diff/5044/chrome/browser/ui/v...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5044/chrome/browser/ui/v...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:489:
RunMoveLoop(mouse_offset_);  // Runs a nested loop, returning when done.
sky: This offset is wrong. I'm not sure what else I could do here though. Do you
have any suggestions.

https://chromiumcodereview.appspot.com/10828133/diff/5044/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5044/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:111: // the chrome process.
derat: Off the top of your head, do you have any ideas what could be causing
this?

[sky, 2012-08-14 22:49:23]

https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:489:
RunMoveLoop(mouse_offset_);  // Runs a nested loop, returning when done.
I'm a bit confused. What location are you trying to calculate?

https://chromiumcodereview.appspot.com/10828133/diff/156/ui/views/widget/widg...
File ui/views/widget/widget.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/156/ui/views/widget/widg...
ui/views/widget/widget.h:323: MoveLoopResult RunMoveLoop(const gfx::Point&
drag_offset);
Document what drag_offset is.

[Elliot Glaysher, 2012-08-14 22:55:00]

https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:489:
RunMoveLoop(mouse_offset_);  // Runs a nested loop, returning when done.
On 2012/08/14 22:49:23, sky wrote:
> I'm a bit confused. What location are you trying to calculate?

The offset from the top left corner of the window to the cursor location where
the drag was started. During the nested message loop, I receive screen absolute
coordinates from the X server, which then I set the new window's location by
this offset.

(I documented this per request.)

[sky, 2012-08-14 23:00:39]

That's something like the following placed in Init.

gfx::Point start_location(source_tab_offset, mouse_offset_.y());
View::ConvertPointToWidget(source_tab, &start_location);

But I'm not sure you really want that since the tab being dragged
moves around in the tabstrip. Seems like you want something like the
above, but only at detach time.

  -Scott

On Tue, Aug 14, 2012 at 3:55 PM,  <erg@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
> File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):
>
>
https://chromiumcodereview.appspot.com/10828133/diff/156/chrome/browser/ui/vi...
> chrome/browser/ui/views/tabs/tab_drag_controller.cc:489:
> RunMoveLoop(mouse_offset_);  // Runs a nested loop, returning when done.
> On 2012/08/14 22:49:23, sky wrote:
>>
>> I'm a bit confused. What location are you trying to calculate?
>
>
> The offset from the top left corner of the window to the cursor location
> where the drag was started. During the nested message loop, I receive
> screen absolute coordinates from the X server, which then I set the new
> window's location by this offset.
>
> (I documented this per request.)
>
> https://chromiumcodereview.appspot.com/10828133/

[Elliot Glaysher, 2012-08-15 20:07:17]

Done at attach() time.

[Daniel Erat, 2012-08-15 20:30:57]

https://chromiumcodereview.appspot.com/10828133/diff/5084/base/message_pump_a...
File base/message_pump_aurax11.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/base/message_pump_a...
base/message_pump_aurax11.h:16: // It would be nice to including X11 headers
here so that we use Window instead
nit: s/including/include/

https://chromiumcodereview.appspot.com/10828133/diff/5084/base/message_pump_a...
base/message_pump_aurax11.h:49: // xserver that |w| has been mapped; events on
|w| are pulled out from the
nit: s/events/StructureNotifyMask (see
http://tronche.com/gui/x/xlib/events/processing-overview.html) events/

https://chromiumcodereview.appspot.com/10828133/diff/5084/base/message_pump_a...
base/message_pump_aurax11.h:57: void BlockUntilWindowMapped(unsigned long w);
nit: s/w/window/

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/aura/root_window...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:810: XUnmapWindow(xdisplay_, xwindow_);
do you need this XUnmapWindow() call?  the man page for XWithdrawWindow() (which
i wasn't familiar with) says that it unmaps the window (or perhaps just calling
XUnmapWindow() would be significant; i'm not sure why the synthetic UnmapNotify
event that the withdraw function sends is necessary).

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/base/event.h
File ui/base/event.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/base/event.h#new...
ui/base/event.h:189: gfx::Point system_location_;
Add a comment describing what "system" means here.  How does it differ from
|root_location_|?  It looks like you're actually using the root location from
the event in the X11 implementation.

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:29: ui::KeyEvent* event) {
i think you may want to break out of the loop when the user hits escape, still. 
it's presumably possible for the mouse button to be released while the pointer
isn't grabbed (even if you have the server grabbed at the same time).

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:82: XUngrabPointer(display,
CurrentTime);
refresh my memory: where's the previous pointer grab coming from?

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:95: NOTREACHED() << "Grab
failed: " << ret;
this shouldn't be a NOTREACHED(); it can legitimately happen if e.g. someone
else grabs the pointer before you

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:111: // on drawn out windows?
Not ungrabing here screws the X server until I kill
nit: s/ungrabing/ungrabbing/

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.h:42: // Are we running a nested
message loop from RunMoveLoop().
nit: s/\./?/

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.h:45: // Our cursor offset on
first invocation.
what's it an offset from, the target window's origin?

[Elliot Glaysher, 2012-08-15 20:55:59]

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/aura/root_window...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:810: XUnmapWindow(xdisplay_, xwindow_);
On 2012/08/15 20:30:57, Daniel Erat wrote:
> do you need this XUnmapWindow() call?  the man page for XWithdrawWindow()
(which
> i wasn't familiar with) says that it unmaps the window (or perhaps just
calling
> XUnmapWindow() would be significant; i'm not sure why the synthetic
UnmapNotify
> event that the withdraw function sends is necessary).

I called both because GTK called both. If I remove the unmap , it still works
though, so removed.

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/base/event.h
File ui/base/event.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/base/event.h#new...
ui/base/event.h:189: gfx::Point system_location_;
On 2012/08/15 20:30:57, Daniel Erat wrote:
> Add a comment describing what "system" means here.  How does it differ from
> |root_location_|?  It looks like you're actually using the root location from
> the event in the X11 implementation.

Commented. "Root location" in aura and "root location" in x11 mean different
things. This tripped me up in an earlier porting patch when I fed the X11 root
location into |root_location_| and broke some chromeos stuff.

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:29: ui::KeyEvent* event) {
On 2012/08/15 20:30:57, Daniel Erat wrote:
> i think you may want to break out of the loop when the user hits escape,
still. 
> it's presumably possible for the mouse button to be released while the pointer
> isn't grabbed (even if you have the server grabbed at the same time).

I used to have that code here that did just that. I deleted it (and tested that
ESC still broke out) when I saw that it was done in a cross platform way in
TabDragController.

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:82: XUngrabPointer(display,
CurrentTime);
On 2012/08/15 20:30:57, Daniel Erat wrote:
> refresh my memory: where's the previous pointer grab coming from?

The implicit mousedown grab that began this drag.

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:95: NOTREACHED() << "Grab
failed: " << ret;
On 2012/08/15 20:30:57, Daniel Erat wrote:
> this shouldn't be a NOTREACHED(); it can legitimately happen if e.g. someone
> else grabs the pointer before you

What should it be instead? A return? Given that I grabbed the server, undid any
other grab and then grabbed the pointer, how would that work?

[Daniel Erat, 2012-08-16 00:25:17]

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:95: NOTREACHED() << "Grab
failed: " << ret;
On 2012/08/15 20:55:59, Elliot Glaysher wrote:
> On 2012/08/15 20:30:57, Daniel Erat wrote:
> > this shouldn't be a NOTREACHED(); it can legitimately happen if e.g. someone
> > else grabs the pointer before you
> 
> What should it be instead? A return? Given that I grabbed the server, undid
any
> other grab and then grabbed the pointer, how would that work?

Assuming that the pointer is grabbed before you grab the server, I think that
this is almost safe.  There are other reasons why a grab can fail (e.g. the
window manager can unmap your window or one of its parents at any time), though,
so I'd prefer that you still handled failure.  It's presumably also possible for
the mouse button to be released (ending the implicit grab) before this code gets
called, which would again make it possible for someone else to grab the pointer
in the meantime.

[Elliot Glaysher, 2012-08-16 19:32:53]

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/5084/ui/views/widget/x11...
ui/views/widget/x11_desktop_window_move_client.cc:95: NOTREACHED() << "Grab
failed: " << ret;
On 2012/08/16 00:25:17, Daniel Erat wrote:
> On 2012/08/15 20:55:59, Elliot Glaysher wrote:
> > On 2012/08/15 20:30:57, Daniel Erat wrote:
> > > this shouldn't be a NOTREACHED(); it can legitimately happen if e.g.
someone
> > > else grabs the pointer before you
> > 
> > What should it be instead? A return? Given that I grabbed the server, undid
> any
> > other grab and then grabbed the pointer, how would that work?
> 
> Assuming that the pointer is grabbed before you grab the server, I think that
> this is almost safe.  There are other reasons why a grab can fail (e.g. the
> window manager can unmap your window or one of its parents at any time),
though,
> so I'd prefer that you still handled failure.  It's presumably also possible
for
> the mouse button to be released (ending the implicit grab) before this code
gets
> called, which would again make it possible for someone else to grab the
pointer
> in the meantime.

Done. Also handled the error after we free the server grab.

[Daniel Erat, 2012-08-16 19:45:13]

The X stuff LGTM now.

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/views/widget/x11_...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/views/widget/x11_...
ui/views/widget/x11_desktop_window_move_client.cc:26:
X11DesktopWindowMoveClient::~X11DesktopWindowMoveClient() {}
should this contain:

  if (in_move_loop_)
    EndMoveLoop();

?

[Elliot Glaysher, 2012-08-16 19:50:31]

ping sky

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/views/widget/x11_...
File ui/views/widget/x11_desktop_window_move_client.cc (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/views/widget/x11_...
ui/views/widget/x11_desktop_window_move_client.cc:26:
X11DesktopWindowMoveClient::~X11DesktopWindowMoveClient() {}
On 2012/08/16 19:45:13, Daniel Erat wrote:
> should this contain:
> 
>   if (in_move_loop_)
>     EndMoveLoop();
> 
> ?

I'm pretty sure that's not possible. This object is owned by things that can't
be deleted while the nested run loop is being run. (And the other
implementations don't do this.)

[sky, 2012-08-16 20:11:20]

https://chromiumcodereview.appspot.com/10828133/diff/163/chrome/browser/ui/vi...
File chrome/browser/ui/views/tabs/tab_drag_controller.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/chrome/browser/ui/vi...
chrome/browser/ui/views/tabs/tab_drag_controller.h:324: void RunMoveLoop(const
gfx::Point& drag_offset);
Document drag_offset.

https://chromiumcodereview.appspot.com/10828133/diff/163/chrome/browser/ui/vi...
chrome/browser/ui/views/tabs/tab_drag_controller.h:477: gfx::Point
window_mouse_offset_;
Add description.

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/aura/client/windo...
File ui/aura/client/window_move_client.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/aura/client/windo...
ui/aura/client/window_move_client.h:23: // offset from the window origin to the
cursor when the window was detached.
'the window was detached' -> when the drag started

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/base/event.h
File ui/base/event.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/base/event.h#newc...
ui/base/event.h:140: return system_location_ != gfx::Point(-1, -1);
Can't -1 -1 be a valid location?

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/base/event.h#newc...
ui/base/event.h:143: gfx::Point system_location() const { return
system_location_; }
const gfx::Point&

[Elliot Glaysher, 2012-08-16 20:33:47]

https://chromiumcodereview.appspot.com/10828133/diff/163/chrome/browser/ui/vi...
File chrome/browser/ui/views/tabs/tab_drag_controller.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/chrome/browser/ui/vi...
chrome/browser/ui/views/tabs/tab_drag_controller.h:477: gfx::Point
window_mouse_offset_;
On 2012/08/16 20:11:20, sky wrote:
> Add description.

Removed stray mark.

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/base/event.h
File ui/base/event.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/163/ui/base/event.h#newc...
ui/base/event.h:140: return system_location_ != gfx::Point(-1, -1);
On 2012/08/16 20:11:20, sky wrote:
> Can't -1 -1 be a valid location?

I guess it might. Changed to out of band boolean to track validity.

[sky, 2012-08-16 20:57:22]

LGTM

[Elliot Glaysher, 2012-08-16 21:26:58]

brettw: base stamp?

[Ben Goodger (Google), 2012-08-16 22:51:39]

lgtm also for the aura/event changes.

https://chromiumcodereview.appspot.com/10828133/diff/15042/ui/base/event.h
File ui/base/event.h (right):

https://chromiumcodereview.appspot.com/10828133/diff/15042/ui/base/event.h#ne...
ui/base/event.h:184: // |location_| in underlying system coordinates. This can
be invalid during
system screen coordinates.

[brettw, 2012-08-17 17:38:43]

base lgtm

[commit-bot: I haz the power, 2012-08-17 17:39:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10828133/5148

[commit-bot: I haz the power, 2012-08-17 18:10:14]

Try job failure for 10828133-5148 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-17 18:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10828133/5148

[commit-bot: I haz the power, 2012-08-17 22:43:48]

Change committed as 152180