platform-window: Add a PlatformWindowFactory for creating windows.

platform-window: Add a PlatformWindowFactory for creating windows.

This allows using a common NativeViewport implementation on x11, win32 and ozone:
PlatformViewport. A test-factory can be used in tests to create dummy
PlatformWindow objects, once off-screen rendering is possible for mojo.

BUG=none

[sadrul, 2014-07-20 08:09:21]

spang@ for changes in ui/ozone/, and platform_viewport.cc (I considered
splitting out OzoneWindowFactory into its own file, but having it in
ui/ozone/public/ seemed a bit odd, since it doesn't really belong in the public
api)

ben@ for all of it.

[spang, 2014-07-21 16:46:58]

On 2014/07/20 08:09:21, sadrul wrote:
> spang@ for changes in ui/ozone/, and platform_viewport.cc (I considered
> splitting out OzoneWindowFactory into its own file, but having it in
> ui/ozone/public/ seemed a bit odd, since it doesn't really belong in the
public
> api)
> 
> ben@ for all of it.

I don't really like creating a new global object just to forward t

[spang, 2014-07-21 16:58:44]

On 2014/07/21 16:46:58, spang wrote:
> On 2014/07/20 08:09:21, sadrul wrote:
> > spang@ for changes in ui/ozone/, and platform_viewport.cc (I considered
> > splitting out OzoneWindowFactory into its own file, but having it in
> > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in the
> public
> > api)
> > 
> > ben@ for all of it.
> 
> I don't really like creating a new global object just to forward t

just to forward the call.

In particular I'm not enthusiastic about this pattern of "set a mutable static
pointer in a lower level component to call code in a higher level one". This
isn't as straightforward as calling the code directly and I'd prefer we'd only
do it when it provides a really big win in flexibility.

So, can we just keep two files in native viewport:

native_viewport_ozone.cc

scoped_ptr<NativeViewport> NativeViewport::Create(
     shell::Context* context,
     NativeViewportDelegate* delegate) { 
   ui::OzonePlatform::InitializeForUI();
  return scoped_ptr<NativeViewport>(new PlatformViewport(
            delegate, base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
ui::OzonePlatform::GetInstance())));
}

native_viewport_x11.c

scoped_ptr<NativeViewport> NativeViewport::Create(
     shell::Context* context,
     NativeViewportDelegate* delegate) { 
   ui::X11WindowFactory::CreateIfNecessary();
  return scoped_ptr<NativeViewport>(new PlatformViewport(
            delegate, base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
ui::X11WindowFactory::GetInstance())));
}

This also gives us a better place to call X11WindowFactory::CreateIfNecessary &
ui::OzonePlatform::InitializeForUI.

[spang, 2014-07-21 16:59:07]

https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
File mojo/services/native_viewport/platform_viewport.cc (left):

https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
mojo/services/native_viewport/platform_viewport.cc:27:
ui::OzonePlatform::InitializeForUI();
did this move somewhere?

[sadrul, 2014-07-21 17:39:54]

On 2014/07/21 16:58:44, spang wrote:
> On 2014/07/21 16:46:58, spang wrote:
> > On 2014/07/20 08:09:21, sadrul wrote:
> > > spang@ for changes in ui/ozone/, and platform_viewport.cc (I considered
> > > splitting out OzoneWindowFactory into its own file, but having it in
> > > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in the
> > public
> > > api)
> > > 
> > > ben@ for all of it.
> > 
> > I don't really like creating a new global object just to forward t
> 
> just to forward the call.
> 
> In particular I'm not enthusiastic about this pattern of "set a mutable static
> pointer in a lower level component to call code in a higher level one". This
> isn't as straightforward as calling the code directly and I'd prefer we'd only
> do it when it provides a really big win in flexibility.
> 
> So, can we just keep two files in native viewport:
> 
> native_viewport_ozone.cc
> 
> scoped_ptr<NativeViewport> NativeViewport::Create(
>      shell::Context* context,
>      NativeViewportDelegate* delegate) { 
>    ui::OzonePlatform::InitializeForUI();
>   return scoped_ptr<NativeViewport>(new PlatformViewport(
>             delegate, base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
> ui::OzonePlatform::GetInstance())));
> }
> 
> native_viewport_x11.c
> 
> scoped_ptr<NativeViewport> NativeViewport::Create(
>      shell::Context* context,
>      NativeViewportDelegate* delegate) { 
>    ui::X11WindowFactory::CreateIfNecessary();
>   return scoped_ptr<NativeViewport>(new PlatformViewport(
>             delegate, base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
> ui::X11WindowFactory::GetInstance())));
> }
> 
> This also gives us a better place to call X11WindowFactory::CreateIfNecessary
&
> ui::OzonePlatform::InitializeForUI.

I have plans to use a TestPlatformWindowFactory instance for tests that creates
a dummy PlatformWindow implementation that doesn't do anything (like
PlatformWindowCompat), for crbug.com/385230. We need PlatformViewport to use the
factory for that.

https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
File mojo/services/native_viewport/platform_viewport.cc (left):

https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
mojo/services/native_viewport/platform_viewport.cc:27:
ui::OzonePlatform::InitializeForUI();
On 2014/07/21 16:59:07, spang wrote:
> did this move somewhere?

I had this below after line 96, but it looks like aura::Env instance gets
created before the call to NativeViewport::Create(). So I removed that. I can
add it back though.

[spang, 2014-07-21 18:09:41]

On 2014/07/21 17:39:54, sadrul wrote:
> On 2014/07/21 16:58:44, spang wrote:
> > On 2014/07/21 16:46:58, spang wrote:
> > > On 2014/07/20 08:09:21, sadrul wrote:
> > > > spang@ for changes in ui/ozone/, and platform_viewport.cc (I considered
> > > > splitting out OzoneWindowFactory into its own file, but having it in
> > > > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in the
> > > public
> > > > api)
> > > > 
> > > > ben@ for all of it.
> > > 
> > > I don't really like creating a new global object just to forward t
> > 
> > just to forward the call.
> > 
> > In particular I'm not enthusiastic about this pattern of "set a mutable
static
> > pointer in a lower level component to call code in a higher level one". This
> > isn't as straightforward as calling the code directly and I'd prefer we'd
only
> > do it when it provides a really big win in flexibility.
> > 
> > So, can we just keep two files in native viewport:
> > 
> > native_viewport_ozone.cc
> > 
> > scoped_ptr<NativeViewport> NativeViewport::Create(
> >      shell::Context* context,
> >      NativeViewportDelegate* delegate) { 
> >    ui::OzonePlatform::InitializeForUI();
> >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> >             delegate, base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
> > ui::OzonePlatform::GetInstance())));
> > }
> > 
> > native_viewport_x11.c
> > 
> > scoped_ptr<NativeViewport> NativeViewport::Create(
> >      shell::Context* context,
> >      NativeViewportDelegate* delegate) { 
> >    ui::X11WindowFactory::CreateIfNecessary();
> >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> >             delegate,
base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
> > ui::X11WindowFactory::GetInstance())));
> > }
> > 
> > This also gives us a better place to call
X11WindowFactory::CreateIfNecessary
> &
> > ui::OzonePlatform::InitializeForUI.
> 
> I have plans to use a TestPlatformWindowFactory instance for tests that
creates
> a dummy PlatformWindow implementation that doesn't do anything (like
> PlatformWindowCompat), for crbug.com/385230. We need PlatformViewport to use
the
> factory for that.
> 

This will probably cause "You can only have one PlatformWindowFactory" type
errors, or else we'll have a bunch of conditional initialization. This is one of
the disadvantages of the pointer - now we have to worry about who gets to own
it. It would better for the tests to instantiate their mock factory directly and
build the native viewport service out of that.

>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> File mojo/services/native_viewport/platform_viewport.cc (left):
> 
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> mojo/services/native_viewport/platform_viewport.cc:27:
> ui::OzonePlatform::InitializeForUI();
> On 2014/07/21 16:59:07, spang wrote:
> > did this move somewhere?
> 
> I had this below after line 96, but it looks like aura::Env instance gets
> created before the call to NativeViewport::Create(). So I removed that. I can
> add it back though.

I didn't think mojo's viewport depends on aura. If it doesn't we do need it.

[spang, 2014-07-21 18:14:47]

