Make DesktopFactoryOzone instances to respect --ozone-platform

Make DesktopFactoryOzone instances to respect --ozone-platform

In order to support "use_ozone=1 ichromeos=0" builds,
DesktopFactoryOzone class needs to respect the Ozone platform
backend that is going to run.

For instance, ui::OzonePlatform class (see ui/ozone/) accomplishes
this using the logic provided by ui::OzoneObject.
In short, OzoneObject allows OzonePlatform instances (e.g. wayland,
GDM, x11) to get selected at runtime, obeying either --ozone-platform
command line or using a default value to select the platform backend.

This patcj extends this logic to DesktopFactoryOzone class, as a first
step towards supporting "use_ozone=1 chromeos=0" builds.
DesktopFactoryOzoneWayland is added (with dummy implementations) as
a sample implementation and starting point.

BUG=295089

[not to use - tonikitoo, 2016-06-01 15:01:36]

Description was changed from

==========
Make DesktopFactoryOzone instances to respect --ozone-platform

In order to support "use_ozone=1 ichromeos=0" builds,
DesktopFactoryOzone class needs to respect the Ozone platform
backend that is going to run.

For instance, ui::OzonePlatform class (see ui/ozone/) accomplishes
this using the logic provided by ui::OzoneObject.
In short, OzoneObject allows OzonePlatform instances (e.g. wayland,
GDM, x11) to get selected at runtime, obeying either --ozone-platform
command line or using a default value to select the platform backend.

This patcj extends this logic to DesktopFactoryOzone class, as a first
step towards supporting "use_ozone=1 chromeos=0" builds.
DesktopFactoryOzoneWayland is added (with dummy implementations) as
a sample implementation and starting point.

BUG=295089
==========

to

==========
Make DesktopFactoryOzone instances to respect --ozone-platform

In order to support "use_ozone=1 ichromeos=0" builds,
DesktopFactoryOzone class needs to respect the Ozone platform
backend that is going to run.

For instance, ui::OzonePlatform class (see ui/ozone/) accomplishes
this using the logic provided by ui::OzoneObject.
In short, OzoneObject allows OzonePlatform instances (e.g. wayland,
GDM, x11) to get selected at runtime, obeying either --ozone-platform
command line or using a default value to select the platform backend.

This patcj extends this logic to DesktopFactoryOzone class, as a first
step towards supporting "use_ozone=1 chromeos=0" builds.
DesktopFactoryOzoneWayland is added (with dummy implementations) as
a sample implementation and starting point.

BUG=295089
==========

[not to use - tonikitoo, 2016-06-01 15:01:36]

tonikitoo@chromium.org changed reviewers:
+ rjkroege@chromium.org, spang@chromium.org

[not to use - tonikitoo, 2016-06-01 16:56:35]

tonikitoo@chromium.org changed reviewers:
+ reveman@chromium.org

[not to use - tonikitoo, 2016-06-01 16:56:54]

tonikitoo@chromium.org changed reviewers:
+ forney@google.com

[Michael Forney, 2016-06-01 20:51:22]

Do the views.gyp changes need corresponding changes to BUILD.gn? If so, please
make them.

[tonikitoo, 2016-06-01 21:24:14]

On 2016/06/01 20:51:22, Michael Forney wrote:
> Do the views.gyp changes need corresponding changes to BUILD.gn? If so, please
> make them.

@forney: No need to touch .gn files this time. See ui/views/BUILD.gn:

  1 # Copyright 2014 The Chromium Authors. All rights reserved.
 (..)
  9 gypi_values = exec_script("//build/gypi_to_gn.py",
 10                           [ rebase_path("views.gyp") ],
 11                           "scope",
 12                           [ "views.gyp" ])
 13 

This directory still converts .gyp to the associated .gn file.

[Michael Forney, 2016-06-01 23:28:45]

On 2016/06/01 21:24:14, tonikitoo1 wrote:
> On 2016/06/01 20:51:22, Michael Forney wrote:
> > Do the views.gyp changes need corresponding changes to BUILD.gn? If so,
please
> > make them.
> 
> @forney: No need to touch .gn files this time. See ui/views/BUILD.gn:
> 
>   1 # Copyright 2014 The Chromium Authors. All rights reserved.
>  (..)
>   9 gypi_values = exec_script("//build/gypi_to_gn.py",
>  10                           [ rebase_path("views.gyp") ],
>  11                           "scope",
>  12                           [ "views.gyp" ])
>  13 
> 
> This directory still converts .gyp to the associated .gn file.

Did you try a gn build? I patched in your CL and it doesn't work.

The gypi_to_gn.py script is is not as magical as you think. Yes, we get the
gypi_values, but you added extra actions and gypi variables that are not
referenced in BUILD.gn (for example, views_desktop_aura_ozone_wayland_sources).

