ui/platform_window: Introduce PlatformWindow for windowing primitives.

ui/platform_window: Introduce PlatformWindow for windowing primitives.

Instead of implementing the native windowing primitives for aura, mojo, ozone
etc., have a single implementation in ui/platform_window, and use that in the
rest of the places. To start off, this CL provides an X11 implementation of
the new interface, and use it for mojo native-viewport.

BUG=361137
R=ben@chromium.org, spang@chromium.org

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

[sadrul, 2014-07-14 07:20:05]

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
File ui/platform_window/platform_window_delegate.h (right):

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window_delegate.h:37: virtual void OnCloseRequest()
= 0;
In response to https://codereview.chromium.org/378003003/#msg2:

OnCloseRequest() simply indicates that the native-platform has requested the
window to be closed. The delegate can chose to close the window by directly by
calling into PlatformWindow::Close(), or do other things before closing the
window, or simply ignore the request.

[spang, 2014-07-14 17:02:29]

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window.h:19: class PLATFORM_WINDOW_EXPORT
PlatformWindow {
I added some comments here in my version (and on each method). Might be nice to
do this here as well.

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window.h:27: virtual void SetBounds(const gfx::Rect&
bounds) = 0;
Should we have GetBounds?

Then WindowTreeHost can implement GetBounds as platform_window_->GetBounds().

[sadrul, 2014-07-14 17:14:35]

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window.h:27: virtual void SetBounds(const gfx::Rect&
bounds) = 0;
On 2014/07/14 17:02:29, spang wrote:
> Should we have GetBounds?
> 
> Then WindowTreeHost can implement GetBounds as platform_window_->GetBounds().

I considered doing this, but maybe it makes sense to let WindowTreeHost maintain
the bounds since it will get notified of the bounds from the
PlatformWindowDelegate::OnBoundsChanged() callback. But then again, it would be
necessary for all delegates to do the same thing ... I will add GetBounds().

[sadrul, 2014-07-14 17:36:21]

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window.h:19: class PLATFORM_WINDOW_EXPORT
PlatformWindow {
On 2014/07/14 17:02:29, spang wrote:
> I added some comments here in my version (and on each method). Might be nice
to
> do this here as well.

Done.

https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
ui/platform_window/platform_window.h:27: virtual void SetBounds(const gfx::Rect&
bounds) = 0;
On 2014/07/14 17:14:35, sadrul wrote:
> On 2014/07/14 17:02:29, spang wrote:
> > Should we have GetBounds?
> > 
> > Then WindowTreeHost can implement GetBounds as
platform_window_->GetBounds().
> 
> I considered doing this, but maybe it makes sense to let WindowTreeHost
maintain
> the bounds since it will get notified of the bounds from the
> PlatformWindowDelegate::OnBoundsChanged() callback. But then again, it would
be
> necessary for all delegates to do the same thing ... I will add GetBounds().

Done.

[spang, 2014-07-14 17:41:20]

On 2014/07/14 17:36:21, sadrul wrote:
>
https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
> File ui/platform_window/platform_window.h (right):
> 
>
https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
> ui/platform_window/platform_window.h:19: class PLATFORM_WINDOW_EXPORT
> PlatformWindow {
> On 2014/07/14 17:02:29, spang wrote:
> > I added some comments here in my version (and on each method). Might be nice
> to
> > do this here as well.
> 
> Done.
> 
>
https://codereview.chromium.org/391583002/diff/70001/ui/platform_window/platf...
> ui/platform_window/platform_window.h:27: virtual void SetBounds(const
gfx::Rect&
> bounds) = 0;
> On 2014/07/14 17:14:35, sadrul wrote:
> > On 2014/07/14 17:02:29, spang wrote:
> > > Should we have GetBounds?
> > > 
> > > Then WindowTreeHost can implement GetBounds as
> platform_window_->GetBounds().
> > 
> > I considered doing this, but maybe it makes sense to let WindowTreeHost
> maintain
> > the bounds since it will get notified of the bounds from the
> > PlatformWindowDelegate::OnBoundsChanged() callback. But then again, it would
> be
> > necessary for all delegates to do the same thing ... I will add GetBounds().
> 
> Done.

lgtm (for whatever that's worth)

I am eagerly awaiting this landing so we can support richer windowing under
ozone.. and pass more tests.

[Ben Goodger (Google), 2014-07-14 19:22:55]

lgtm

[sadrul, 2014-07-14 22:49:19]

+sky@

I realized that with my changes, EnableTestNativeViewport() (which was necessary
to unflake some tests) became a no-op. So I updated the CL to do the same thing
(i.e. set override_redirect to True when creating the X11 Winow) from X11Window.
This change is in patchset#6. However, with this change, the relevant tests
(i.e. mojo_view_manager_unittests) time-out both locally and in the trybots. I
spent some time trying to figure out why this was happening, but came up blank
(the call to view_manager_init->EmbedRoot() in view_manager_unittest.cc never
calls the callback, which is why the run-loop never terminates, but I couldn't
figure out where to go from there).

So I have simply removed the EnableTestNativeViewport() in patchset#7 instead.
The tests are happier locally with this change (and in the trybots in earlier
equivalent patchsets). Do you think it makes sense to try to land this and see
if the tests keep working?

[sky, 2014-07-15 04:26:55]

What I had done is something like:

#!/bin/sh

i=0
while [ $i -lt 1000 ]; do
  echo $i
  mojo_view_manager_unittests
  if [ $? -ne 0 ];
  then
    exit 0
  fi
  i=$((i+1))
done

If this works fine, then land away.

[sadrul, 2014-07-15 05:46:52]

On 2014/07/15 04:26:55, sky wrote:
> What I had done is something like:
> 
> #!/bin/sh
> 
> i=0
> while [ $i -lt 1000 ]; do
>   echo $i
>   mojo_view_manager_unittests
>   if [ $? -ne 0 ];
>   then
>     exit 0
>   fi
>   i=$((i+1))
> done
> 
> If this works fine, then land away.

It gets stuck after ~25 iterations :(

I then tried this with ToT, and it still gets stuck after a few iterations :/

Any chance you could give it a try on your system when you get a chance with
this patch?  ｡＾･ｪ･＾｡

[sadrul, 2014-07-15 17:48:36]

Committed patchset #7 manually as r283213 (presubmit successful).