aura: Try to make Linux host resize code more reliable.

aura: Try to make Linux host resize code more reliable.

There's no guarantee that we'll get the host window size that
we ask for.  This adjusts WindowTest.Transform to not ask for
a specific size, and instead just use the size it's been
given.

I also fixed a race in DesktopHostLinux::Show() when running
under X window managers that don't take the _WM_S0 selection. 
We now wait for notification that the host window has been
mapped before trying to focus it.

I moved the logic for choosing to create a fullscreen host
window out of DesktopHostLinux and into the browser.

Finally, I changed DesktopHostLinux::SetSize() (called by
Desktop::SetHostSize()) update the internal size and notify
the desktop immediately.

BUG=100979, 100894
TEST=tests pass on ion3; fullscreen window is created on chrome os

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

[Daniel Erat, 2011-10-21 19:15:23]

I need to be OOO for 2-3 hours but can check this in after that (if you don't
want to do so earlier).

[sadrul, 2011-10-21 19:19:55]

This LGTM.

aura_unittests is currently green, so landing this after 2-3 hrs sounds OK to
me.

[Daniel Erat, 2011-10-21 19:49:40]

Actually, this change is broken -- it drops ConfigureNotify events. :-P I'll
fix it soon.

On Friday, October 21, 2011,  <sadrul@chromium.org> wrote:
> This LGTM.
>
> aura_unittests is currently green, so landing this after 2-3 hrs sounds OK
to
> me.
>
> http://codereview.chromium.org/8374005/
>

[oshima, 2011-10-21 19:55:45]

http://codereview.chromium.org/8374005/diff/4001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8374005/diff/4001/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:966:
gfx::Screen::GetMonitorAreaNearestPoint(gfx::Point()));
Just a suggestion. I've been using string to compare rect/ponit and find it
useful.

EXPECT_EQ(expected.ToString(), actual.ToString());
EXPECT_EQ("0,0 100x100", actual.ToString());

It makes it easy to read failure message.

[Daniel Erat, 2011-10-21 23:00:36]

Another look?  I noticed some more problems.

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:219: gfx::Size size_;
using a Rect here didn't make sense; we never need the position, and worse, we
weren't updating it in response to ConfigureNotify events

[sadrul, 2011-10-22 18:53:04]

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop.h#newcode74
ui/aura/desktop.h:74: // may not be immediately reflected in GetHostSize().
I dislike this part of the change.

I confirmed that, with at least ion3, a Desktop->SetHostSize triggers a
ConfigureNotify event, even though it ignores the resize request. I think we
should either keep the current behaviour (this would be my preference), or wait
for ConfigureNotify before returning from SetSize.

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:219: gfx::Size size_;
On 2011/10/21 23:00:37, Daniel Erat wrote:
> using a Rect here didn't make sense; we never need the position, and worse, we
> weren't updating it in response to ConfigureNotify events

Nice

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:238: 0, 0, DisplayWidth(xdisplay_, 0),
DisplayHeight(xdisplay_, 0));
I was thinking about this change ... perhaps we are doing this in the wrong
place. Perhaps we should let the client/shell decide the desktop size for the
environment. That way, chrome can make sure it's fullscreen when it's running in
proper chromeos, and not on a chromeos build on a linux desktop.

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:401: XSetInputFocus(xdisplay_, xwindow_,
RevertToNone, CurrentTime);
How about we just XMapWindow+XFlush here and return, and call XSetInputFocus
from MapNotify event in Dispatch

[Daniel Erat, 2011-10-23 04:02:55]

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop.h#newcode74
ui/aura/desktop.h:74: // may not be immediately reflected in GetHostSize().
On 2011/10/22 18:53:04, sadrul wrote:
> I dislike this part of the change.
> 
> I confirmed that, with at least ion3, a Desktop->SetHostSize triggers a
> ConfigureNotify event, even though it ignores the resize request. I think we
> should either keep the current behaviour (this would be my preference), or
wait
> for ConfigureNotify before returning from SetSize.

I'm in favor of waiting for the ConfigureNotify before returning (since keeping
the current behavior means that we're confused about our size for a period of
time).  ICCCM requires that WMs send a synthetic ConfigureNotify even if they
ignore the request IIRC; I just wasn't sure if that was another part of the spec
that Tuomo disagreed with. :-P

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:238: 0, 0, DisplayWidth(xdisplay_, 0),
DisplayHeight(xdisplay_, 0));
On 2011/10/22 18:53:04, sadrul wrote:
> I was thinking about this change ... perhaps we are doing this in the wrong
> place. Perhaps we should let the client/shell decide the desktop size for the
> environment. That way, chrome can make sure it's fullscreen when it's running
in
> proper chromeos, and not on a chromeos build on a linux desktop.