Take a look at
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/BUILD.gn&...

That is the corresponding part of BUILD.gn to your changes to views.gyp.

[tonikitoo, 2016-06-03 23:58:14]

On 2016/06/01 23:28:45, Michael Forney wrote:
> On 2016/06/01 21:24:14, tonikitoo1 wrote:
> > On 2016/06/01 20:51:22, Michael Forney wrote:
> > > Do the views.gyp changes need corresponding changes to BUILD.gn? If so,
> please
> > > make them.
> > 
> > @forney: No need to touch .gn files this time. See ui/views/BUILD.gn:
> > 
> >   1 # Copyright 2014 The Chromium Authors. All rights reserved.
> >  (..)
> >   9 gypi_values = exec_script("//build/gypi_to_gn.py",
> >  10                           [ rebase_path("views.gyp") ],
> >  11                           "scope",
> >  12                           [ "views.gyp" ])
> >  13 
> > 
> > This directory still converts .gyp to the associated .gn file.
> 
> Did you try a gn build? I patched in your CL and it doesn't work.
> 
> The gypi_to_gn.py script is is not as magical as you think. Yes, we get the
> gypi_values, but you added extra actions and gypi variables that are not
> referenced in BUILD.gn (for example,
views_desktop_aura_ozone_wayland_sources).
> 
> Take a look at
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/BUILD.gn&...
> 
> That is the corresponding part of BUILD.gn to your changes to views.gyp.

@forney: Thanks. That was indeed the case. I have uploaded the patch with GN
support.

I have built it with the following GN options:
   is_component_build = false
   is_debug = false
   use_aura = true
   use_ash = true
   use_ozone = true
   ozone_platform_wayland = true
   ozone_auto_platforms = false
   use_wayland_egl = false

PTAL.

[tonikitoo, 2016-06-06 15:38:13]

On 2016/06/03 23:58:14, tonikitoo1 wrote:
> On 2016/06/01 23:28:45, Michael Forney wrote:
> > On 2016/06/01 21:24:14, tonikitoo1 wrote:
> > > On 2016/06/01 20:51:22, Michael Forney wrote:
> > > > Do the views.gyp changes need corresponding changes to BUILD.gn? If so,
> > please
> > > > make them.
> > > 
> > > @forney: No need to touch .gn files this time. See ui/views/BUILD.gn:
> > > 
> > >   1 # Copyright 2014 The Chromium Authors. All rights reserved.
> > >  (..)
> > >   9 gypi_values = exec_script("//build/gypi_to_gn.py",
> > >  10                           [ rebase_path("views.gyp") ],
> > >  11                           "scope",
> > >  12                           [ "views.gyp" ])
> > >  13 
> > > 
> > > This directory still converts .gyp to the associated .gn file.
> > 
> > Did you try a gn build? I patched in your CL and it doesn't work.
> > 
> > The gypi_to_gn.py script is is not as magical as you think. Yes, we get the
> > gypi_values, but you added extra actions and gypi variables that are not
> > referenced in BUILD.gn (for example,
> views_desktop_aura_ozone_wayland_sources).
> > 
> > Take a look at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/BUILD.gn&...
> > 
> > That is the corresponding part of BUILD.gn to your changes to views.gyp.
> 
> @forney: Thanks. That was indeed the case. I have uploaded the patch with GN
> support.
> 
> I have built it with the following GN options:
>    is_component_build = false
>    is_debug = false
>    use_aura = true
>    use_ozone = true
>    ozone_platform_wayland = true
>    ozone_auto_platforms = false
>    use_wayland_egl = false
> 
> PTAL.

Patch 3 actually add the new files:
ui/views/widget/desktop_aura/desktop_factory_ozone_wayland.cc/h left over by
accident in Patch 2.

Also in the GN args used, "use_ash" was incorrect. It should be "use_ash =
false"

[Michael Forney, 2016-06-06 20:08:21]

lgtm

We'll see what the chromium/ozone people think.

https://codereview.chromium.org/2024953007/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_factory_ozone.cc (right):

https://codereview.chromium.org/2024953007/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_factory_ozone.cc:32: // TODO(tonikitoo):
Currently need to leak this object.
I wasn't sure about this, but it looks like this is what we currently do for
OzonePlatform, so I guess it's fine.

[joone, 2016-06-06 22:37:57]

joone.hur@intel.com changed reviewers:
+ joone.hur@intel.com

[joone, 2016-06-06 22:37:58]

https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp#newc...
ui/views/views.gyp:846: '--platform_list=<(platform_list_txt_file)',

How about passing '--platform=wayland' instead of this?

[joone, 2016-06-07 00:04:17]

https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp#newc...
ui/views/views.gyp:846: '--platform_list=<(platform_list_txt_file)',
On 2016/06/06 22:37:58, joone wrote:
> 
> How about passing '--platform=wayland' instead of this?