On 2014/07/21 18:09:41, spang wrote:
> On 2014/07/21 17:39:54, sadrul wrote:
> > On 2014/07/21 16:58:44, spang wrote:
> > > On 2014/07/21 16:46:58, spang wrote:
> > > > On 2014/07/20 08:09:21, sadrul wrote:
> > > > > spang@ for changes in ui/ozone/, and platform_viewport.cc (I
considered
> > > > > splitting out OzoneWindowFactory into its own file, but having it in
> > > > > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in
the
> > > > public
> > > > > api)
> > > > > 
> > > > > ben@ for all of it.
> > > > 
> > > > I don't really like creating a new global object just to forward t
> > > 
> > > just to forward the call.
> > > 
> > > In particular I'm not enthusiastic about this pattern of "set a mutable
> static
> > > pointer in a lower level component to call code in a higher level one".
This
> > > isn't as straightforward as calling the code directly and I'd prefer we'd
> only
> > > do it when it provides a really big win in flexibility.
> > > 
> > > So, can we just keep two files in native viewport:
> > > 
> > > native_viewport_ozone.cc
> > > 
> > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > >      shell::Context* context,
> > >      NativeViewportDelegate* delegate) { 
> > >    ui::OzonePlatform::InitializeForUI();
> > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > >             delegate, base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
> > > ui::OzonePlatform::GetInstance())));
> > > }
> > > 
> > > native_viewport_x11.c
> > > 
> > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > >      shell::Context* context,
> > >      NativeViewportDelegate* delegate) { 
> > >    ui::X11WindowFactory::CreateIfNecessary();
> > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > >             delegate,
> base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
> > > ui::X11WindowFactory::GetInstance())));
> > > }
> > > 
> > > This also gives us a better place to call
> X11WindowFactory::CreateIfNecessary
> > &
> > > ui::OzonePlatform::InitializeForUI.
> > 
> > I have plans to use a TestPlatformWindowFactory instance for tests that
> creates
> > a dummy PlatformWindow implementation that doesn't do anything (like
> > PlatformWindowCompat), for crbug.com/385230. We need PlatformViewport to use
> the
> > factory for that.
> > 
> 
> This will probably cause "You can only have one PlatformWindowFactory" type
> errors, or else we'll have a bunch of conditional initialization. This is one
of
> the disadvantages of the pointer - now we have to worry about who gets to own
> it. It would better for the tests to instantiate their mock factory directly
and
> build the native viewport service out of that.

I don't want to hold things up too long over this, so I'll give ben a change to
chime in if he wants and otherwise will approve. I just do think there is a
better structure we try to get to eventually.

> 
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > File mojo/services/native_viewport/platform_viewport.cc (left):
> > 
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > mojo/services/native_viewport/platform_viewport.cc:27:
> > ui::OzonePlatform::InitializeForUI();
> > On 2014/07/21 16:59:07, spang wrote:
> > > did this move somewhere?
> > 
> > I had this below after line 96, but it looks like aura::Env instance gets
> > created before the call to NativeViewport::Create(). So I removed that. I
can
> > add it back though.
> 
> I didn't think mojo's viewport depends on aura. If it doesn't we do need it.

[sadrul, 2014-07-21 18:21:01]

On 2014/07/21 18:09:41, spang wrote:
> On 2014/07/21 17:39:54, sadrul wrote:
> > On 2014/07/21 16:58:44, spang wrote:
> > > On 2014/07/21 16:46:58, spang wrote:
> > > > On 2014/07/20 08:09:21, sadrul wrote:
> > > > > spang@ for changes in ui/ozone/, and platform_viewport.cc (I
considered
> > > > > splitting out OzoneWindowFactory into its own file, but having it in
> > > > > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in
the
> > > > public
> > > > > api)
> > > > > 
> > > > > ben@ for all of it.
> > > > 
> > > > I don't really like creating a new global object just to forward t
> > > 
> > > just to forward the call.
> > > 
> > > In particular I'm not enthusiastic about this pattern of "set a mutable
> static
> > > pointer in a lower level component to call code in a higher level one".
This
> > > isn't as straightforward as calling the code directly and I'd prefer we'd
> only
> > > do it when it provides a really big win in flexibility.
> > > 
> > > So, can we just keep two files in native viewport:
> > > 
> > > native_viewport_ozone.cc
> > > 
> > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > >      shell::Context* context,
> > >      NativeViewportDelegate* delegate) { 
> > >    ui::OzonePlatform::InitializeForUI();
> > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > >             delegate, base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
> > > ui::OzonePlatform::GetInstance())));
> > > }
> > > 
> > > native_viewport_x11.c
> > > 
> > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > >      shell::Context* context,
> > >      NativeViewportDelegate* delegate) { 
> > >    ui::X11WindowFactory::CreateIfNecessary();
> > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > >             delegate,
> base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
> > > ui::X11WindowFactory::GetInstance())));
> > > }
> > > 
> > > This also gives us a better place to call
> X11WindowFactory::CreateIfNecessary
> > &
> > > ui::OzonePlatform::InitializeForUI.
> > 
> > I have plans to use a TestPlatformWindowFactory instance for tests that
> creates
> > a dummy PlatformWindow implementation that doesn't do anything (like
> > PlatformWindowCompat), for crbug.com/385230. We need PlatformViewport to use
> the
> > factory for that.
> > 
> 
> This will probably cause "You can only have one PlatformWindowFactory" type
> errors, or else we'll have a bunch of conditional initialization.

Yes (see PlatformWindowFactory::PlatformWindowFactory)

> This is one of
> the disadvantages of the pointer - now we have to worry about who gets to own
> it.

It owns itself (and destroys itself via AtExitManager)

> It would better for the tests to instantiate their mock factory directly and
> build the native viewport service out of that.

The test would create the mock factory, but it may not necessarily be able to
replace/intercept the calls to NativeViewport::Create()

> 
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > File mojo/services/native_viewport/platform_viewport.cc (left):
> > 
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > mojo/services/native_viewport/platform_viewport.cc:27:
> > ui::OzonePlatform::InitializeForUI();
> > On 2014/07/21 16:59:07, spang wrote:
> > > did this move somewhere?
> > 
> > I had this below after line 96, but it looks like aura::Env instance gets
> > created before the call to NativeViewport::Create(). So I removed that. I
can
> > add it back though.
> 
> I didn't think mojo's viewport depends on aura. If it doesn't we do need it.

Added it back.

[spang, 2014-07-21 18:33:39]

On 2014/07/21 18:21:01, sadrul wrote:
> On 2014/07/21 18:09:41, spang wrote:
> > On 2014/07/21 17:39:54, sadrul wrote:
> > > On 2014/07/21 16:58:44, spang wrote:
> > > > On 2014/07/21 16:46:58, spang wrote:
> > > > > On 2014/07/20 08:09:21, sadrul wrote:
> > > > > > spang@ for changes in ui/ozone/, and platform_viewport.cc (I
> considered
> > > > > > splitting out OzoneWindowFactory into its own file, but having it in
> > > > > > ui/ozone/public/ seemed a bit odd, since it doesn't really belong in
> the
> > > > > public
> > > > > > api)
> > > > > > 
> > > > > > ben@ for all of it.
> > > > > 
> > > > > I don't really like creating a new global object just to forward t
> > > > 
> > > > just to forward the call.
> > > > 
> > > > In particular I'm not enthusiastic about this pattern of "set a mutable
> > static
> > > > pointer in a lower level component to call code in a higher level one".
> This
> > > > isn't as straightforward as calling the code directly and I'd prefer
we'd
> > only
> > > > do it when it provides a really big win in flexibility.
> > > > 
> > > > So, can we just keep two files in native viewport:
> > > > 
> > > > native_viewport_ozone.cc
> > > > 
> > > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > > >      shell::Context* context,
> > > >      NativeViewportDelegate* delegate) { 
> > > >    ui::OzonePlatform::InitializeForUI();
> > > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > > >             delegate,
base::Bind(&ui::OzonePlatform::CreatePlatformWindow,
> > > > ui::OzonePlatform::GetInstance())));
> > > > }
> > > > 
> > > > native_viewport_x11.c
> > > > 
> > > > scoped_ptr<NativeViewport> NativeViewport::Create(
> > > >      shell::Context* context,
> > > >      NativeViewportDelegate* delegate) { 
> > > >    ui::X11WindowFactory::CreateIfNecessary();
> > > >   return scoped_ptr<NativeViewport>(new PlatformViewport(
> > > >             delegate,
> > base::Bind(&ui::X11WindowFactory::CreatePlatformWindow,
> > > > ui::X11WindowFactory::GetInstance())));
> > > > }
> > > > 
> > > > This also gives us a better place to call
> > X11WindowFactory::CreateIfNecessary
> > > &
> > > > ui::OzonePlatform::InitializeForUI.
> > > 
> > > I have plans to use a TestPlatformWindowFactory instance for tests that
> > creates
> > > a dummy PlatformWindow implementation that doesn't do anything (like
> > > PlatformWindowCompat), for crbug.com/385230. We need PlatformViewport to
use
> > the
> > > factory for that.
> > > 
> > 
> > This will probably cause "You can only have one PlatformWindowFactory" type
> > errors, or else we'll have a bunch of conditional initialization.
> 
> Yes (see PlatformWindowFactory::PlatformWindowFactory)

