Fix X11TopmostWindowFinder

Fix X11TopmostWindowFinder to take into account
- Window manager provided window borders
- Custom shapes specified via views::Widget::SetShape()

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270877

[pkotwicz, 2014-05-03 01:26:48]

Sadrul, PTAL

I will fix up the unit tests once this CL lands. Honestly, I am not too keen on
caching information for the browser window (i.e. it does not make
X11TopmostWindowFinder::FindLocalProcessWindowAt() significantly faster. In
particular dragging within a tab strip is super slow singularly because
X11TopmostWindowFinder::FindLocalProcessWindowAt() is slow. (crbug.com/367382)
and this CL does not contribute to fixing the slowness in a significant way.

We most likely need a full cache of the X11 window heirarchy. I do not mind
working on that once you are happy with the state of tests. The call to
gfx::Screen::GetWindowAtScreenPoint() in tooltip_controller.cc is somewhat
problematic...

[pkotwicz, 2014-05-07 01:43:49]

Ping?

[sadrul, 2014-05-08 18:53:29]

Looks good. A couple of nits. Can we have tests?

https://codereview.chromium.org/268673017/diff/80001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/268673017/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1136: if
(ui::GetAtomArrayProperty(xwindow_, "_NET_WM_STATE", &atom_list)) {
Early out instead if this is false?

https://codereview.chromium.org/268673017/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1182: if
(ui::GetIntArrayProperty(xwindow_, "_NET_FRAME_EXTENTS", &insets) &&
Add a comment about the ordering in _NET_FRAME_EXTENTS

https://codereview.chromium.org/268673017/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1305: 0, 0,
window_shape_, false);
Interesting. Can there be a test for this case?

[pkotwicz, 2014-05-09 21:11:07]

Sadrul, can you please take another look? I think I have addressed all of your
comments

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:349:
MapWindow(show_state);
I added a call to ResetWindowRegion() here. This only has an effect if the user
is running Chrome with a non-EWMH compliant window manager. Otherwise, the shape
is set as a result of OnWMStateUpdated()

[sadrul, 2014-05-14 02:49:05]

LGTM

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
(right):

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:8: //
conflicts with the definition of None in gtest-type-util.h
In a few places, we include the X11 headers, followed by a few #undefs, before
including other chromium headers. We can do the same here.

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:213: bool
ShapeRectContainsPoint(std::vector<gfx::Rect> shape_rects,
const &

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:233:
XSynchronize(gfx::GetXDisplay(), True);
Interesting!

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:241:
ui::X11EventSource::GetInstance()->DispatchXEvents();
I assume this is expected to dispatch the events from showing the window because
of the call to XSynchronize above?

[pkotwicz, 2014-05-14 04:07:24]

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
(right):

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:8: //
conflicts with the definition of None in gtest-type-util.h
A couple of the header files include Xlib.h. (x11_atom_cache.h does) Should I
remove the includes to Xlib.h and use forward declaration instead?

I tried doing forward declaration for https://codereview.chromium.org/268083002/
and got stuck on how to forward declare XMotionEvent in
x11_whole_screen_move_loop_delegate.h

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:241:
ui::X11EventSource::GetInstance()->DispatchXEvents();
Yes. This has the important side effect of ensuring that we get the
PropertyNotify for having set _NET_WM_STATE in
DesktopWindowTreeHostX11::InitX11Window()

[sadrul, 2014-05-14 04:11:49]

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
(right):

https://codereview.chromium.org/268673017/diff/100001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:8: //
conflicts with the definition of None in gtest-type-util.h
On 2014/05/14 04:07:25, pkotwicz wrote:
> A couple of the header files include Xlib.h. (x11_atom_cache.h does) Should I
> remove the includes to Xlib.h and use forward declaration instead?
> 
> I tried doing forward declaration for
https://codereview.chromium.org/268083002/
> and got stuck on how to forward declare XMotionEvent in
> x11_whole_screen_move_loop_delegate.h

Perhaps forward declare XEvent instead, and use XEvent in the parameter, with a
CHECK in the implementation on the event-type?

[pkotwicz, 2014-05-14 17:47:51]

Sadrul, can you please quickly take a look at the changes in x11_atom_cache.*

https://codereview.chromium.org/268673017/diff/160001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
(right):

https://codereview.chromium.org/268673017/diff/160001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:241:
virtual void TearDown() OVERRIDE {
I make the X11 display connection asynchronous again at the end of the test

[pkotwicz, 2014-05-15 15:56:00]

Sadrul, ping!

[sadrul, 2014-05-15 15:57:19]

On 2014/05/15 15:56:00, pkotwicz wrote:
> Sadrul, ping!

Can you land the atom changes separately first?

[pkotwicz, 2014-05-15 22:02:10]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-15 22:03:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/268673017/180001

[commit-bot: I haz the power, 2014-05-16 00:41:57]

Change committed as 270877

[Adrian Kuegel, 2014-05-16 08:23:08]

A revert of this CL has been created in
https://codereview.chromium.org/291583003/ by akuegel@chromium.org.

The reason for reverting is: Speculative revert because of failure in
desktop_screen_x11_unittest.cc (DesktopScreenX11Test.GetWindowAtScreenPoint)..