Implement OzonePlatform

Implement OzonePlatform

This provides a way to select an ozone implementation to use at build time.
It replaces the previous ad-hoc requirement to inject implementations of
ozone interfaces somewhere during initialization, such as by overriding
ContentMainDelegate::PreSandboxStartup(). That requirement made it
difficult for external ozone implementations to build internal targets such
as content_shell because those targets do not initialize the external ozone
implementation without additional patching. Enabling external ports of
chromium is one of the main goals of ozone.

The OzonePlatform code is located at ui/ozone and depends on code in
ui/gfx and ui/events because it must inject implementations into
those components. The ozone platform is initialized from
ui/aura or ui/gl, as those components need the interfaces provided by ozone
in order to function.

There are two in-tree platforms currently: test (image dump) and dri
(libdrm-based direct rendering). The platform is selected by the setting
ozone_platform gyp variable and defaults to "test".

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

[spang, 2013-10-25 18:00:10]

This is the single-platform version. Multi-platform is a followup CL.

[spang, 2013-10-25 18:01:50]

CC dnicoara, sadrul, ozone-wayland

[rjkroege, 2013-10-25 18:18:15]

On 2013/10/25 18:01:50, spang wrote:
> CC dnicoara, sadrul, ozone-wayland

I think it should be ui/base/ozone as with ui/base/x ui/base/win etc.?

[rjkroege, 2013-10-25 18:24:02]

https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.cc
File ui/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.cc#ne...
ui/ozone/ozone_platform.cc:14: gfx::SurfaceFactoryOzone::SetInstance(NULL);
I will be curious if this ever results in a null impl_ in practice. A fun race
bug for someone perhaps.

https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h
File ui/ozone/ozone_platform.h (right):

https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h#new...
ui/ozone/ozone_platform.h:15: class OZONE_EXPORT OzonePlatform {
this class is part of the external ozone api. it needs more comment

[kalyank, 2013-10-25 18:57:15]

Note: I am still getting familiar with gyp :)

In ozone.gyp, we include platform specific files. I am wondering, if we could
separate them to their own gyp files and include something like platform.gyp as
part of dependency in ozone.gyp. This would make it possible to include
ozone-implementations which are not part of the chromium tree. 

What I mean is:

 'dependencies': [	
	'<(DEPTH)/base/base.gyp:base',	
	'<(DEPTH)/ui/gfx/gfx.gyp:gfx',	
	'<(DEPTH)/skia/skia.gyp:skia',
        'platform/<value of ozone_platform gyp variable>/<value of
ozone_platform gyp variable>.gyp'
         ],

[spang, 2013-10-25 19:02:54]

On 2013/10/25 18:57:15, kalyank wrote:
> Note: I am still getting familiar with gyp :)
> 
> In ozone.gyp, we include platform specific files. I am wondering, if we could
> separate them to their own gyp files and include something like platform.gyp
as
> part of dependency in ozone.gyp. This would make it possible to include
> ozone-implementations which are not part of the chromium tree. 
> 

kalyank, don't worry, there is a way to do what you need without creating a
separate target for each platform. I'll send a patch against ozone-wayland
showing how before this lands.

[kalyank, 2013-10-25 19:08:26]

On 2013/10/25 19:02:54, spang wrote:
> 
> kalyank, don't worry, there is a way to do what you need without creating a
> separate target for each platform. I'll send a patch against ozone-wayland
> showing how before this lands.

spang, cool, thanks.

[spang, 2013-10-25 19:14:27]

On 2013/10/25 18:24:02, rjkroege wrote:
> https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.cc
> File ui/ozone/ozone_platform.cc (right):
> 
>
https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.cc#ne...
> ui/ozone/ozone_platform.cc:14: gfx::SurfaceFactoryOzone::SetInstance(NULL);
> I will be curious if this ever results in a null impl_ in practice. A fun race
> bug for someone perhaps.

If it results in a segfault then I will be glad since the object is already
destroyed. We are not calling GetInstance() very often though so it probably
makes little difference.

