Aura/ash split: Don't use X11 window borders.

Aura/ash split: Don't use X11 window borders.

This disables window manager borders, and supports dragging of the
skyline, as well as resizing from the corners.

This patch also moves our ::Atom caching out of RootWindowHostLinux and into its own Singleton.

BUG=125106
TEST=none

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

[Elliot Glaysher, 2012-05-09 19:39:18]

http://codereview.chromium.org/10381063/diff/21/ui/aura/desktop/desktop_root_...
File ui/aura/desktop/desktop_root_window_event_filter.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/desktop/desktop_root_...
ui/aura/desktop/desktop_root_window_event_filter.cc:90: // like I'm copypasting
the entire file.
I think this is the next thing I'm going to do. Several of the EventFilters in
ash make sense to reuse on the desktop, some won't even need modifications.

http://codereview.chromium.org/10381063/diff/21/ui/base/win/events_win.cc
File ui/base/win/events_win.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/base/win/events_win.cc#new...
ui/base/win/events_win.cc:228: // coordinates. Is that equivalent to root
coordinates on windows?
attn ben: is this correct for window?

http://codereview.chromium.org/10381063/diff/21/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/base/x/events_x.cc#newcode796
ui/base/x/events_x.cc:796: // not handle an important case?
attn derat/sadrul: this appears to work, but I don't know what a valuator is or
how I apply them.

[Daniel Erat, 2012-05-09 20:58:27]

http://codereview.chromium.org/10381063/diff/21/ui/aura/event.cc
File ui/aura/event.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/event.cc#newcode128
ui/aura/event.cc:128: // TODO(oshima): This isn't correct with more than two
RootWindows.
Not sure I understand this.  Should it be "two or more RootWindows", or is there
something else going on here that I'm not seeing?

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window.h#newcode140
ui/aura/root_window.h:140: void SetUseOSWindowBorders(bool use_os_borders);
Mind renaming this to something like SetUseHostWindowBorders() (to match
set_hide_host_cursor())?

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window.h#newcode161
ui/aura/root_window.h:161: bool DispatchWindowDragMovement(int hittest,
MouseEvent* event);
DispatchHostWindowDragMovement()?

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:59: unsigned long flags;
nit: indent 2 spaces instead of 4

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:63: unsigned long status;
Holy 64-bit-unsafety!  Still, it looks like this is what metacity does, so i
guess it's safest to match it... Add a comment, though?

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:832: sizeof(MotifWmHints)/sizeof(long));
does this break on 64-bit?  should you be dividing by 4 instead?  the man page
says:

nelements: Specifies the number of elements of the specified data format.

hate Xlib so much.

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:895: XUngrabPointer(xdisplay_, CurrentTime);
this seems strange to me.  won't it get released automatically when the user
releases the mouse button?

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linux.h
File ui/aura/root_window_host_linux.h (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.h:32: void SetShouldUseWindowManagerBorder(bool
use_wm_border);
delete this

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_win.cc
File ui/aura/root_window_host_win.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_win....
ui/aura/root_window_host_win.cc:289: void
RootWindowHostWin::SetUseOSWindowBorders(bool use_os_borders) {
nit: NOTIMPLEMENTED()

http://codereview.chromium.org/10381063/diff/21/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/base/x/events_x.cc#newcode796
ui/base/x/events_x.cc:796: // not handle an important case?
On 2012/05/09 19:39:18, Elliot Glaysher wrote:
> attn derat/sadrul: this appears to work, but I don't know what a valuator is
or
> how I apply them.

I don't think that you need to worry about valuators here, but I'll let Sadrul
comment.

[Elliot Glaysher, 2012-05-09 22:23:08]

ptal

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:832: sizeof(MotifWmHints)/sizeof(long));
On 2012/05/09 20:58:27, Daniel Erat wrote:
> does this break on 64-bit?  should you be dividing by 4 instead?  the man page
> says:
> 
> nelements: Specifies the number of elements of the specified data format.
> 
> hate Xlib so much.

This is what GDK does and I'm testing this in a 64-bit build.

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:895: XUngrabPointer(xdisplay_, CurrentTime);
On 2012/05/09 20:58:27, Daniel Erat wrote:
> this seems strange to me.  won't it get released automatically when the user
> releases the mouse button?

Yeap, and that's the point. The application needs to dump it's pointer grab
because what we're about to do is tell the window manager that it's now
responsible for moving the window around; it immediately grabs when it receives
the event below.

If we don't release our grab before we send this message, we're still holding
the grab and the window manager doesn't grab for the drag.

Hate X11.

[Daniel Erat, 2012-05-09 22:38:16]

LGTM for X stuff.

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:832: sizeof(MotifWmHints)/sizeof(long));
On 2012/05/09 22:23:09, Elliot Glaysher wrote:
> On 2012/05/09 20:58:27, Daniel Erat wrote:
> > does this break on 64-bit?  should you be dividing by 4 instead?  the man
page
> > says:
> > 
> > nelements: Specifies the number of elements of the specified data format.
> > 
> > hate Xlib so much.
> 
> This is what GDK does and I'm testing this in a 64-bit build.