I saw the code, my point is that this construct is a source of pain.

> 
> > This is one of
> > the disadvantages of the pointer - now we have to worry about who gets to
own
> > it.
> 
> It owns itself (and destroys itself via AtExitManager)

Sorry, own is overloaded. What I meant was: now we have to worry about who sets
it. We'll now have conflicts between ozone init and test init because one wants
to install a real factory and the other wants to install a fake one. The problem
is the shared global. The event source had similar issues.

> 
> > It would better for the tests to instantiate their mock factory directly and
> > build the native viewport service out of that.
> 
> The test would create the mock factory, but it may not necessarily be able to
> replace/intercept the calls to NativeViewport::Create()
 
It's not desirable for tests to intercept code paths in other components. This
is a code smell.

> 
> > 
> > >
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > > File mojo/services/native_viewport/platform_viewport.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/403263002/diff/40001/mojo/services/native_vie...
> > > mojo/services/native_viewport/platform_viewport.cc:27:
> > > ui::OzonePlatform::InitializeForUI();
> > > On 2014/07/21 16:59:07, spang wrote:
> > > > did this move somewhere?
> > > 
> > > I had this below after line 96, but it looks like aura::Env instance gets
> > > created before the call to NativeViewport::Create(). So I removed that. I
> can
> > > add it back though.
> > 
> > I didn't think mojo's viewport depends on aura. If it doesn't we do need it.
> 
> Added it back.

[spang, 2014-07-21 22:40:29]

lgtm

[Ben Goodger (Google), 2014-07-23 16:03:43]

https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
File ui/platform_window/win/win_window_factory.h (right):

