ozone: Port WindowTreeHostOzone on top of PlatformWindow

ozone: Port WindowTreeHostOzone on top of PlatformWindow

Most of the WindowTreeHost functionality is actually implemented by the
platform window, so there's not much in WindowTreeHostOzone aside from
forwarding calls to and from the platform part.
    
Should be no functional change for in-tree platforms. To accomplish
that, this adds a transitional class PlatformWindowCompat that
implements PlatformWindow functions as WindowTreeHostOzone did before.
As a followup, we'll convert each platform to use the new PlatformWindow
natively & then remove PlatformWindowCompat.

BUG=392280
TEST=built with chromeos==1 use_ozone==1 & ran all platforms.
NOTRY=true
TBR=ben@chromium.org

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

[spang, 2014-07-08 21:51:15]

depends on https://codereview.chromium.org/378003003/

[spang, 2014-07-14 22:09:42]

PTAL

[sadrul, 2014-07-14 22:58:07]

https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.h (right):

https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.h:26: public ui::PlatformEventDispatcher,
It should no longer be necessary for WindowTreeHostOzone to be a
PlatformEventDispatcher.

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
File ui/ozone/common/platform_window_base.h (right):

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
ui/ozone/common/platform_window_base.h:24: class OZONE_EXPORT PlatformWindowBase
: public PlatformWindow {
I don't think we should have this. If we must, we should add default
implementations in PlatformWindow instead, but I don't think we should do that
either.

For the short-term, have a single PlatformWindowStub that does what Compat does,
but the platforms should provide complete implementations (with NOTIMPLEMENTED()
where appropriate).

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

https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.h (right):

https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.h:26: public ui::PlatformEventDispatcher,
On 2014/07/14 22:58:07, sadrul wrote:
> It should no longer be necessary for WindowTreeHostOzone to be a
> PlatformEventDispatcher.

Done.

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
File ui/ozone/common/platform_window_base.h (right):

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
ui/ozone/common/platform_window_base.h:24: class OZONE_EXPORT PlatformWindowBase
: public PlatformWindow {
On 2014/07/14 22:58:07, sadrul wrote:
> I don't think we should have this. If we must, we should add default
> implementations in PlatformWindow instead, but I don't think we should do that
> either.

Please elaborate - what's the harm? Maximizing doesn't make any sense when the
output is an image file, or a TV. All 5 of our current platforms need stubs
here. X11 will be the only one that is reasonably complete, and can just not
inherit this class.

Putting it on PlatformWindow itself is probaby a mistake, since out of tree
platforms may break silently if we change the interface. I'd like them to have
the option of getting a compile error instead.

> For the short-term, have a single PlatformWindowStub that does what Compat
does,
> but the platforms should provide complete implementations (with
NOTIMPLEMENTED()
> where appropriate).

I disagree. PlatformWindowCompat is not a stub, and its name captures what it
actually does. In any case, it's not going to exist very long.

[spang, 2014-07-14 23:26:11]

On 2014/07/14 23:17:41, spang wrote:
>
https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
> File ui/aura/window_tree_host_ozone.h (right):
> 
>
https://codereview.chromium.org/375053002/diff/40001/ui/aura/window_tree_host...
> ui/aura/window_tree_host_ozone.h:26: public ui::PlatformEventDispatcher,
> On 2014/07/14 22:58:07, sadrul wrote:
> > It should no longer be necessary for WindowTreeHostOzone to be a
> > PlatformEventDispatcher.
> 
> Done.
> 
>
https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
> File ui/ozone/common/platform_window_base.h (right):
> 
>
https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
> ui/ozone/common/platform_window_base.h:24: class OZONE_EXPORT
PlatformWindowBase
> : public PlatformWindow {
> On 2014/07/14 22:58:07, sadrul wrote:
> > I don't think we should have this. If we must, we should add default
> > implementations in PlatformWindow instead, but I don't think we should do
that
> > either.
> 
> Please elaborate - what's the harm? Maximizing doesn't make any sense when the
> output is an image file, or a TV. All 5 of our current platforms need stubs
> here. X11 will be the only one that is reasonably complete, and can just not
> inherit this class.
> 

To put it another way, ozone is home to several regressive target platforms for
which half of the functions here cannot be implemented. If you want me to copy
these stubs to all of those, I'll probably do it, but I might not be happy out
it.

> Putting it on PlatformWindow itself is probaby a mistake, since out of tree
> platforms may break silently if we change the interface. I'd like them to have
> the option of getting a compile error instead.
> 
> > For the short-term, have a single PlatformWindowStub that does what Compat
> does,
> > but the platforms should provide complete implementations (with
> NOTIMPLEMENTED()
> > where appropriate).
> 
> I disagree. PlatformWindowCompat is not a stub, and its name captures what it
> actually does. In any case, it's not going to exist very long.

[sadrul, 2014-07-15 16:53:10]

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
File ui/ozone/common/platform_window_base.h (right):

https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
ui/ozone/common/platform_window_base.h:24: class OZONE_EXPORT PlatformWindowBase
: public PlatformWindow {
On 2014/07/14 23:17:41, spang wrote:
> On 2014/07/14 22:58:07, sadrul wrote:
> > I don't think we should have this. If we must, we should add default
> > implementations in PlatformWindow instead, but I don't think we should do
that
> > either.
> 
> Please elaborate - what's the harm? Maximizing doesn't make any sense when the
> output is an image file, or a TV. All 5 of our current platforms need stubs
> here. X11 will be the only one that is reasonably complete, and can just not
> inherit this class.

The platforms that don't need maximize will have an empty implementation.

> Putting it on PlatformWindow itself is probaby a mistake, since out of tree
> platforms may break silently if we change the interface. I'd like them to have
> the option of getting a compile error instead.

Yes, and this applies for the in-tree platforms too. Each [new] ozone platform
that gets added should explicitly make the decision of which methods to
implement and which not. I think this is generally the style chromium follows
(see aura::WindowDelegate, for an example, and the numerous overrides that
provide empty implementation for a number of the interface there), observers
being the exception. I personally don't have strong preference either way, but
consistency would be good.

> 
> > For the short-term, have a single PlatformWindowStub that does what Compat
> does,
> > but the platforms should provide complete implementations (with
> NOTIMPLEMENTED()
> > where appropriate).
> 
> I disagree. PlatformWindowCompat is not a stub, and its name captures what it
> actually does. In any case, it's not going to exist very long.

[spang, 2014-07-15 17:38:16]

On 2014/07/15 16:53:10, sadrul wrote:
>
https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
> File ui/ozone/common/platform_window_base.h (right):
> 
>
https://codereview.chromium.org/375053002/diff/40001/ui/ozone/common/platform...
> ui/ozone/common/platform_window_base.h:24: class OZONE_EXPORT
PlatformWindowBase
> : public PlatformWindow {
> On 2014/07/14 23:17:41, spang wrote:
> > On 2014/07/14 22:58:07, sadrul wrote:
> > > I don't think we should have this. If we must, we should add default
> > > implementations in PlatformWindow instead, but I don't think we should do
> that
> > > either.
> > 
> > Please elaborate - what's the harm? Maximizing doesn't make any sense when
the
> > output is an image file, or a TV. All 5 of our current platforms need stubs
> > here. X11 will be the only one that is reasonably complete, and can just not
> > inherit this class.
> 
> The platforms that don't need maximize will have an empty implementation.
> 
> > Putting it on PlatformWindow itself is probaby a mistake, since out of tree
> > platforms may break silently if we change the interface. I'd like them to
have
> > the option of getting a compile error instead.
> 
> Yes, and this applies for the in-tree platforms too.

In-tree platforms go through our review process, and we'll check that the change
makes sense at that time.

> Each [new] ozone platform
> that gets added should explicitly make the decision of which methods to
> implement and which not. I think this is generally the style chromium follows
> (see aura::WindowDelegate, for an example, and the numerous overrides that
> provide empty implementation for a number of the interface there), observers
> being the exception. I personally don't have strong preference either way, but
> consistency would be good.

content uses a similar pattern: see RenderWidgetHostViewBase

Anyway, this argument is probably wasting more time than we'd gain by
deduplicating our in-tree platforms. I have merged PlatformWindowBase into
PlatformWindowCompat for now.

> 
> > 
> > > For the short-term, have a single PlatformWindowStub that does what Compat
> > does,
> > > but the platforms should provide complete implementations (with
> > NOTIMPLEMENTED()
> > > where appropriate).
> > 
> > I disagree. PlatformWindowCompat is not a stub, and its name captures what
it
> > actually does. In any case, it's not going to exist very long.

[sadrul, 2014-07-15 22:49:21]

lgtm

[spang, 2014-07-16 18:10:35]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-07-16 18:13:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/spang@chromium.org/375053002/100001

[commit-bot: I haz the power, 2014-07-16 23:27:10]

Change committed as 283573