Okay.

http://codereview.chromium.org/10381063/diff/21/ui/aura/root_window_host_linu...
ui/aura/root_window_host_linux.cc:895: XUngrabPointer(xdisplay_, CurrentTime);
On 2012/05/09 22:23:09, Elliot Glaysher wrote:
> On 2012/05/09 20:58:27, Daniel Erat wrote:
> > this seems strange to me.  won't it get released automatically when the user
> > releases the mouse button?
> 
> Yeap, and that's the point. The application needs to dump it's pointer grab
> because what we're about to do is tell the window manager that it's now
> responsible for moving the window around; it immediately grabs when it
receives
> the event below.
> 
> If we don't release our grab before we send this message, we're still holding
> the grab and the window manager doesn't grab for the drag.
> 
> Hate X11.

Thanks, had forgotten the super-flaky, racy "grab pass" that EWMH uses for this.
 Mind adding a comment?

[sadrul, 2012-05-10 13:34:10]

http://codereview.chromium.org/10381063/diff/1011/ui/aura/event.cc
File ui/aura/event.cc (right):

http://codereview.chromium.org/10381063/diff/1011/ui/aura/event.cc#newcode103
ui/aura/event.cc:103:
root_location_(ui::EventRootLocationFromNative(native_event)) {
This might be tricky; I think 'root_location_' is meant to be in the
RootWindow's coordinate system, and 'location_' is meant to be in a Window's
coordinate system. Changing the meaning of 'root_location_' to be in the
system's coordinate system may introduce bugs. It looks like you need the
system-coordinate only in RootWindowHostLinux::DispatchHostWindowDragMovement.
Would it make sense to keep the meaning of root_location_ unchanged, and extract
the desired location in RWHL::DispatchHostWindowDragMovement from
event->native_event()?

[Ben Goodger (Google), 2012-05-10 14:52:58]

http://codereview.chromium.org/10381063/diff/1011/ui/aura/root_window_host.h
File ui/aura/root_window_host.h (right):

http://codereview.chromium.org/10381063/diff/1011/ui/aura/root_window_host.h#...
ui/aura/root_window_host.h:98: virtual void SetUseHostWindowBorders(bool
use_os_borders) = 0;
So I'd rather not expose chrome-ui related functionality on aura types. Is there
a way you can do this from a type outside of aura? e.g. on Windows we get the
host's accelerated widget (HWND) and install a win32 window subclass on it
(basically replace the window procedure) which allows us to respond differently
to messages etc.

http://codereview.chromium.org/10381063/diff/1011/ui/aura/root_window_host.h#...
ui/aura/root_window_host.h:104: MouseEvent* event) = 0;
similar to above. seems like you could attach a helper to the root window host
linux that intercepts mouse moves and does this when appropriate.

[Elliot Glaysher, 2012-05-15 20:45:19]

Everyone, PTAL.

derat: I've modified where the X11 code lives and split off the X11 ::Atom cache
off RootHostWindowLinux.

ben: With r137175 committed, I'm now putting all the x11 specific window manager
code in a new X11WindowEventFilter class, which hangs off the
RootWindowEventFilter.

sadrul: I removed all modifications to the event code and put the event
processing in X11WindowEventFilter.

[Ben Goodger (Google), 2012-05-15 20:52:07]

Cool. I like this approach more. I'll let the X-heads comment on your X11 stuff.

http://codereview.chromium.org/10381063/diff/11034/ui/views/widget/x11_window...
File ui/views/widget/x11_window_event_filter.h (right):

http://codereview.chromium.org/10381063/diff/11034/ui/views/widget/x11_window...
ui/views/widget/x11_window_event_filter.h:27:
X11WindowEventFilter(aura::RootWindow* root_window);
explicit

[Daniel Erat, 2012-05-15 20:59:40]

http://codereview.chromium.org/10381063/diff/11034/ui/base/x/x11_atom_cache.cc
File ui/base/x/x11_atom_cache.cc (right):

http://codereview.chromium.org/10381063/diff/11034/ui/base/x/x11_atom_cache.c...
ui/base/x/x11_atom_cache.cc:14: // X11 server. Must be kept in sync with
AtomCache::AtomName
This makes me nervous (especially combined with non-alphabetical ordering of the
entries in the list).  It's more code, but mind making this be an array of
structs containing (enum, string) pairs and loading everything into a map
instead?  See e.g.
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...
and
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl....