https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
WinWindowFactory : public PlatformWindowFactory {
Why do we need platform versions of this? Can't we just have a
PlatformWindow::Create() that is defined on each platform? Seems like you could
still have a default factory that calls this and then a test factory that
creates something else.

[sadrul, 2014-07-23 16:40:17]

https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
File ui/platform_window/win/win_window_factory.h (right):

https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
WinWindowFactory : public PlatformWindowFactory {
On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> Why do we need platform versions of this? Can't we just have a
> PlatformWindow::Create() that is defined on each platform? Seems like you
could
> still have a default factory that calls this and then a test factory that
> creates something else.

The problem here is that for ozone, there can't be an implementation of
PlatformWindow::Create() in here (since PlatformWindow is created by an
OzonePlatform implementation in the ozone target, which has an implicit
dependency on platform_window, and so there will be cyclic dependency).

[spang, 2014-07-23 17:35:53]

On 2014/07/23 16:40:17, sadrul wrote:
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> File ui/platform_window/win/win_window_factory.h (right):
> 
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
> WinWindowFactory : public PlatformWindowFactory {
> On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > Why do we need platform versions of this? Can't we just have a
> > PlatformWindow::Create() that is defined on each platform? Seems like you
> could
> > still have a default factory that calls this and then a test factory that
> > creates something else.
> 
> The problem here is that for ozone, there can't be an implementation of
> PlatformWindow::Create() in here (since PlatformWindow is created by an
> OzonePlatform implementation in the ozone target, which has an implicit
> dependency on platform_window, and so there will be cyclic dependency).

We could avoid the factory the same way we did for ui/display: move
platform_window.h into a smaller ui/platform_window/types component. Then you
can add a dep from ui/platform_window to ui/ozone and just call
OzonePlatform::CreatePlatform. I personally think this is nicer than creating
another singleton.

[spang, 2014-07-23 17:44:31]

On 2014/07/23 17:35:53, spang wrote:
> On 2014/07/23 16:40:17, sadrul wrote:
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > File ui/platform_window/win/win_window_factory.h (right):
> > 
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
> > WinWindowFactory : public PlatformWindowFactory {
> > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > Why do we need platform versions of this? Can't we just have a
> > > PlatformWindow::Create() that is defined on each platform? Seems like you
> > could
> > > still have a default factory that calls this and then a test factory that
> > > creates something else.
> > 
> > The problem here is that for ozone, there can't be an implementation of
> > PlatformWindow::Create() in here (since PlatformWindow is created by an
> > OzonePlatform implementation in the ozone target, which has an implicit
> > dependency on platform_window, and so there will be cyclic dependency).
> 
> We could avoid the factory the same way we did for ui/display: move
> platform_window.h into a smaller ui/platform_window/types component. Then you
> can add a dep from ui/platform_window to ui/ozone and just call
> OzonePlatform::CreatePlatform. I personally think this is nicer than creating
> another singleton.

To clarify, for chromium we often use this pattern of defining an interface and
providing a static function to build the correct implementation of that
interface.

When the static function and the interface are in the same component, then this
pattern inherently requires that all implementations also live in that
component, because the static function references the implementation, and the
implementation references the interface, and so having the implementation in a
different component creates a cycle.

So this breaks ozone because we've centralized the platform specific code in a
different component instead of spreading it throughout the tree.

All we need to break the cycle is split ui/platform_window into two pieces, as
we did for ui/display & ui/display/types. Then we can remove the factory.

[Ben Goodger (Google), 2014-07-23 17:53:02]

On 2014/07/23 17:44:31, spang wrote:
> On 2014/07/23 17:35:53, spang wrote:
> > On 2014/07/23 16:40:17, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > File ui/platform_window/win/win_window_factory.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
> > > WinWindowFactory : public PlatformWindowFactory {
> > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > Why do we need platform versions of this? Can't we just have a
> > > > PlatformWindow::Create() that is defined on each platform? Seems like
you
> > > could
> > > > still have a default factory that calls this and then a test factory
that
> > > > creates something else.
> > > 
> > > The problem here is that for ozone, there can't be an implementation of
> > > PlatformWindow::Create() in here (since PlatformWindow is created by an
> > > OzonePlatform implementation in the ozone target, which has an implicit
> > > dependency on platform_window, and so there will be cyclic dependency).
> > 
> > We could avoid the factory the same way we did for ui/display: move
> > platform_window.h into a smaller ui/platform_window/types component. Then
you
> > can add a dep from ui/platform_window to ui/ozone and just call
> > OzonePlatform::CreatePlatform. I personally think this is nicer than
creating
> > another singleton.
> 
> To clarify, for chromium we often use this pattern of defining an interface
and
> providing a static function to build the correct implementation of that
> interface.
> 
> When the static function and the interface are in the same component, then
this
> pattern inherently requires that all implementations also live in that
> component, because the static function references the implementation, and the
> implementation references the interface, and so having the implementation in a
> different component creates a cycle.
> 
> So this breaks ozone because we've centralized the platform specific code in a
> different component instead of spreading it throughout the tree.
> 
> All we need to break the cycle is split ui/platform_window into two pieces, as
> we did for ui/display & ui/display/types. Then we can remove the factory.

sgtm

[sadrul, 2014-07-23 18:51:32]

On 2014/07/23 17:44:31, spang wrote:
> On 2014/07/23 17:35:53, spang wrote:
> > On 2014/07/23 16:40:17, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > File ui/platform_window/win/win_window_factory.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
> > > WinWindowFactory : public PlatformWindowFactory {
> > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > Why do we need platform versions of this? Can't we just have a
> > > > PlatformWindow::Create() that is defined on each platform? Seems like
you
> > > could
> > > > still have a default factory that calls this and then a test factory
that
> > > > creates something else.
> > > 
> > > The problem here is that for ozone, there can't be an implementation of
> > > PlatformWindow::Create() in here (since PlatformWindow is created by an
> > > OzonePlatform implementation in the ozone target, which has an implicit
> > > dependency on platform_window, and so there will be cyclic dependency).
> > 
> > We could avoid the factory the same way we did for ui/display: move
> > platform_window.h into a smaller ui/platform_window/types component. Then
you
> > can add a dep from ui/platform_window to ui/ozone and just call
> > OzonePlatform::CreatePlatform. I personally think this is nicer than
creating
> > another singleton.
> 
> To clarify, for chromium we often use this pattern of defining an interface
and
> providing a static function to build the correct implementation of that
> interface.
> 
> When the static function and the interface are in the same component, then
this
> pattern inherently requires that all implementations also live in that
> component, because the static function references the implementation, and the
> implementation references the interface, and so having the implementation in a
> different component creates a cycle.
> 
> So this breaks ozone because we've centralized the platform specific code in a
> different component instead of spreading it throughout the tree.
> 
> All we need to break the cycle is split ui/platform_window into two pieces, as
> we did for ui/display & ui/display/types. Then we can remove the factory.

If I understand this correctly: PlatformWindow and PlatformWindowDelegate will
be in ui/platform_window/types/ in a 'platform_window_types' target, right? So
PlatformWindow::Create() will have to remain undefined in this target (since the
'win_window' and 'x11_window' targets will depend on 'platform_window_types'
target, and so we cannot implement PlatformWindow::Create() to create a
WinWindow/X11Window from this target)?

[spang, 2014-07-23 21:06:09]

On 2014/07/23 18:51:32, sadrul wrote:
> On 2014/07/23 17:44:31, spang wrote:
> > On 2014/07/23 17:35:53, spang wrote:
> > > On 2014/07/23 16:40:17, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > File ui/platform_window/win/win_window_factory.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > ui/platform_window/win/win_window_factory.h:15: class WIN_WINDOW_EXPORT
> > > > WinWindowFactory : public PlatformWindowFactory {
> > > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > > Why do we need platform versions of this? Can't we just have a
> > > > > PlatformWindow::Create() that is defined on each platform? Seems like
> you
> > > > could
> > > > > still have a default factory that calls this and then a test factory
> that
> > > > > creates something else.
> > > > 
> > > > The problem here is that for ozone, there can't be an implementation of
> > > > PlatformWindow::Create() in here (since PlatformWindow is created by an
> > > > OzonePlatform implementation in the ozone target, which has an implicit
> > > > dependency on platform_window, and so there will be cyclic dependency).
> > > 
> > > We could avoid the factory the same way we did for ui/display: move
> > > platform_window.h into a smaller ui/platform_window/types component. Then
> you
> > > can add a dep from ui/platform_window to ui/ozone and just call
> > > OzonePlatform::CreatePlatform. I personally think this is nicer than
> creating
> > > another singleton.
> > 
> > To clarify, for chromium we often use this pattern of defining an interface
> and
> > providing a static function to build the correct implementation of that
> > interface.
> > 
> > When the static function and the interface are in the same component, then
> this
> > pattern inherently requires that all implementations also live in that
> > component, because the static function references the implementation, and
the
> > implementation references the interface, and so having the implementation in
a
> > different component creates a cycle.
> > 
> > So this breaks ozone because we've centralized the platform specific code in
a
> > different component instead of spreading it throughout the tree.
> > 
> > All we need to break the cycle is split ui/platform_window into two pieces,
as
> > we did for ui/display & ui/display/types. Then we can remove the factory.
> 
> If I understand this correctly: PlatformWindow and PlatformWindowDelegate will
> be in ui/platform_window/types/ in a 'platform_window_types' target, right? So
> PlatformWindow::Create() will have to remain undefined in this target (since
the
> 'win_window' and 'x11_window' targets will depend on 'platform_window_types'
> target, and so we cannot implement PlatformWindow::Create() to create a
> WinWindow/X11Window from this target)?


That's right, I think we could do

x11_window component implements static X11Window::Create (or just expose the
ctor - "new X11Window")

win_window component implements static WinWindow::Create

ozone component implements OzonePlatform::CreatePlatformWindow

platform_window component implements CreateDefaultPlatformWindow() that calls
one of the above based on build platform

Any issues?

[sadrul, 2014-07-23 21:14:08]

On 2014/07/23 21:06:09, spang wrote:
> On 2014/07/23 18:51:32, sadrul wrote:
> > On 2014/07/23 17:44:31, spang wrote:
> > > On 2014/07/23 17:35:53, spang wrote:
> > > > On 2014/07/23 16:40:17, sadrul wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > File ui/platform_window/win/win_window_factory.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > ui/platform_window/win/win_window_factory.h:15: class
WIN_WINDOW_EXPORT
> > > > > WinWindowFactory : public PlatformWindowFactory {
> > > > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > > > Why do we need platform versions of this? Can't we just have a
> > > > > > PlatformWindow::Create() that is defined on each platform? Seems
like
> > you
> > > > > could
> > > > > > still have a default factory that calls this and then a test factory
> > that
> > > > > > creates something else.
> > > > > 
> > > > > The problem here is that for ozone, there can't be an implementation
of
> > > > > PlatformWindow::Create() in here (since PlatformWindow is created by
an
> > > > > OzonePlatform implementation in the ozone target, which has an
implicit
> > > > > dependency on platform_window, and so there will be cyclic
dependency).
> > > > 
> > > > We could avoid the factory the same way we did for ui/display: move
> > > > platform_window.h into a smaller ui/platform_window/types component.
Then
> > you
> > > > can add a dep from ui/platform_window to ui/ozone and just call
> > > > OzonePlatform::CreatePlatform. I personally think this is nicer than
> > creating
> > > > another singleton.
> > > 
> > > To clarify, for chromium we often use this pattern of defining an
interface
> > and
> > > providing a static function to build the correct implementation of that
> > > interface.
> > > 
> > > When the static function and the interface are in the same component, then
> > this
> > > pattern inherently requires that all implementations also live in that
> > > component, because the static function references the implementation, and
> the
> > > implementation references the interface, and so having the implementation
in
> a
> > > different component creates a cycle.
> > > 
> > > So this breaks ozone because we've centralized the platform specific code
in
> a
> > > different component instead of spreading it throughout the tree.
> > > 
> > > All we need to break the cycle is split ui/platform_window into two
pieces,
> as
> > > we did for ui/display & ui/display/types. Then we can remove the factory.
> > 
> > If I understand this correctly: PlatformWindow and PlatformWindowDelegate
will
> > be in ui/platform_window/types/ in a 'platform_window_types' target, right?
So
> > PlatformWindow::Create() will have to remain undefined in this target (since
> the
> > 'win_window' and 'x11_window' targets will depend on 'platform_window_types'
> > target, and so we cannot implement PlatformWindow::Create() to create a
> > WinWindow/X11Window from this target)?
> 
> 
> That's right, I think we could do
> 
> x11_window component implements static X11Window::Create (or just expose the
> ctor - "new X11Window")
> 
> win_window component implements static WinWindow::Create
> 
> ozone component implements OzonePlatform::CreatePlatformWindow
> 
> platform_window component implements CreateDefaultPlatformWindow() that calls
> one of the above based on build platform
> 
> Any issues?

Nope. I just wanted to make sure we could not have a PlatformWindow::Create()
with the _type target.

X11Window and WinWindow already expose their constructors. So I am going to make
the platform_window target include the factory, and the factory is going to have
the platform-dependent code for creating the appropriate version of the
PlatformWindow. (the factory is still going to be useful for overriding in
tests)

[sadrul, 2014-07-24 14:11:23]

On 2014/07/23 21:14:08, sadrul wrote:
> On 2014/07/23 21:06:09, spang wrote:
> > On 2014/07/23 18:51:32, sadrul wrote:
> > > On 2014/07/23 17:44:31, spang wrote:
> > > > On 2014/07/23 17:35:53, spang wrote:
> > > > > On 2014/07/23 16:40:17, sadrul wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > > File ui/platform_window/win/win_window_factory.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > > ui/platform_window/win/win_window_factory.h:15: class
> WIN_WINDOW_EXPORT
> > > > > > WinWindowFactory : public PlatformWindowFactory {
> > > > > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > > > > Why do we need platform versions of this? Can't we just have a
> > > > > > > PlatformWindow::Create() that is defined on each platform? Seems
> like
> > > you
> > > > > > could
> > > > > > > still have a default factory that calls this and then a test
factory
> > > that
> > > > > > > creates something else.
> > > > > > 
> > > > > > The problem here is that for ozone, there can't be an implementation
> of
> > > > > > PlatformWindow::Create() in here (since PlatformWindow is created by
> an
> > > > > > OzonePlatform implementation in the ozone target, which has an
> implicit
> > > > > > dependency on platform_window, and so there will be cyclic
> dependency).
> > > > > 
> > > > > We could avoid the factory the same way we did for ui/display: move
> > > > > platform_window.h into a smaller ui/platform_window/types component.
> Then
> > > you
> > > > > can add a dep from ui/platform_window to ui/ozone and just call
> > > > > OzonePlatform::CreatePlatform. I personally think this is nicer than
> > > creating
> > > > > another singleton.
> > > > 
> > > > To clarify, for chromium we often use this pattern of defining an
> interface
> > > and
> > > > providing a static function to build the correct implementation of that
> > > > interface.
> > > > 
> > > > When the static function and the interface are in the same component,
then
> > > this
> > > > pattern inherently requires that all implementations also live in that
> > > > component, because the static function references the implementation,
and
> > the
> > > > implementation references the interface, and so having the
implementation
> in
> > a
> > > > different component creates a cycle.
> > > > 
> > > > So this breaks ozone because we've centralized the platform specific
code
> in
> > a
> > > > different component instead of spreading it throughout the tree.
> > > > 
> > > > All we need to break the cycle is split ui/platform_window into two
> pieces,
> > as
> > > > we did for ui/display & ui/display/types. Then we can remove the
factory.
> > > 
> > > If I understand this correctly: PlatformWindow and PlatformWindowDelegate
> will
> > > be in ui/platform_window/types/ in a 'platform_window_types' target,
right?
> So
> > > PlatformWindow::Create() will have to remain undefined in this target
(since
> > the
> > > 'win_window' and 'x11_window' targets will depend on
'platform_window_types'
> > > target, and so we cannot implement PlatformWindow::Create() to create a
> > > WinWindow/X11Window from this target)?
> > 
> > 
> > That's right, I think we could do
> > 
> > x11_window component implements static X11Window::Create (or just expose the
> > ctor - "new X11Window")
> > 
> > win_window component implements static WinWindow::Create
> > 
> > ozone component implements OzonePlatform::CreatePlatformWindow
> > 
> > platform_window component implements CreateDefaultPlatformWindow() that
calls
> > one of the above based on build platform
> > 
> > Any issues?
> 
> Nope. I just wanted to make sure we could not have a PlatformWindow::Create()
> with the _type target.
> 
> X11Window and WinWindow already expose their constructors. So I am going to
make
> the platform_window target include the factory, and the factory is going to
have
> the platform-dependent code for creating the appropriate version of the
> PlatformWindow. (the factory is still going to be useful for overriding in
> tests)

Done. PTAL

[spang, 2014-07-24 19:23:24]

On 2014/07/24 14:11:23, sadrul wrote:
> On 2014/07/23 21:14:08, sadrul wrote:
> > On 2014/07/23 21:06:09, spang wrote:
> > > On 2014/07/23 18:51:32, sadrul wrote:
> > > > On 2014/07/23 17:44:31, spang wrote:
> > > > > On 2014/07/23 17:35:53, spang wrote:
> > > > > > On 2014/07/23 16:40:17, sadrul wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > > > File ui/platform_window/win/win_window_factory.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/403263002/diff/100001/ui/platform_window/win/...
> > > > > > > ui/platform_window/win/win_window_factory.h:15: class
> > WIN_WINDOW_EXPORT
> > > > > > > WinWindowFactory : public PlatformWindowFactory {
> > > > > > > On 2014/07/23 16:03:43, Ben Goodger (Google) wrote:
> > > > > > > > Why do we need platform versions of this? Can't we just have a
> > > > > > > > PlatformWindow::Create() that is defined on each platform? Seems
> > like
> > > > you
> > > > > > > could
> > > > > > > > still have a default factory that calls this and then a test
> factory
> > > > that
> > > > > > > > creates something else.
> > > > > > > 
> > > > > > > The problem here is that for ozone, there can't be an
implementation
> > of
> > > > > > > PlatformWindow::Create() in here (since PlatformWindow is created
by
> > an
> > > > > > > OzonePlatform implementation in the ozone target, which has an
> > implicit
> > > > > > > dependency on platform_window, and so there will be cyclic
> > dependency).
> > > > > > 
> > > > > > We could avoid the factory the same way we did for ui/display: move
> > > > > > platform_window.h into a smaller ui/platform_window/types component.
> > Then
> > > > you
> > > > > > can add a dep from ui/platform_window to ui/ozone and just call
> > > > > > OzonePlatform::CreatePlatform. I personally think this is nicer than
> > > > creating
> > > > > > another singleton.
> > > > > 
> > > > > To clarify, for chromium we often use this pattern of defining an
> > interface
> > > > and
> > > > > providing a static function to build the correct implementation of
that
> > > > > interface.
> > > > > 
> > > > > When the static function and the interface are in the same component,
> then
> > > > this
> > > > > pattern inherently requires that all implementations also live in that
> > > > > component, because the static function references the implementation,
> and
> > > the
> > > > > implementation references the interface, and so having the
> implementation
> > in
> > > a
> > > > > different component creates a cycle.
> > > > > 
> > > > > So this breaks ozone because we've centralized the platform specific
> code
> > in
> > > a
> > > > > different component instead of spreading it throughout the tree.
> > > > > 
> > > > > All we need to break the cycle is split ui/platform_window into two
> > pieces,
> > > as
> > > > > we did for ui/display & ui/display/types. Then we can remove the
> factory.
> > > > 
> > > > If I understand this correctly: PlatformWindow and
PlatformWindowDelegate
> > will
> > > > be in ui/platform_window/types/ in a 'platform_window_types' target,
> right?
> > So
> > > > PlatformWindow::Create() will have to remain undefined in this target
> (since
> > > the
> > > > 'win_window' and 'x11_window' targets will depend on
> 'platform_window_types'
> > > > target, and so we cannot implement PlatformWindow::Create() to create a
> > > > WinWindow/X11Window from this target)?
> > > 
> > > 
> > > That's right, I think we could do
> > > 
> > > x11_window component implements static X11Window::Create (or just expose
the
> > > ctor - "new X11Window")
> > > 
> > > win_window component implements static WinWindow::Create
> > > 
> > > ozone component implements OzonePlatform::CreatePlatformWindow
> > > 
> > > platform_window component implements CreateDefaultPlatformWindow() that
> calls
> > > one of the above based on build platform
> > > 
> > > Any issues?
> > 
> > Nope. I just wanted to make sure we could not have a
PlatformWindow::Create()
> > with the _type target.
> > 
> > X11Window and WinWindow already expose their constructors. So I am going to
> make
> > the platform_window target include the factory, and the factory is going to
> have
> > the platform-dependent code for creating the appropriate version of the
> > PlatformWindow. (the factory is still going to be useful for overriding in
> > tests)
> 
> Done. PTAL

thanks. lgtm

I do like this better although I wonder if the tests couldn't be made to work
without overriding the constructor.

[spang, 2014-07-24 19:23:36]

https://codereview.chromium.org/403263002/diff/320001/ui/platform_window/DEPS
File ui/platform_window/DEPS (right):

https://codereview.chromium.org/403263002/diff/320001/ui/platform_window/DEPS...
ui/platform_window/DEPS:4: "+ui/ozone",
ui/ozone/public

[sadrul, 2014-07-24 20:37:34]

https://codereview.chromium.org/403263002/diff/320001/ui/platform_window/DEPS
File ui/platform_window/DEPS (right):

https://codereview.chromium.org/403263002/diff/320001/ui/platform_window/DEPS...
ui/platform_window/DEPS:4: "+ui/ozone",
On 2014/07/24 19:23:36, spang wrote:
> ui/ozone/public

Whoops. Done.

[sadrul, 2014-07-29 04:19:17]

ben@: ping

[Ben Goodger (Google), 2014-07-31 20:21:53]

On 2014/07/29 04:19:17, sadrul wrote:
> ben@: ping

What didn't work out about splitting the types? I object to adding more
singletons.

[spang, 2014-07-31 20:34:21]

On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> On 2014/07/29 04:19:17, sadrul wrote:
> > ben@: ping
> 
> What didn't work out about splitting the types? I object to adding more
> singletons.

This patch will actually work OK if we convert the factory to a (stateless)
static function. The dependency problems are fixed by tsplit. We should do that.

As for tests,

[sadrul, 2014-07-31 20:35:52]

On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> On 2014/07/29 04:19:17, sadrul wrote:
> > ben@: ping
> 
> What didn't work out about splitting the types? I object to adding more
> singletons.

The latest patchset does split out the types into its own target. However, this
still doesn't allow having a PlatformWindow::Create (because for ozone, its
implementation would depend on the 'ozone' target (which would have to depend on
the '_types' target, causing cycling dependency)). We discussed this a little in
earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
https://codereview.chromium.org/403263002/#msg17
https://codereview.chromium.org/403263002/#msg18

[spang, 2014-07-31 20:36:14]

On 2014/07/31 20:34:21, spang wrote:
> On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > On 2014/07/29 04:19:17, sadrul wrote:
> > > ben@: ping
> > 
> > What didn't work out about splitting the types? I object to adding more
> > singletons.
> 
> This patch will actually work OK if we convert the factory to a (stateless)
> static function. The dependency problems are fixed by tsplit. We should do
that.
> 
> As for tests,

As for tests, we should be able to avoid taking this platform-specific path when
we actually want a mock window,

[spang, 2014-07-31 20:46:33]

On 2014/07/31 20:35:52, sadrul wrote:
> On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > On 2014/07/29 04:19:17, sadrul wrote:
> > > ben@: ping
> > 
> > What didn't work out about splitting the types? I object to adding more
> > singletons.
> 
> The latest patchset does split out the types into its own target. However,
this
> still doesn't allow having a PlatformWindow::Create (because for ozone, its
> implementation would depend on the 'ozone' target (which would have to depend
on
> the '_types' target, causing cycling dependency)). We discussed this a little
in
> earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> https://codereview.chromium.org/403263002/#msg17
> https://codereview.chromium.org/403263002/#msg18

Er, I guess we're not on the same page.

If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
something like:

scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
    PlatformWindowDelegate* delegate,
    const gfx::Rect& bounds);

Then that will work. No singleton or global state is necessary.

[sadrul, 2014-07-31 20:54:10]

On 2014/07/31 20:46:33, spang wrote:
> On 2014/07/31 20:35:52, sadrul wrote:
> > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > ben@: ping
> > > 
> > > What didn't work out about splitting the types? I object to adding more
> > > singletons.
> > 
> > The latest patchset does split out the types into its own target. However,
> this
> > still doesn't allow having a PlatformWindow::Create (because for ozone, its
> > implementation would depend on the 'ozone' target (which would have to
depend
> on
> > the '_types' target, causing cycling dependency)). We discussed this a
little
> in
> > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > https://codereview.chromium.org/403263002/#msg17
> > https://codereview.chromium.org/403263002/#msg18
> 
> Er, I guess we're not on the same page.
> 
> If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
> something like:
> 
> scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
>     PlatformWindowDelegate* delegate,
>     const gfx::Rect& bounds);
> 
> Then that will work. No singleton or global state is necessary.

The DefaultPlatformWindowFactory creates and owns the PlatformEventSource. We do
need that for X11.

[spang, 2014-07-31 21:07:45]

On 2014/07/31 20:54:10, sadrul wrote:
> On 2014/07/31 20:46:33, spang wrote:
> > On 2014/07/31 20:35:52, sadrul wrote:
> > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > > ben@: ping
> > > > 
> > > > What didn't work out about splitting the types? I object to adding more
> > > > singletons.
> > > 
> > > The latest patchset does split out the types into its own target. However,
> > this
> > > still doesn't allow having a PlatformWindow::Create (because for ozone,
its
> > > implementation would depend on the 'ozone' target (which would have to
> depend
> > on
> > > the '_types' target, causing cycling dependency)). We discussed this a
> little
> > in
> > > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > > https://codereview.chromium.org/403263002/#msg17
> > > https://codereview.chromium.org/403263002/#msg18
> > 
> > Er, I guess we're not on the same page.
> > 
> > If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
> > something like:
> > 
> > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> >     PlatformWindowDelegate* delegate,
> >     const gfx::Rect& bounds);
> > 
> > Then that will work. No singleton or global state is necessary.
> 
> The DefaultPlatformWindowFactory creates and owns the PlatformEventSource. We
do
> need that for X11.

Ok, that is a real issue. Maybe you could add & call

X11EventSource::Initialize()

in the X11 version of CreateDefaultPlatformWindow?

Since PlatformEventSource is already global, doing it that way isn't adding more
global state.

[sadrul, 2014-07-31 21:35:57]

On 2014/07/31 21:07:45, spang wrote:
> On 2014/07/31 20:54:10, sadrul wrote:
> > On 2014/07/31 20:46:33, spang wrote:
> > > On 2014/07/31 20:35:52, sadrul wrote:
> > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > > > ben@: ping
> > > > > 
> > > > > What didn't work out about splitting the types? I object to adding
more
> > > > > singletons.
> > > > 
> > > > The latest patchset does split out the types into its own target.
However,
> > > this
> > > > still doesn't allow having a PlatformWindow::Create (because for ozone,
> its
> > > > implementation would depend on the 'ozone' target (which would have to
> > depend
> > > on
> > > > the '_types' target, causing cycling dependency)). We discussed this a
> > little
> > > in
> > > > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > > > https://codereview.chromium.org/403263002/#msg17
> > > > https://codereview.chromium.org/403263002/#msg18
> > > 
> > > Er, I guess we're not on the same page.
> > > 
> > > If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
> > > something like:
> > > 
> > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> > >     PlatformWindowDelegate* delegate,
> > >     const gfx::Rect& bounds);
> > > 
> > > Then that will work. No singleton or global state is necessary.
> > 
> > The DefaultPlatformWindowFactory creates and owns the PlatformEventSource.
We
> do
> > need that for X11.
> 
> Ok, that is a real issue. Maybe you could add & call
> 
> X11EventSource::Initialize()
> 
> in the X11 version of CreateDefaultPlatformWindow?
> 
> Since PlatformEventSource is already global, doing it that way isn't adding
more
> global state.

There's currently an owner of PlatformEventSource (usually aura::Env). Is your
suggestion that we remove that, and let the PES leak, or clean itself up upon
exit?

I am still unclear on how we are going to create the mock PlatformWindow during
tests. Consider a test that indirectly calls WindowTreeHost::Create(), or
NativeViewport::Create(). The implementation of this ::Create() calls into
CreateDefaultPlatformWindow() ... and we detect that we are running a test and
create the mock PlatformWindow in there? Or we detect that we are running a test
in the ::Create() functions, and not call CreateDefaultPlatformWindow()?

[sadrul, 2014-07-31 21:36:39]

On 2014/07/31 21:35:57, sadrul wrote:
> On 2014/07/31 21:07:45, spang wrote:
> > On 2014/07/31 20:54:10, sadrul wrote:
> > > On 2014/07/31 20:46:33, spang wrote:
> > > > On 2014/07/31 20:35:52, sadrul wrote:
> > > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > > > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > > > > ben@: ping
> > > > > > 
> > > > > > What didn't work out about splitting the types? I object to adding
> more
> > > > > > singletons.
> > > > > 
> > > > > The latest patchset does split out the types into its own target.
> However,
> > > > this
> > > > > still doesn't allow having a PlatformWindow::Create (because for
ozone,
> > its
> > > > > implementation would depend on the 'ozone' target (which would have to
> > > depend
> > > > on
> > > > > the '_types' target, causing cycling dependency)). We discussed this a
> > > little
> > > > in
> > > > > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > > > > https://codereview.chromium.org/403263002/#msg17
> > > > > https://codereview.chromium.org/403263002/#msg18
> > > > 
> > > > Er, I guess we're not on the same page.
> > > > 
> > > > If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
> > > > something like:
> > > > 
> > > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> > > >     PlatformWindowDelegate* delegate,
> > > >     const gfx::Rect& bounds);
> > > > 
> > > > Then that will work. No singleton or global state is necessary.
> > > 
> > > The DefaultPlatformWindowFactory creates and owns the PlatformEventSource.
> We
> > do
> > > need that for X11.
> > 
> > Ok, that is a real issue. Maybe you could add & call
> > 
> > X11EventSource::Initialize()
> > 
> > in the X11 version of CreateDefaultPlatformWindow?
> > 
> > Since PlatformEventSource is already global, doing it that way isn't adding
> more
> > global state.
> 
> There's currently an owner of PlatformEventSource (usually aura::Env). Is your
> suggestion that we remove that, and let the PES leak, or clean itself up upon
> exit?
> 
> I am still unclear on how we are going to create the mock PlatformWindow
during
> tests.