> 
> https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h
> File ui/ozone/ozone_platform.h (right):
> 
>
https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h#new...
> ui/ozone/ozone_platform.h:15: class OZONE_EXPORT OzonePlatform {
> this class is part of the external ozone api. it needs more comment

[spang, 2013-10-25 19:24:25]

On 2013/10/25 19:08:26, kalyank wrote:
> On 2013/10/25 19:02:54, spang wrote:
> > 
> > kalyank, don't worry, there is a way to do what you need without creating a
> > separate target for each platform. I'll send a patch against ozone-wayland
> > showing how before this lands.
> 
> spang, cool, thanks.

Actually you were right, this file is actually missing a necessary piece. We
need to provide a gyp list you can merge your files into.

[kalyank, 2013-10-25 19:34:24]

On 2013/10/25 19:14:27, spang wrote:

> >
>
https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h#new...
> > ui/ozone/ozone_platform.h:15: class OZONE_EXPORT OzonePlatform {
> > this class is part of the external ozone api. it needs more comment

The ownership of SurfaceFactory and EventFactory are managed by ozoneplatform
subclass and not passed to the caller. It would be nice to see some related
comments in ozone_platform.h

[spang, 2013-10-25 19:44:58]

On 2013/10/25 19:34:24, kalyank wrote:
> On 2013/10/25 19:14:27, spang wrote:
> 
> > >
> >
>
https://codereview.chromium.org/44933002/diff/1/ui/ozone/ozone_platform.h#new...
> > > ui/ozone/ozone_platform.h:15: class OZONE_EXPORT OzonePlatform {
> > > this class is part of the external ozone api. it needs more comment
> 
> The ownership of SurfaceFactory and EventFactory are managed by ozoneplatform
> subclass and not passed to the caller. It would be nice to see some related
> comments in ozone_platform.h

Will clarify ownership in next upload:

+  // Factory getters to override in subclasses. The returned objects will be
+  // injected into the appropriate layer at startup. Subclasses should not
+  // inject these objects themselves. Ownership is retained by OzonePlatform.
   virtual gfx::SurfaceFactoryOzone* GetSurfaceFactoryOzone() = 0;

[kalyank, 2013-10-25 22:15:45]

Tested ozone-wayland by removing all ozone related initializations during
PreSandboxStartup() phase and doing it as here (content main runner). Everything
works as normal.

[vignatti (out of this project), 2013-10-28 15:26:43]

I'm very happy with this work overall. I've added my comments inline. Thanks for
carrying it over spang@.

https://codereview.chromium.org/44933002/diff/1/content/app/content_main_runn...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/44933002/diff/1/content/app/content_main_runn...
content/app/content_main_runner.cc:730: 
have you thought whether a single line like this would work also for other
targets that don't depend on content like aura_demo, or a few other tests?

https://codereview.chromium.org/44933002/diff/1/ui/gfx/ozone/surface_factory_...
File ui/gfx/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/44933002/diff/1/ui/gfx/ozone/surface_factory_...
ui/gfx/ozone/surface_factory_ozone.cc:58: char* envvar =
getenv("ASH_DISPLAY_SPEC");
I'm not sure we want to use this CL for fixing it, but the snip on this method
is also implementation specific and has to go away. Maybe we can define most of
SurfaceFactoryOzone methods as pure virtual?

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
File ui/base/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
ui/base/ozone/ozone_platform.cc:24: 
In the description you mention that it "depends on code in 
ui/gfx and ui/events because it must inject implementations into those
components". So, it's worth to mention that it depends on views as well
(ui/views/) for desktop aura cases and therefore some platforms want to
implement DesktopFactoryOzone.

That said, we probably want to add here the ::SetInstance on
views::DesktopFactoryOzone as well.

[spang, 2013-10-28 15:42:30]

https://codereview.chromium.org/44933002/diff/1/content/app/content_main_runn...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/44933002/diff/1/content/app/content_main_runn...
content/app/content_main_runner.cc:730: 
On 2013/10/28 15:26:44, vignatti wrote:
> have you thought whether a single line like this would work also for other
> targets that don't depend on content like aura_demo, or a few other tests? 

I do think we should be able to find a place to own the platform in most cases.
We should probably not try to do them all in one CL though.

https://codereview.chromium.org/44933002/diff/1/ui/gfx/ozone/surface_factory_...
File ui/gfx/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/44933002/diff/1/ui/gfx/ozone/surface_factory_...
ui/gfx/ozone/surface_factory_ozone.cc:58: char* envvar =
getenv("ASH_DISPLAY_SPEC");
On 2013/10/28 15:26:44, vignatti wrote:
> I'm not sure we want to use this CL for fixing it, but the snip on this method
> is also implementation specific and has to go away. Maybe we can define most
of
> SurfaceFactoryOzone methods as pure virtual?

I agree this probably doesn't belong, but not for this CL ;).

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
File ui/base/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
ui/base/ozone/ozone_platform.cc:24: 
On 2013/10/28 15:26:44, vignatti wrote:
> In the description you mention that it "depends on code in 
> ui/gfx and ui/events because it must inject implementations into those
> components". So, it's worth to mention that it depends on views as well
> (ui/views/) for desktop aura cases and therefore some platforms want to
> implement DesktopFactoryOzone.
> 
> That said, we probably want to add here the ::SetInstance on
> views::DesktopFactoryOzone as well.


We can't actually add a dependency from ui/base to views as this is against the
layering rules. We discussed this exact issue yesterday and we think we should
instead kill DesktopFactoryOzone and provide a different abstraction in ui/base
(I think sadrul suggested calling it PlatformWindow or something like that).

In the meantime, you'll have to workaround and inject DFO yourself.

[rjkroege, 2013-10-28 15:48:19]

lgtm

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone.gyp
File ui/base/ozone/ozone.gyp (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone.gyp#...
ui/base/ozone/ozone.gyp:38: ['exclude', '^platform/dri/'],
you will explain to me in the fullness of time how this will evolve with regards
to naming right?

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/platform/d...
File ui/base/ozone/platform/dri/ozone_platform_dri.h (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/platform/d...
ui/base/ozone/platform/dri/ozone_platform_dri.h:14: class OzonePlatformDri :
public OzonePlatform {
Add a brief (i.e. 1 sentence) description of the dri platform

[spang, 2013-10-28 16:01:41]

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone.gyp
File ui/base/ozone/ozone.gyp (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone.gyp#...
ui/base/ozone/ozone.gyp:38: ['exclude', '^platform/dri/'],
On 2013/10/28 15:48:19, rjkroege wrote:
> you will explain to me in the fullness of time how this will evolve with
regards
> to naming right?

Maybe we can discuss on the new ozone-dev list.

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/platform/d...
File ui/base/ozone/platform/dri/ozone_platform_dri.h (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/platform/d...
ui/base/ozone/platform/dri/ozone_platform_dri.h:14: class OzonePlatformDri :
public OzonePlatform {
On 2013/10/28 15:48:19, rjkroege wrote:
> Add a brief (i.e. 1 sentence) description of the dri platform

Done.

[vignatti (out of this project), 2013-10-28 16:08:49]

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
File ui/base/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
ui/base/ozone/ozone_platform.cc:24: 
On 2013/10/28 15:42:30, spang wrote:
> On 2013/10/28 15:26:44, vignatti wrote:
> > In the description you mention that it "depends on code in 
> > ui/gfx and ui/events because it must inject implementations into those
> > components". So, it's worth to mention that it depends on views as well
> > (ui/views/) for desktop aura cases and therefore some platforms want to
> > implement DesktopFactoryOzone.
> > 
> > That said, we probably want to add here the ::SetInstance on
> > views::DesktopFactoryOzone as well.
> 
> 
> We can't actually add a dependency from ui/base to views as this is against
the
> layering rules. We discussed this exact issue yesterday and we think we should
> instead kill DesktopFactoryOzone and provide a different abstraction in
ui/base
> (I think sadrul suggested calling it PlatformWindow or something like that).
> 
> In the meantime, you'll have to workaround and inject DFO yourself.

ok, I see.

I'm very interested then to hear what's sadrul@ ideas cause otherwise the
solution you're proposing doesn't solve the main problem and desktop aura
targets will still have to inject platform specific code in Chromium. Please let
me know your plans on this.

[spang, 2013-10-28 16:16:47]

On 2013/10/28 16:08:49, vignatti wrote:
>
https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
> File ui/base/ozone/ozone_platform.cc (right):
> 
>
https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
> ui/base/ozone/ozone_platform.cc:24: 
> On 2013/10/28 15:42:30, spang wrote:
> > On 2013/10/28 15:26:44, vignatti wrote:
> > > In the description you mention that it "depends on code in 
> > > ui/gfx and ui/events because it must inject implementations into those
> > > components". So, it's worth to mention that it depends on views as well
> > > (ui/views/) for desktop aura cases and therefore some platforms want to
> > > implement DesktopFactoryOzone.
> > > 
> > > That said, we probably want to add here the ::SetInstance on
> > > views::DesktopFactoryOzone as well.
> > 
> > 
> > We can't actually add a dependency from ui/base to views as this is against
> the
> > layering rules. We discussed this exact issue yesterday and we think we
should
> > instead kill DesktopFactoryOzone and provide a different abstraction in
> ui/base
> > (I think sadrul suggested calling it PlatformWindow or something like that).
> > 
> > In the meantime, you'll have to workaround and inject DFO yourself.
> 
> ok, I see.
> 
> I'm very interested then to hear what's sadrul@ ideas cause otherwise the
> solution you're proposing doesn't solve the main problem and desktop aura
> targets will still have to inject platform specific code in Chromium. Please
let
> me know your plans on this.


The main problem I want to fix is your having to apply patches to src.git.
That's still true even if we don't fix this immediately because you can inject
DFO from your OzonePlatform subclass. As an out-of-tree project, you are free to
play fast and loose with the layering rules (i.e., ignore them).

[vignatti (out of this project), 2013-10-28 16:22:04]

On 2013/10/28 16:16:47, spang wrote:
> On 2013/10/28 16:08:49, vignatti wrote:
> >
>
https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
> > File ui/base/ozone/ozone_platform.cc (right):
> > 
> >
>
https://codereview.chromium.org/44933002/diff/150001/ui/base/ozone/ozone_plat...
> > ui/base/ozone/ozone_platform.cc:24: 
> > On 2013/10/28 15:42:30, spang wrote:
> > > On 2013/10/28 15:26:44, vignatti wrote:
> > > > In the description you mention that it "depends on code in 
> > > > ui/gfx and ui/events because it must inject implementations into those
> > > > components". So, it's worth to mention that it depends on views as well
> > > > (ui/views/) for desktop aura cases and therefore some platforms want to
> > > > implement DesktopFactoryOzone.
> > > > 
> > > > That said, we probably want to add here the ::SetInstance on
> > > > views::DesktopFactoryOzone as well.
> > > 
> > > 
> > > We can't actually add a dependency from ui/base to views as this is
against
> > the
> > > layering rules. We discussed this exact issue yesterday and we think we
> should
> > > instead kill DesktopFactoryOzone and provide a different abstraction in
> > ui/base
> > > (I think sadrul suggested calling it PlatformWindow or something like
that).
> > > 
> > > In the meantime, you'll have to workaround and inject DFO yourself.
> > 
> > ok, I see.
> > 
> > I'm very interested then to hear what's sadrul@ ideas cause otherwise the
> > solution you're proposing doesn't solve the main problem and desktop aura
> > targets will still have to inject platform specific code in Chromium. Please
> let
> > me know your plans on this.
> 
> 
> The main problem I want to fix is your having to apply patches to src.git.
> That's still true even if we don't fix this immediately because you can inject
> DFO from your OzonePlatform subclass. As an out-of-tree project, you are free
to
> play fast and loose with the layering rules (i.e., ignore them).

agreed.

I think that calling DFO::SetInstance() from our SurfaceFactoryOzone
implementation works then. Let's give a go on this CL and we can revive it again
later if needed.

LGTM.

[kalyank, 2013-10-28 16:25:39]

> 
> I think that calling DFO::SetInstance() from our SurfaceFactoryOzone
> implementation works then. Let's give a go on this CL and we can revive it
again
> later if needed.
> 
> LGTM.

Yes, we can do this on our side till proper fix is in place. LGTM too.

[spang, 2013-10-28 16:41:39]

+beng

to see if we can add ui/base/ozone

[Ben Goodger (Google), 2013-10-30 17:45:08]

Note:

ui/base is now not as low level as the name implies. We spent some time recently
straightening out dependencies in src/ui, such that ui/base is now *above*
ui/gfx etc. I think we will also want to make ui/aura etc. not depend on ui/base
(i.e. further componentization of ui/base). If this makes you want to reconsider
placing this code in ui/base, lmk.

[rjkroege, 2013-10-30 17:52:19]

On 2013/10/30 17:45:08, Ben Goodger (Google) wrote:
> Note:
> 
> ui/base is now not as low level as the name implies. We spent some time
recently
> straightening out dependencies in src/ui, such that ui/base is now *above*
> ui/gfx etc. I think we will also want to make ui/aura etc. not depend on
ui/base
> (i.e. further componentization of ui/base). If this makes you want to
reconsider
> placing this code in ui/base, lmk.

[spang, 2013-10-30 17:53:33]

On 2013/10/30 17:45:08, Ben Goodger (Google) wrote:
> Note:
> 
> ui/base is now not as low level as the name implies. We spent some time
recently
> straightening out dependencies in src/ui, such that ui/base is now *above*
> ui/gfx etc. I think we will also want to make ui/aura etc. not depend on
ui/base
> (i.e. further componentization of ui/base). If this makes you want to
reconsider
> placing this code in ui/base, lmk.

Hm, we may need to discuss placement then.

We probably want aura_demo to work with ozone. Which means we would have to
instantiate OzonePlatform somewhere for that target in order to get the relevant
bits injected into ui/gfx and ui/events.

[rjkroege, 2013-10-30 17:55:42]

On 2013/10/30 17:52:19, rjkroege wrote:
> On 2013/10/30 17:45:08, Ben Goodger (Google) wrote:
> > Note:
> > 
> > ui/base is now not as low level as the name implies. We spent some time
> recently
> > straightening out dependencies in src/ui, such that ui/base is now *above*
> > ui/gfx etc. I think we will also want to make ui/aura etc. not depend on
> ui/base
> > (i.e. further componentization of ui/base). If this makes you want to
> reconsider
> > placing this code in ui/base, lmk.

ui/base/ozone depends on ui/gfx/ozone and ui/events/ozone -- so at that level,
it seemed like the right place.

but we would like ui/aura/ to be able to depend on the code here so that the
aura examples could run on ozone. Given that ozone always implies aura. maybe
ui/aura/ would be better?

[Ben Goodger (Google), 2013-10-30 17:57:53]

Since it's a standalone component (according to your gyp file), I'm also
fine with ui/ozone...

-Ben


On Wed, Oct 30, 2013 at 10:55 AM, <rjkroege@chromium.org> wrote:

> On 2013/10/30 17:52:19, rjkroege wrote:
>
>> On 2013/10/30 17:45:08, Ben Goodger (Google) wrote:
>> > Note:
>> >
>> > ui/base is now not as low level as the name implies. We spent some time
>> recently
>> > straightening out dependencies in src/ui, such that ui/base is now
>> *above*
>> > ui/gfx etc. I think we will also want to make ui/aura etc. not depend on
>> ui/base
>> > (i.e. further componentization of ui/base). If this makes you want to
>> reconsider
>> > placing this code in ui/base, lmk.
>>
>
> ui/base/ozone depends on ui/gfx/ozone and ui/events/ozone -- so at that
> level,
> it seemed like the right place.
>
> but we would like ui/aura/ to be able to depend on the code here so that
> the
> aura examples could run on ozone. Given that ozone always implies aura.
> maybe
> ui/aura/ would be better?
>
>
https://codereview.chromium.**org/44933002/<https://codereview.chromium.org/4...
>

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

[spang, 2013-10-30 23:02:36]

Ok, renamed to ui/ozone.

I've rethought initialization a bit and removed the change in
ContentMainRunnerImpl. It will now be initialized:

1. When the first RootWindowHostOzone is constructed, or
2. When InitializeGLBindings is called,

whichever is earlier.

On 2013/10/30 17:57:53, Ben Goodger (Google) wrote:
> Since it's a standalone component (according to your gyp file), I'm also
> fine with ui/ozone...
> 
> -Ben
> 
> 
> On Wed, Oct 30, 2013 at 10:55 AM, <mailto:rjkroege@chromium.org> wrote:
> 
> > On 2013/10/30 17:52:19, rjkroege wrote:
> >
> >> On 2013/10/30 17:45:08, Ben Goodger (Google) wrote:
> >> > Note:
> >> >
> >> > ui/base is now not as low level as the name implies. We spent some time
> >> recently
> >> > straightening out dependencies in src/ui, such that ui/base is now
> >> *above*
> >> > ui/gfx etc. I think we will also want to make ui/aura etc. not depend on
> >> ui/base
> >> > (i.e. further componentization of ui/base). If this makes you want to
> >> reconsider
> >> > placing this code in ui/base, lmk.
> >>
> >
> > ui/base/ozone depends on ui/gfx/ozone and ui/events/ozone -- so at that
> > level,
> > it seemed like the right place.
> >
> > but we would like ui/aura/ to be able to depend on the code here so that
> > the
> > aura examples could run on ozone. Given that ozone always implies aura.
> > maybe
> > ui/aura/ would be better?
> >
> >
>
https://codereview.chromium.**org/44933002/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2013-10-30 23:21:01]

On 2013/10/30 23:02:36, spang wrote:
> Ok, renamed to ui/ozone.
> 
> I've rethought initialization a bit and removed the change in
> ContentMainRunnerImpl. It will now be initialized:
> 
> 1. When the first RootWindowHostOzone is constructed, or
> 2. When InitializeGLBindings is called,
> 
> whichever is earlier.
> 

Any particular reason for this change ?
My understanding is that in Multi process mode InitializeGLBindings is called in
GPU process side while RootWindowHostOzone is constructed on Browser Process
side. Isn't that the case? (Just confused with comment which ever is earlier).
How about Desktop Aura?

[kalyank, 2013-10-30 23:21:14]

https://codereview.chromium.org/44933002/diff/470001/ui/aura/root_window_host...
File ui/aura/root_window_host_ozone.cc (right):

https://codereview.chromium.org/44933002/diff/470001/ui/aura/root_window_host...
ui/aura/root_window_host_ozone.cc:17:
factory_(ui::EventFactoryOzone::GetInstance()) {
We try to retrieve eventfactory instance before ozoneplatform is even
initialized?

[Ben Goodger (Google), 2013-10-30 23:56:08]

https://codereview.chromium.org/44933002/diff/470001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/44933002/diff/470001/ui/ui.gyp#newcode583
ui/ui.gyp:583: 'ozone/ozone.gyp:ozone',
is this still needed?

[spang, 2013-10-31 00:38:23]

https://codereview.chromium.org/44933002/diff/470001/ui/aura/root_window_host...
File ui/aura/root_window_host_ozone.cc (right):

https://codereview.chromium.org/44933002/diff/470001/ui/aura/root_window_host...
ui/aura/root_window_host_ozone.cc:17:
factory_(ui::EventFactoryOzone::GetInstance()) {
On 2013/10/30 23:21:15, kalyank wrote:
> We try to retrieve eventfactory instance before ozoneplatform is even
> initialized?

Fixed. Was hitting a fallback that we should remove in a followup CL.

https://codereview.chromium.org/44933002/diff/470001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/44933002/diff/470001/ui/ui.gyp#newcode583
ui/ui.gyp:583: 'ozone/ozone.gyp:ozone',
On 2013/10/30 23:56:09, Ben Goodger (Google) wrote:
> is this still needed?

No, removed.

[spang, 2013-10-31 00:50:51]

On 2013/10/30 23:21:01, kalyank wrote:
> On 2013/10/30 23:02:36, spang wrote:
> > Ok, renamed to ui/ozone.
> > 
> > I've rethought initialization a bit and removed the change in
> > ContentMainRunnerImpl. It will now be initialized:
> > 
> > 1. When the first RootWindowHostOzone is constructed, or
> > 2. When InitializeGLBindings is called,
> > 
> > whichever is earlier.
> > 
> 
> Any particular reason for this change ?
> My understanding is that in Multi process mode InitializeGLBindings is called
in
> GPU process side while RootWindowHostOzone is constructed on Browser Process
> side. Isn't that the case? (Just confused with comment which ever is earlier).
> How about Desktop Aura?

The reason is that targets that use aura or GL but not content should also work
when ozone is enabled. I do expect the browser to initialize from
RootWindowHostOzone and the GPU to initialize from InitializeGLBindings. That is
why there are two calls to Initialize.

You may need a patch to use DesktopRootWindowHostOzone with wayland until we
rearrange the code.

[Ben Goodger (Google), 2013-10-31 01:20:50]

lgtm

[vignatti (out of this project), 2013-10-31 08:43:09]

On 2013/10/31 00:50:51, spang wrote:
> On 2013/10/30 23:21:01, kalyank wrote:
> > On 2013/10/30 23:02:36, spang wrote:
> > > Ok, renamed to ui/ozone.
> > > 
> > > I've rethought initialization a bit and removed the change in
> > > ContentMainRunnerImpl. It will now be initialized:
> > > 
> > > 1. When the first RootWiwndoHostOzone is constructed, or
> > > 2. When InitializeGLBindings is called,
> > > 
> > > whichever is earlier.
> > > 
> > 
> > Any particular reason for this change ?
> > My understanding is that in Multi process mode InitializeGLBindings is
called
> in
> > GPU process side while RootWindowHostOzone is constructed on Browser Process
> > side. Isn't that the case? (Just confused with comment which ever is
earlier).
> > How about Desktop Aura?
> 
> The reason is that targets that use aura or GL but not content should also
work
> when ozone is enabled. I do expect the browser to initialize from
> RootWindowHostOzone and the GPU to initialize from InitializeGLBindings. That
is
> why there are two calls to Initialize.
> 
> You may need a patch to use DesktopRootWindowHostOzone with wayland until we
> rearrange the code.

sorry, but I'm not seeing neither the Desktop Aura's RootWindowHostOzone (we
don't have a class name for it yet) and InitializeGLBindings called earlier than
ContentMainRunnerImpl.

I think the main problem of these changes is with the Desktop Aura's
RootWindowHostOzone side of it, i.e. the browser process initialization; the GPU
process seems fine. In the browswer process, views::CreateDesktopScreen is
required to be up in a very early stage (at PostMainMessageLoopStart) and so it
has to delegates its creation to the underlying Ozone platform. That happens
before views asks for any native widget creation and so explains why we need
Ozone platform set before RootWindowHostOzone.

But I don't know either where in the browser process's initialization this has
to be stick so targets based on content and also the others like aura_demo etc
would set OzonePlatform fine.

[spang, 2013-10-31 14:51:11]

On 2013/10/31 08:43:09, vignatti wrote:
> On 2013/10/31 00:50:51, spang wrote:
> > On 2013/10/30 23:21:01, kalyank wrote:
> > > On 2013/10/30 23:02:36, spang wrote:
> > > > Ok, renamed to ui/ozone.
> > > > 
> > > > I've rethought initialization a bit and removed the change in
> > > > ContentMainRunnerImpl. It will now be initialized:
> > > > 
> > > > 1. When the first RootWiwndoHostOzone is constructed, or
> > > > 2. When InitializeGLBindings is called,
> > > > 
> > > > whichever is earlier.
> > > > 
> > > 
> > > Any particular reason for this change ?
> > > My understanding is that in Multi process mode InitializeGLBindings is
> called
> > in
> > > GPU process side while RootWindowHostOzone is constructed on Browser
Process
> > > side. Isn't that the case? (Just confused with comment which ever is
> earlier).
> > > How about Desktop Aura?
> > 
> > The reason is that targets that use aura or GL but not content should also
> work
> > when ozone is enabled. I do expect the browser to initialize from
> > RootWindowHostOzone and the GPU to initialize from InitializeGLBindings.
That
> is
> > why there are two calls to Initialize.
> > 
> > You may need a patch to use DesktopRootWindowHostOzone with wayland until we
> > rearrange the code.
> 
> sorry, but I'm not seeing neither the Desktop Aura's RootWindowHostOzone (we
> don't have a class name for it yet) and InitializeGLBindings called earlier
than
> ContentMainRunnerImpl.

They are not.

> 
> I think the main problem of these changes is with the Desktop Aura's
> RootWindowHostOzone side of it, i.e. the browser process initialization; the
GPU
> process seems fine. In the browswer process, views::CreateDesktopScreen is
> required to be up in a very early stage (at PostMainMessageLoopStart) and so
it
> has to delegates its creation to the underlying Ozone platform. That happens
> before views asks for any native widget creation and so explains why we need
> Ozone platform set before RootWindowHostOzone.
> 
> But I don't know either where in the browser process's initialization this has
> to be stick so targets based on content and also the others like aura_demo etc
> would set OzonePlatform fine.

We can have multiple calls to Initialize in the browser. Let's add another for
the
desktop case in a followup CL.

[commit-bot: I haz the power, 2013-10-31 15:52:16]

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

[commit-bot: I haz the power, 2013-10-31 18:22:45]

Change committed as 232170