mojo: Fix running view-manager related apptests on X11.

mojo: Fix running view-manager related apptests on X11.

When X11Window creates an X11 window for apptests, set override_redirect to true
in order to prevent the X11 window-manager from intercepting the map-requests
for the window, which can, at times, and for unknown reasons, cause
X11Window::Show() to block because it never receives the MapNotify event.

BUG=504917
Committed: https://crrev.com/da9ab7ea905b7ec4ff60889911078a5994473043
Cr-Commit-Position: refs/heads/master@{#339626}

[sadrul, 2015-07-16 07:32:48]

sadrul@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2015-07-16 19:17:50]

msw@chromium.org changed reviewers:
+ erg@chromium.org

[msw, 2015-07-16 19:17:50]

Thank you immensely for digging into these hangs!

I'm not sufficiently familiar with X11 to understand this well. +erg and +cc
derat for review or to pull in others.

https://codereview.chromium.org/1228173003/diff/1/components/view_manager/vie...
File components/view_manager/view_manager_app.cc (right):

https://codereview.chromium.org/1228173003/diff/1/components/view_manager/vie...
components/view_manager/view_manager_app.cc:58:
ui::test::SetUseOverrideRedirectWindowByDefault(true);
Should this be part of gfx::GLSurface::InitializeOneOffForTests?

https://codereview.chromium.org/1228173003/diff/1/ui/platform_window/x11/x11_...
File ui/platform_window/x11/x11_window.h (right):

https://codereview.chromium.org/1228173003/diff/1/ui/platform_window/x11/x11_...
ui/platform_window/x11/x11_window.h:70: X11_WINDOW_EXPORT void
SetUseOverrideRedirectWindowByDefault(
Can you add an explanatory comment here? Even after reading
https://tronche.com/gui/x/xlib/window/attributes/override-redirect.html I don't
fully understand the functionality of XSetWindowAttributes::override_redirect
(or the reasoning for setting it).

[Elliot Glaysher, 2015-07-16 19:23:55]

lgtm

I remember all the previous problems we've had with the various window managers
we've used on the testing xvfb scripts. I'm slightly wary of divergence between
what we're testing and what we're using, but TBH, we have larger divergences on
the linux port today.

[sadrul, 2015-07-16 19:57:14]

https://codereview.chromium.org/1228173003/diff/1/components/view_manager/vie...
File components/view_manager/view_manager_app.cc (right):

https://codereview.chromium.org/1228173003/diff/1/components/view_manager/vie...
components/view_manager/view_manager_app.cc:58:
ui::test::SetUseOverrideRedirectWindowByDefault(true);
On 2015/07/16 19:17:50, msw wrote:
> Should this be part of gfx::GLSurface::InitializeOneOffForTests?

Hm, good point. Done.

https://codereview.chromium.org/1228173003/diff/1/ui/platform_window/x11/x11_...
File ui/platform_window/x11/x11_window.h (right):

https://codereview.chromium.org/1228173003/diff/1/ui/platform_window/x11/x11_...
ui/platform_window/x11/x11_window.h:70: X11_WINDOW_EXPORT void
SetUseOverrideRedirectWindowByDefault(
On 2015/07/16 19:17:50, msw wrote:
> Can you add an explanatory comment here? Even after reading
> https://tronche.com/gui/x/xlib/window/attributes/override-redirect.html I
don't
> fully understand the functionality of XSetWindowAttributes::override_redirect
> (or the reasoning for setting it).

Added a comment. This is fairly voodoo, and might very well depend on the
particular window-manager/x-server.

[msw, 2015-07-16 20:02:44]

lgtm with a nit, assuming the new dependency is okay.

https://codereview.chromium.org/1228173003/diff/40001/ui/platform_window/x11/...
File ui/platform_window/x11/x11_window.h (right):

https://codereview.chromium.org/1228173003/diff/40001/ui/platform_window/x11/...
ui/platform_window/x11/x11_window.h:78: }  // namespace test
nit: blank line before closing the namespace, to mirror opening.

[Daniel Erat, 2015-07-17 03:13:40]

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

[Daniel Erat, 2015-07-17 03:13:40]

what all are these windows used for in tests? they won't be able to request the
input focus if they're override-redirect, for instance. will that cause
problems? (if the tests pass, maybe it's fine.)

[sadrul, 2015-07-17 05:11:53]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org

[sadrul, 2015-07-17 05:11:53]

On 2015/07/16 20:02:44, msw wrote:
> lgtm with a nit, assuming the new dependency is okay.

platform_window/x11 is pretty light-weight, and doesn't bring in other new
dependencies. So I think it should be OK.

+piman@ to confirm as ui/gl/ owner.

On 2015/07/17 03:13:40, Daniel Erat wrote:
> what all are these windows used for in tests? they won't be able to request
the
> input focus if they're override-redirect, for instance. will that cause
> problems? (if the tests pass, maybe it's fine.)

I think this should be relatively safe (we do this for browser-tests, ash-tests
etc. too, for example).

https://codereview.chromium.org/1228173003/diff/40001/ui/platform_window/x11/...
File ui/platform_window/x11/x11_window.h (right):

https://codereview.chromium.org/1228173003/diff/40001/ui/platform_window/x11/...
ui/platform_window/x11/x11_window.h:78: }  // namespace test
On 2015/07/16 20:02:44, msw wrote:
> nit: blank line before closing the namespace, to mirror opening.

Done.

[Ken Russell (switch to Gerrit), 2015-07-20 19:34:17]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-07-20 19:34:18]

ui/gl changes LGTM

[piman, 2015-07-20 21:59:12]

lgtm

[sadrul, 2015-07-21 05:55:28]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-07-21 05:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
msw@chromium.org, piman@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1228173003/#ps100001
(title: "tot-merge")

[commit-bot: I haz the power, 2015-07-21 05:55:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228173003/100001

[commit-bot: I haz the power, 2015-07-21 06:01:03]

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

[commit-bot: I haz the power, 2015-07-21 06:01:03]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[sadrul, 2015-07-21 06:13:48]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2015-07-21 06:13:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228173003/100001

[sadrul, 2015-07-21 06:27:54]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-07-21 06:27:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
piman@chromium.org, erg@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1228173003/#ps120001
(title: ".")

[commit-bot: I haz the power, 2015-07-21 06:28:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228173003/120001

[commit-bot: I haz the power, 2015-07-21 08:22:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-21 08:23:15]

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