+ "without the factory"

> Consider a test that indirectly calls WindowTreeHost::Create(), or
> NativeViewport::Create(). The implementation of this ::Create() calls into
> CreateDefaultPlatformWindow() ... and we detect that we are running a test and
> create the mock PlatformWindow in there? Or we detect that we are running a
test
> in the ::Create() functions, and not call CreateDefaultPlatformWindow()?

[spang, 2014-08-01 17:34:44]

On 2014/07/31 21:35:57, sadrul wrote:
> On 2014/07/31 21:07:45, spang wrote:
> > On 2014/07/31 20:54:10, sadrul wrote:
> > > On 2014/07/31 20:46:33, spang wrote:
> > > > On 2014/07/31 20:35:52, sadrul wrote:
> > > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > > > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > > > > ben@: ping
> > > > > > 
> > > > > > What didn't work out about splitting the types? I object to adding
> more
> > > > > > singletons.
> > > > > 
> > > > > The latest patchset does split out the types into its own target.
> However,
> > > > this
> > > > > still doesn't allow having a PlatformWindow::Create (because for
ozone,
> > its
> > > > > implementation would depend on the 'ozone' target (which would have to
> > > depend
> > > > on
> > > > > the '_types' target, causing cycling dependency)). We discussed this a
> > > little
> > > > in
> > > > > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > > > > https://codereview.chromium.org/403263002/#msg17
> > > > > https://codereview.chromium.org/403263002/#msg18
> > > > 
> > > > Er, I guess we're not on the same page.
> > > > 
> > > > If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow to
> > > > something like:
> > > > 
> > > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> > > >     PlatformWindowDelegate* delegate,
> > > >     const gfx::Rect& bounds);
> > > > 
> > > > Then that will work. No singleton or global state is necessary.
> > > 
> > > The DefaultPlatformWindowFactory creates and owns the PlatformEventSource.
> We
> > do
> > > need that for X11.
> > 
> > Ok, that is a real issue. Maybe you could add & call
> > 
> > X11EventSource::Initialize()
> > 
> > in the X11 version of CreateDefaultPlatformWindow?
> > 
> > Since PlatformEventSource is already global, doing it that way isn't adding
> more
> > global state.
> 
> There's currently an owner of PlatformEventSource (usually aura::Env). Is your
> suggestion that we remove that, and let the PES leak, or clean itself up upon
> exit?

