Connect X11 ConfigureNotify events to Mojo

Connect X11 ConfigureNotify events to Mojo

This change plumbs top level window resize notifications from X11 through to the WindowManager and to the GLSurface associated with the top level window. It's an incremental step towards adding support for window manager level layout management (see crbug.com/389785).


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

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

[hansmuller, 2014-07-02 18:05:19]

This is just an incremental step towards fixing crbug.com/389785 - Handle
interactive resizing in Native Viewport, View Manager, Window Manager.

[jamesr, 2014-07-02 18:12:50]

The /gles2/ and /native_viewport/ parts lgtm.  The view manager code seems fine,
but I'm not very familiar with that part so Ben or Scott should probably review
as well.

https://codereview.chromium.org/354933002/diff/10001/mojo/services/native_vie...
File mojo/services/native_viewport/native_viewport_x11.cc (right):

https://codereview.chromium.org/354933002/diff/10001/mojo/services/native_vie...
mojo/services/native_viewport/native_viewport_x11.cc:53: // TODO(hansmuller):
should not be processing the shell's commad line here.
I think it'd be better to split this part into a separate patch if it's not too
much trouble

https://codereview.chromium.org/354933002/diff/10001/mojo/services/native_vie...
mojo/services/native_viewport/native_viewport_x11.cc:198: } else  if
(event->type == ConfigureNotify) {
nit: i think you have two spaces between 'else' and 'if' here.

you can run 'git cl format' to have an autoformatter fix up things like this for
you

[hansmuller, 2014-07-02 18:36:34]

On 2014/07/02 18:12:50, jamesr wrote:
> ...
>
https://codereview.chromium.org/354933002/diff/10001/mojo/services/native_vie...
> mojo/services/native_viewport/native_viewport_x11.cc:53: // TODO(hansmuller):
> should not be processing the shell's commad line here.
> I think it'd be better to split this part into a separate patch if it's not
too
> much trouble

No trouble at all, I've taken it out.

> 
>
https://codereview.chromium.org/354933002/diff/10001/mojo/services/native_vie...
> mojo/services/native_viewport/native_viewport_x11.cc:198: } else  if
> (event->type == ConfigureNotify) {
> nit: i think you have two spaces between 'else' and 'if' here.

I did. Thanks for the pointer about 'git cl format'.

[Ben Goodger (Google), 2014-07-08 15:41:57]

https://codereview.chromium.org/354933002/diff/30001/mojo/examples/window_man...
File mojo/examples/window_manager/window_manager.cc (right):

https://codereview.chromium.org/354933002/diff/30001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:162: explicit
RootLayoutManager(ViewManager* view_manager,
nit: explicit only if there is one param to the ctor.

https://codereview.chromium.org/354933002/diff/30001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:171: // Overridden from
NodeObserver
nit: trailing :

https://codereview.chromium.org/354933002/diff/30001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:181: // TODO(hansmuller): this is
just a hack to force a redraw
Can you schedule the draw in the service side of SetBounds? Seems like it
shouldn't be too much extra effort.

https://codereview.chromium.org/354933002/diff/30001/mojo/examples/window_man...
mojo/examples/window_manager/window_manager.cc:290: content_node_id_));
you're leaking the layout manager. The WindowManager could probably own it via
scoped_ptr.

https://codereview.chromium.org/354933002/diff/30001/mojo/services/native_vie...
File mojo/services/native_viewport/native_viewport_x11.cc (right):

https://codereview.chromium.org/354933002/diff/30001/mojo/services/native_vie...
mojo/services/native_viewport/native_viewport_x11.cc:12: #include
"mojo/shell/switches.h"
doesn't look like you need either of these #includes anymore.

https://codereview.chromium.org/354933002/diff/30001/mojo/services/view_manag...
File mojo/services/view_manager/window_tree_host_impl.cc (right):

https://codereview.chromium.org/354933002/diff/30001/mojo/services/view_manag...
mojo/services/view_manager/window_tree_host_impl.cc:143: Node* root_node =
root_node_manager_->root();
Instead of doing this (and adding the root_node_manager_ member) I'd instead
implement an aura::LayoutManager and attach that to the root aura::Window.
That'll keep the root node's window in sync with the root window.

[hansmuller, 2014-07-08 21:49:39]

Thanks for the feedback, I've made all of the changes you suggested.

[Ben Goodger (Google), 2014-07-08 22:08:08]

lgtm

https://codereview.chromium.org/354933002/diff/70001/mojo/services/view_manag...
File mojo/services/view_manager/window_tree_host_impl.cc (right):

https://codereview.chromium.org/354933002/diff/70001/mojo/services/view_manag...
mojo/services/view_manager/window_tree_host_impl.cc:64:
SetChildBoundsDirect(child, gfx::Rect(0, 0,
You can replace the second param with this:

gfx::Rect(requested_bounds.width(),
          requested_bounds.height());

(rect has a ctor that takes w, h params).

[hansmuller, 2014-07-08 23:06:34]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-08 23:13:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/354933002/90001

[hansmuller, 2014-07-09 00:02:10]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-09 00:05:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/354933002/110001

[commit-bot: I haz the power, 2014-07-09 05:33:05]

Change committed as 281955

[horo, 2014-07-09 07:22:48]

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

The reason for reverting is:
http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28...

[  FAILED  ] ViewManagerTest.SetNodeBounds (109 ms)
.

[sky, 2014-07-09 21:23:22]

https://codereview.chromium.org/354933002/diff/110001/mojo/services/view_mana...
File mojo/services/view_manager/window_tree_host_impl.cc (right):

https://codereview.chromium.org/354933002/diff/110001/mojo/services/view_mana...
mojo/services/view_manager/window_tree_host_impl.cc:33: RootLayoutManager() :
child_(NULL) { }
nit: {}

https://codereview.chromium.org/354933002/diff/110001/mojo/services/view_mana...
mojo/services/view_manager/window_tree_host_impl.cc:53:
child_->SetBounds(gfx::Rect(0, 0, bounds.width(), bounds.height()));
nit: gfx::Rect(bounds.size()) and nuke 52 so that you end up with:
child_->SetBounds(gfx::Rect(child_->parent()->bounds().size());

Otheriwse you should make 52:
const gfx::Rect& bounds = ...;
but since it all fits on one line, what I suggest above is easier.

https://codereview.chromium.org/354933002/diff/110001/mojo/services/view_mana...
mojo/services/view_manager/window_tree_host_impl.cc:65:
requested_bounds.width(),
nit: gfx::Rect(requested_bounds.size())

[hansmuller, 2014-07-09 21:52:38]

Thanks for the feedback, I've made the changes.

[hansmuller, 2014-07-09 21:52:54]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:55:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/354933002/130001

[commit-bot: I haz the power, 2014-07-09 23:50:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 04:33:49]

Change committed as 282254