Yeah, I wasn't entirely convinced about this part.  Moving the logic to the
shell means that the shell needs to know some platform-dependent things, though:
- whether there's a window manager running or not (this could be controlled by a
flag that we set in the Chrome OS scripts, I guess)
- the actual resolution of the host display (not sure how this should get there)

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:401: XSetInputFocus(xdisplay_, xwindow_,
RevertToNone, CurrentTime);
On 2011/10/22 18:53:04, sadrul wrote:
> How about we just XMapWindow+XFlush here and return, and call XSetInputFocus
> from MapNotify event in Dispatch

Thanks, I like this more as well.  Sounds fine, as I don't think there's any
requirement that the host window actually has / is on the way to receiving the
focus by the time that this method returns.

[sadrul, 2011-10-23 20:45:02]

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/2005/ui/aura/desktop_host_linux.c...
ui/aura/desktop_host_linux.cc:238: 0, 0, DisplayWidth(xdisplay_, 0),
DisplayHeight(xdisplay_, 0));
On 2011/10/23 04:02:55, Daniel Erat wrote:
> On 2011/10/22 18:53:04, sadrul wrote:
> > I was thinking about this change ... perhaps we are doing this in the wrong
> > place. Perhaps we should let the client/shell decide the desktop size for
the
> > environment. That way, chrome can make sure it's fullscreen when it's
running
> in
> > proper chromeos, and not on a chromeos build on a linux desktop.
> 
> Yeah, I wasn't entirely convinced about this part.  Moving the logic to the
> shell means that the shell needs to know some platform-dependent things,
though:
> - whether there's a window manager running or not (this could be controlled by
a
> flag that we set in the Chrome OS scripts, I guess)

I was thinking perhaps we can always to this for chromeos (by using
system::runtime_environment::IsRunningOnChromeOS from within chrome to confirm
chrome is running in proper chromeos, and not a chromeos build on the desktop).

> - the actual resolution of the host display (not sure how this should get
there)

[Daniel Erat, 2011-10-24 17:40:39]

Another look?

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (left):

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:285: case MotionNotify: {
I'm just moving this so these are in alphabetical order.

[oshima, 2011-10-24 20:29:22]

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:397: desktop_->OnHostResized(size);
Sorry if I'm missing something. Don't we still need XResizeWindow here?

[Daniel Erat, 2011-10-24 20:33:28]

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8374005/diff/11001/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:397: desktop_->OnHostResized(size);
On 2011/10/24 20:29:22, oshima wrote:
> Sorry if I'm missing something. Don't we still need XResizeWindow here?

Whoops.  Um, yes.  Sorry, bad merge.

[sadrul, 2011-10-24 20:40:24]

LGTM

http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:986: EXPECT_EQ(gfx::Rect(gfx::Point(),
transformed_size).ToString(),
I think just 'gfx::Rect(transformed_size)' will work

[Daniel Erat, 2011-10-24 20:45:40]

http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:986: EXPECT_EQ(gfx::Rect(gfx::Point(),
transformed_size).ToString(),
On 2011/10/24 20:40:24, sadrul wrote:
> I think just 'gfx::Rect(transformed_size)' will work

Good point.  Done.

[oshima, 2011-10-24 20:52:41]

LGTM

On Mon, Oct 24, 2011 at 1:45 PM, <derat@chromium.org> wrote:

>
> http://codereview.chromium.**org/8374005/diff/6004/ui/aura/**
>
window_unittest.cc<http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc>
> File ui/aura/window_unittest.cc (right):
>
> http://codereview.chromium.**org/8374005/diff/6004/ui/aura/**
>
window_unittest.cc#newcode986<http://codereview.chromium.org/8374005/diff/6004/ui/aura/window_unittest.cc#newcode986>
> ui/aura/window_unittest.cc:**986: EXPECT_EQ(gfx::Rect(gfx::**Point(),
> transformed_size).ToString(),
> On 2011/10/24 20:40:24, sadrul wrote:
>
>> I think just 'gfx::Rect(transformed_size)' will work
>>
>
> Good point.  Done.
>
>
http://codereview.chromium.**org/8374005/<http://codereview.chromium.org/8374...
>