You could have it register on AtExitManager instead of owning it on
PlatformWindowFactory, which itself uses AtExitManager. This is still a step
toward reducing (or at least not creating more) singletons.

What are the tests doing now? If we need tPlatformWindowFactory 

>  
> I am still unclear on how we are going to create the mock PlatformWindow
during
> tests. Consider a test that indirectly calls WindowTreeHost::Create(), or
> NativeViewport::Create(). The implementation of this ::Create() calls into
> CreateDefaultPlatformWindow() ... and we detect that we are running a test and
> create the mock PlatformWindow in there? Or we detect that we are running a
test
> in the ::Create() functions, and not call CreateDefaultPlatformWindow()?

[spang, 2014-08-01 17:38:05]

On 2014/08/01 17:34:44, spang wrote:
> On 2014/07/31 21:35:57, sadrul wrote:
> > On 2014/07/31 21:07:45, spang wrote:
> > > On 2014/07/31 20:54:10, sadrul wrote:
> > > > On 2014/07/31 20:46:33, spang wrote:
> > > > > On 2014/07/31 20:35:52, sadrul wrote:
> > > > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> > > > > > > On 2014/07/29 04:19:17, sadrul wrote:
> > > > > > > > ben@: ping
> > > > > > > 
> > > > > > > What didn't work out about splitting the types? I object to adding
> > more
> > > > > > > singletons.
> > > > > > 
> > > > > > The latest patchset does split out the types into its own target.
> > However,
> > > > > this
> > > > > > still doesn't allow having a PlatformWindow::Create (because for
> ozone,
> > > its
> > > > > > implementation would depend on the 'ozone' target (which would have
to
> > > > depend
> > > > > on
> > > > > > the '_types' target, causing cycling dependency)). We discussed this
a
> > > > little
> > > > > in
> > > > > > earlier comments: https://codereview.chromium.org/403263002/#msg16 ,
> > > > > > https://codereview.chromium.org/403263002/#msg17
> > > > > > https://codereview.chromium.org/403263002/#msg18
> > > > > 
> > > > > Er, I guess we're not on the same page.
> > > > > 
> > > > > If you rename your DefaultPlatformWindowFactory::CreatePlatformWindow
to
> > > > > something like:
> > > > > 
> > > > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> > > > >     PlatformWindowDelegate* delegate,
> > > > >     const gfx::Rect& bounds);
> > > > > 
> > > > > Then that will work. No singleton or global state is necessary.
> > > > 
> > > > The DefaultPlatformWindowFactory creates and owns the
PlatformEventSource.
> > We
> > > do
> > > > need that for X11.
> > > 
> > > Ok, that is a real issue. Maybe you could add & call
> > > 
> > > X11EventSource::Initialize()
> > > 
> > > in the X11 version of CreateDefaultPlatformWindow?
> > > 
> > > Since PlatformEventSource is already global, doing it that way isn't
adding
> > more
> > > global state.
> > 
> > There's currently an owner of PlatformEventSource (usually aura::Env). Is
your
> > suggestion that we remove that, and let the PES leak, or clean itself up
upon
> > exit?
> 
> You could have it register on AtExitManager instead of owning it on
> PlatformWindowFactory, which itself uses AtExitManager. This is still a step
> toward reducing (or at least not creating more) singletons.
> 
> What are the tests doing now? If we need tPlatformWindowFactory 