We had better create DesktopFactoryOzone(Headless|Egltest) for including the
following functions instead.

views::DesktopFactoryOzone* CreateDesktopFactoryOzoneHeadless() {
  return nullptr;
}

views::DesktopFactoryOzone* CreateDesktopFactoryOzoneEgltest() {
  return nullptr;
}

[tonikitoo, 2016-06-07 12:17:14]

On 2016/06/06 22:37:58, joone wrote:
> https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp
> File ui/views/views.gyp (right):
> 
>
https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp#newc...
> ui/views/views.gyp:846: '--platform_list=<(platform_list_txt_file)',
> 
> How about passing '--platform=wayland' instead of this?

Hi Joone.

platform_list_txt file is might have more than one platform listed in it,
depending on our GYP / GN paramaters. For instance ozone_auto_platforms=true
sets 'headless' as the default ozone platform IIRC. So platform_list_txt would
look like

---
headless
wayland
-----

If one runs content_shell without specifying --ozone-platform={bleh}, 'headless'
would be picked up then.

So basically, I chose to pass to the script the list of platforms that are
listed in 'platform_list.txt' instead of a specific one.

Did I understand your question ok?

[rjkroege, 2016-06-07 23:42:09]

The goal here is to support running Chrome in "desktop mode" on Wayland?

I am not convinced that this patch is the right longterm approach. I don't think
that desktop_factory_ozone_wayland.h and desktop_factory_ozone_wayland.cc belong
here.

If I am correctly understanding the top-level-goal, it would seem to me that the
ongoing mus work gives us most of the necessary refactoring already: we just
need mus's WindowTreeHost's mode to work with ozone wayland and for mus's
PlatformScreen implementation to do "the right thing".  (Though I'm certainly
glossing over a long list of issues.)



On 2016/06/07 12:17:14, tonikitoo1 wrote:
> On 2016/06/06 22:37:58, joone wrote:
> > https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp
> > File ui/views/views.gyp (right):
> > 
> >
>
https://codereview.chromium.org/2024953007/diff/40001/ui/views/views.gyp#newc...
> > ui/views/views.gyp:846: '--platform_list=<(platform_list_txt_file)',
> > 
> > How about passing '--platform=wayland' instead of this?
> 
> Hi Joone.
> 
> platform_list_txt file is might have more than one platform listed in it,
> depending on our GYP / GN paramaters. For instance ozone_auto_platforms=true
> sets 'headless' as the default ozone platform IIRC. So platform_list_txt would
> look like
> 
> ---
> headless
> wayland
> -----
> 
> If one runs content_shell without specifying --ozone-platform={bleh},
'headless'
> would be picked up then.
> 
> So basically, I chose to pass to the script the list of platforms that are
> listed in 'platform_list.txt' instead of a specific one.
> 
> Did I understand your question ok?

[not to use - tonikitoo, 2016-06-08 00:01:37]

On 2016/06/07 23:42:09, rjkroege wrote:

Hi Robert. Thanks for looking.

> The goal here is to support running Chrome in "desktop mode" on Wayland?

Right, this is the goal. Actually, this specific CL would be just the tip of the
iceberg.

It aims at creating DesktopFactoryOzone{Bleh} from
DesktopFactoryOzone::GetInstance(), in accordance to --ozone-platform={bleh}

> I am not convinced that this patch is the right longterm approach. I don't
think
> that desktop_factory_ozone_wayland.h and desktop_factory_ozone_wayland.cc
belong
> here.

I tried to follow the logic in window_tree_host_{backend}.cc/h and
screen_window{backend}.cc/h in ui/views/widget/desktop_aura/.

> If I am correctly understanding the top-level-goal, it would seem to me that
the
> ongoing mus work gives us most of the necessary refactoring already: we just
> need mus's WindowTreeHost's mode to work with ozone wayland and for mus's
> PlatformScreen implementation to do "the right thing".  (Though I'm certainly
> glossing over a long list of issues.)

Is "mus" chromeos-specific at the moment?

[joone, 2016-09-30 07:45:20]

@tonikitoo can you rebase your CL?

[fwang, 2017-02-17 11:03:48]

I believe the agreed approach for upstream Ozone is to do desktop integration
via Mus
(https://docs.google.com/presentation/d/13TV2sjLlO8Nqqm6jgaNLwM9v03QEA0idIOS8M...)
so I guess we can close this now?

[tonikitoo, 2017-02-17 17:07:51]

On 2017/02/17 11:03:48, fwang wrote:
> I believe the agreed approach for upstream Ozone is to do desktop integration
> via Mus
>
(https://docs.google.com/presentation/d/13TV2sjLlO8Nqqm6jgaNLwM9v03QEA0idIOS8M...)
> so I guess we can close this now?

Closed.