[Elliot Glaysher, 2012-05-15 21:56:32]

http://codereview.chromium.org/10381063/diff/11034/ui/base/x/x11_atom_cache.cc
File ui/base/x/x11_atom_cache.cc (right):

http://codereview.chromium.org/10381063/diff/11034/ui/base/x/x11_atom_cache.c...
ui/base/x/x11_atom_cache.cc:14: // X11 server. Must be kept in sync with
AtomCache::AtomName
On 2012/05/15 20:59:41, Daniel Erat wrote:
> This makes me nervous (especially combined with non-alphabetical ordering of
the
> entries in the list).  It's more code, but mind making this be an array of
> structs containing (enum, string) pairs and loading everything into a map
> instead?  See e.g.
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...
> and
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl....

Done.

[sadrul, 2012-05-15 22:08:12]

lgtm

http://codereview.chromium.org/10381063/diff/4004/ui/base/x/x11_atom_cache.h
File ui/base/x/x11_atom_cache.h (right):

http://codereview.chromium.org/10381063/diff/4004/ui/base/x/x11_atom_cache.h#...
ui/base/x/x11_atom_cache.h:34: // Pre-caches all Atoms on first use to minimize
roundtrips to the X11
This doc may be confusing with ui::GetAtom (in ui/base/x/x11_util.h). Perhaps
clarify that the cache is for the atoms listed in AtomName.

[Daniel Erat, 2012-05-15 22:31:23]

LGTM

Thanks, this looks good to me.  Just one nit and a question.

http://codereview.chromium.org/10381063/diff/4004/ui/views/widget/x11_window_...
File ui/views/widget/x11_window_event_filter.cc (right):

http://codereview.chromium.org/10381063/diff/4004/ui/views/widget/x11_window_...
ui/views/widget/x11_window_event_filter.cc:85: if (event->type() ==
ui::ET_MOUSE_PRESSED) {
maybe just switch this to != and return early?

http://codereview.chromium.org/10381063/diff/4004/ui/views/widget/x11_window_...
ui/views/widget/x11_window_event_filter.cc:95: case ButtonRelease: {
i don't think i follow the ButtonRelease and MotionNotify cases here; do we
really bundle those in an aura::MouseEvent with type ET_MOUSE_PRESSED sometimes?

[Elliot Glaysher, 2012-05-15 23:01:19]

done.

http://codereview.chromium.org/10381063/diff/4004/ui/views/widget/x11_window_...
File ui/views/widget/x11_window_event_filter.cc (right):

http://codereview.chromium.org/10381063/diff/4004/ui/views/widget/x11_window_...
ui/views/widget/x11_window_event_filter.cc:95: case ButtonRelease: {
On 2012/05/15 22:31:23, Daniel Erat wrote:
> i don't think i follow the ButtonRelease and MotionNotify cases here; do we
> really bundle those in an aura::MouseEvent with type ET_MOUSE_PRESSED
sometimes?

No, this was me copy/pasting from the general event code I wrote and then cut
out. Removed.

[Elliot Glaysher, 2012-05-16 17:11:01]

ping ben; need ui/ stamp

[Ben Goodger (Google), 2012-05-16 19:47:53]

lgtm

[commit-bot: I haz the power, 2012-05-16 19:48:27]

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

[commit-bot: I haz the power, 2012-05-16 21:34:53]

Change committed as 137525

[DaleCurtis, 2012-05-18 00:34:27]

On 2012/05/16 21:34:53, I haz the power (commit-bot) wrote:
> Change committed as 137525

This is causing a missing base::MessagePumpX::GetDefaultXDisplay() symbol in
libui.so when built w/o aura:

https://code.google.com/p/chromium/issues/detail?id=128584

Seems like x11_atom_cache probably should be in sources! for ui.gyp when
use_aura == 0, however I'm not sure.

[Elliot Glaysher, 2012-05-18 16:31:34]

Possibly, but there's nothing aura specific here. There's a rule in
filename_rules.gypi that should filter out any file starting with x11_
when use_x11!=1.

I'm not really sure what a linux build without use_x11 would be at
this point, since we pulled wayland out.

-- Elliot

On Thu, May 17, 2012 at 5:34 PM,  <dalecurtis@chromium.org> wrote:
> On 2012/05/16 21:34:53, I haz the power (commit-bot) wrote:
>>
>> Change committed as 137525
>
>
> This is causing a missing base::MessagePumpX::GetDefaultXDisplay() symbol in
> libui.so when built w/o aura:
>
> https://code.google.com/p/chromium/issues/detail?id=128584
>
> Seems like x11_atom_cache probably should be in sources! for ui.gyp when
> use_aura == 0, however I'm not sure.
>
> https://chromiumcodereview.appspot.com/10381063/