Whoops.

If we need a PlatformWindowFactory interface so we can pass one to the
WindowTreeHost constructor in tests, then maybe you could keep the class but own
tfactory on

> 
> >  
> > I am still unclear on how we are going to create the mock PlatformWindow
> during
> > tests. Consider a test that indirectly calls WindowTreeHost::Create(), or
> > NativeViewport::Create(). The implementation of this ::Create() calls into
> > CreateDefaultPlatformWindow() ... and we detect that we are running a test
and
> > create the mock PlatformWindow in there? Or we detect that we are running a
> test
> > in the ::Create() functions, and not call CreateDefaultPlatformWindow()?

[spang, 2014-08-01 17:48:03]

On Fri, Aug 1, 2014 at 1:38 PM,  <spang@chromium.org> wrote:
> On 2014/08/01 17:34:44, spang wrote:
>>
>> On 2014/07/31 21:35:57, sadrul wrote:
>> > On 2014/07/31 21:07:45, spang wrote:
>> > > On 2014/07/31 20:54:10, sadrul wrote:
>> > > > On 2014/07/31 20:46:33, spang wrote:
>> > > > > On 2014/07/31 20:35:52, sadrul wrote:
>> > > > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
>> > > > > > > On 2014/07/29 04:19:17, sadrul wrote:
>> > > > > > > > ben@: ping
>> > > > > > >
>> > > > > > > What didn't work out about splitting the types? I object to
>> > > > > > > adding
>> > more
>> > > > > > > singletons.
>> > > > > >
>> > > > > > The latest patchset does split out the types into its own
>> > > > > > target.
>> > However,
>> > > > > this
>> > > > > > still doesn't allow having a PlatformWindow::Create (because for
>> ozone,
>> > > its
>> > > > > > implementation would depend on the 'ozone' target (which would
>> > > > > > have
>
> to
>>
>> > > > depend
>> > > > > on
>> > > > > > the '_types' target, causing cycling dependency)). We discussed
>> > > > > > this
>
> a
>>
>> > > > little
>> > > > > in
>> > > > > > earlier comments:
>> > > > > > https://codereview.chromium.org/403263002/#msg16 ,
>> > > > > > https://codereview.chromium.org/403263002/#msg17
>> > > > > > https://codereview.chromium.org/403263002/#msg18
>> > > > >
>> > > > > Er, I guess we're not on the same page.
>> > > > >
>> > > > > If you rename your
>> > > > > DefaultPlatformWindowFactory::CreatePlatformWindow
>
> to
>>
>> > > > > something like:
>> > > > >
>> > > > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
>> > > > >     PlatformWindowDelegate* delegate,
>> > > > >     const gfx::Rect& bounds);
>> > > > >
>> > > > > Then that will work. No singleton or global state is necessary.
>> > > >
>> > > > The DefaultPlatformWindowFactory creates and owns the
>
> PlatformEventSource.
>>
>> > We
>> > > do
>> > > > need that for X11.
>> > >
>> > > Ok, that is a real issue. Maybe you could add & call
>> > >
>> > > X11EventSource::Initialize()
>> > >
>> > > in the X11 version of CreateDefaultPlatformWindow?
>> > >
>> > > Since PlatformEventSource is already global, doing it that way isn't
>
> adding
>>
>> > more
>> > > global state.
>> >
>> > There's currently an owner of PlatformEventSource (usually aura::Env).
>> > Is
>
> your
>>
>> > suggestion that we remove that, and let the PES leak, or clean itself up
>
> upon
>>
>> > exit?
>
>
>> You could have it register on AtExitManager instead of owning it on
>> PlatformWindowFactory, which itself uses AtExitManager. This is still a
>> step
>> toward reducing (or at least not creating more) singletons.
>
>
>> What are the tests doing now? If we need tPlatformWindowFactory
>
>
> Whoops.
>
> If we need a PlatformWindowFactory interface so we can pass one to the
> WindowTreeHost constructor in tests, then maybe you could keep the class but
> own
> tfactory on

I'm not having a good day with my new alternative layout keyboard.

If we need a PlatformWindowFactory interface so we can pass one to the
WindowTreeHost constructor in tests, then maybe you could keep the class but
own one from aura's Env and another from mojo's native viewport
service, rather than as a singleton.

>
>
>
>> >
>> > I am still unclear on how we are going to create the mock PlatformWindow
>> during
>> > tests. Consider a test that indirectly calls WindowTreeHost::Create(),
>> > or
>> > NativeViewport::Create(). The implementation of this ::Create() calls
>> > into
>> > CreateDefaultPlatformWindow() ... and we detect that we are running a
>> > test
>
> and
>>
>> > create the mock PlatformWindow in there? Or we detect that we are
>> > running a
>> test
>> > in the ::Create() functions, and not call CreateDefaultPlatformWindow()?
>
>
>
>
> https://codereview.chromium.org/403263002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rjkroege, 2015-08-13 18:15:26]

sadrul: kill it or finish it?

On 2014/08/01 17:48:03, spang wrote:
> On Fri, Aug 1, 2014 at 1:38 PM,  <mailto:spang@chromium.org> wrote:
> > On 2014/08/01 17:34:44, spang wrote:
> >>
> >> On 2014/07/31 21:35:57, sadrul wrote:
> >> > On 2014/07/31 21:07:45, spang wrote:
> >> > > On 2014/07/31 20:54:10, sadrul wrote:
> >> > > > On 2014/07/31 20:46:33, spang wrote:
> >> > > > > On 2014/07/31 20:35:52, sadrul wrote:
> >> > > > > > On 2014/07/31 20:21:53, Ben Goodger (Google) wrote:
> >> > > > > > > On 2014/07/29 04:19:17, sadrul wrote:
> >> > > > > > > > ben@: ping
> >> > > > > > >
> >> > > > > > > What didn't work out about splitting the types? I object to
> >> > > > > > > adding
> >> > more
> >> > > > > > > singletons.
> >> > > > > >
> >> > > > > > The latest patchset does split out the types into its own
> >> > > > > > target.
> >> > However,
> >> > > > > this
> >> > > > > > still doesn't allow having a PlatformWindow::Create (because for
> >> ozone,
> >> > > its
> >> > > > > > implementation would depend on the 'ozone' target (which would
> >> > > > > > have
> >
> > to
> >>
> >> > > > depend
> >> > > > > on
> >> > > > > > the '_types' target, causing cycling dependency)). We discussed
> >> > > > > > this
> >
> > a
> >>
> >> > > > little
> >> > > > > in
> >> > > > > > earlier comments:
> >> > > > > > https://codereview.chromium.org/403263002/#msg16 ,
> >> > > > > > https://codereview.chromium.org/403263002/#msg17
> >> > > > > > https://codereview.chromium.org/403263002/#msg18
> >> > > > >
> >> > > > > Er, I guess we're not on the same page.
> >> > > > >
> >> > > > > If you rename your
> >> > > > > DefaultPlatformWindowFactory::CreatePlatformWindow
> >
> > to
> >>
> >> > > > > something like:
> >> > > > >
> >> > > > > scoped_ptr<PlatformWindow> CreateDefaultPlatformWindow(
> >> > > > >     PlatformWindowDelegate* delegate,
> >> > > > >     const gfx::Rect& bounds);
> >> > > > >
> >> > > > > Then that will work. No singleton or global state is necessary.
> >> > > >
> >> > > > The DefaultPlatformWindowFactory creates and owns the
> >
> > PlatformEventSource.
> >>
> >> > We
> >> > > do
> >> > > > need that for X11.
> >> > >
> >> > > Ok, that is a real issue. Maybe you could add & call
> >> > >
> >> > > X11EventSource::Initialize()
> >> > >
> >> > > in the X11 version of CreateDefaultPlatformWindow?
> >> > >
> >> > > Since PlatformEventSource is already global, doing it that way isn't
> >
> > adding
> >>
> >> > more
> >> > > global state.
> >> >
> >> > There's currently an owner of PlatformEventSource (usually aura::Env).
> >> > Is
> >
> > your
> >>
> >> > suggestion that we remove that, and let the PES leak, or clean itself up
> >
> > upon
> >>
> >> > exit?
> >
> >
> >> You could have it register on AtExitManager instead of owning it on
> >> PlatformWindowFactory, which itself uses AtExitManager. This is still a
> >> step
> >> toward reducing (or at least not creating more) singletons.
> >
> >
> >> What are the tests doing now? If we need tPlatformWindowFactory
> >
> >
> > Whoops.
> >
> > If we need a PlatformWindowFactory interface so we can pass one to the
> > WindowTreeHost constructor in tests, then maybe you could keep the class but
> > own
> > tfactory on
> 
> I'm not having a good day with my new alternative layout keyboard.
> 
> If we need a PlatformWindowFactory interface so we can pass one to the
> WindowTreeHost constructor in tests, then maybe you could keep the class but
> own one from aura's Env and another from mojo's native viewport
> service, rather than as a singleton.
> 
> >
> >
> >
> >> >
> >> > I am still unclear on how we are going to create the mock PlatformWindow
> >> during
> >> > tests. Consider a test that indirectly calls WindowTreeHost::Create(),
> >> > or
> >> > NativeViewport::Create(). The implementation of this ::Create() calls
> >> > into
> >> > CreateDefaultPlatformWindow() ... and we detect that we are running a
> >> > test
> >
> > and
> >>
> >> > create the mock PlatformWindow in there? Or we detect that we are
> >> > running a
> >> test
> >> > in the ::Create() functions, and not call CreateDefaultPlatformWindow()?
> >
> >
> >
> >
> > https://codereview.chromium.org/403263002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2015-08-13 18:31:50]

On 2015/08/13 18:15:26, rjkroege wrote:
> sadrul: kill it or finish it?

view_manager has already been cleaned up
(https://codereview.chromium.org/1180573004,
https://codereview.chromium.org/1216113004), without using a factory. Similar
change can be made for WindowTreeHost* in aura (would reduce some code dup, and
avoid more dup for android-aura: https://codereview.chromium.org/1284343005/).

[sadrul, 2015-08-13 18:32:05]

sadrul@chromium.org changed reviewers:
- ben@chromium.org, spang@chromium.org