Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0

Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.

GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

BUG=295089
Committed: https://crrev.com/b10ea66fcc43a0eed828eb62ffb2bd141b825fce
Cr-Commit-Position: refs/heads/master@{#430424}

[tonikitoo, 2016-10-10 17:10:47]

Description was changed from

==========
ozone=1 chromeos=0

GN args:

  is_debug = false
  is_component_build = true
  symbol_level = 1

  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false

  use_sysroot = false
  treat_warnings_as_errors = false

  enable_nacl = false
  remove_webcore_debug_symbols = true

  enable_package_mash_services = true

Launch:

<out>/chrome --mash  --ozone-platform=wayland|x11   --no-sandbox
--user-data-dir=~/tmp

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

to

==========
ozone=1 chromeos=0

GN args:

  is_debug = false
  is_component_build = true
  symbol_level = 1

  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false

  use_sysroot = false
  treat_warnings_as_errors = false

  enable_nacl = false
  remove_webcore_debug_symbols = true

  enable_package_mash_services = true

Launch:

<out>/chrome --mash  --ozone-platform=wayland|x11   --no-sandbox
--user-data-dir=~/tmp

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

[fwang, 2016-10-11 07:30:09]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[fwang, 2016-10-11 07:30:10]

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stu...
File chrome/browser/ui/ozone_stubs.h (right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stu...
chrome/browser/ui/ozone_stubs.h:17: 
Extra blank line at the end of the file.

[fwang, 2016-10-11 09:34:13]

This basically l g t m

When testing "chrome --mash", many images from ash/resources/ash_resources.grd
fails to load and shows up as yellow rectangles. That's probably because the
chrome_*_percent.pak resources loaded at startup miss some files from the
ash_resources_*_percent.pak. See also
https://bugs.chromium.org/p/chromium/issues/detail?id=628715

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1543: # These files rely on Gtk+ calls, which is not
available on Ozone builds.
GTK+ calls, which are not...

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:20:
return nullptr;
I think this should be

#if !defined(USE_OZONE)
   return new DesktopBrowserFrameAuraLinux(browser_frame,...
#else
   return nullptr;
#endif

to avoid duplicate returns when !USE_OZONE

[fwang, 2016-10-11 10:04:57]

On 2016/10/11 09:34:13, fwang wrote:
> When testing "chrome --mash", many images from ash/resources/ash_resources.grd
> fails to load and shows up as yellow rectangles. That's probably because the
> chrome_*_percent.pak resources loaded at startup miss some files from the
> ash_resources_*_percent.pak. See also
> https://bugs.chromium.org/p/chromium/issues/detail?id=628715

This is maybe what you want to do to bundle the ash_resources_*_percent.pak into
the chrome_*_percent.pak:

diff --git a/chrome/chrome_paks.gni b/chrome/chrome_paks.gni
index 3e93269..7b47a58f 100644
--- a/chrome/chrome_paks.gni
+++ b/chrome/chrome_paks.gni
@@ -50,7 +50,7 @@ template("chrome_repack_percent") {
       deps += invoker.deps
     }
 
-    if (use_ash) {
+    if (use_ash || use_ozone) {
       sources +=
           [ "$root_gen_dir/ash/resources/ash_resources_${percent}_percent.pak"
]
       deps += [ "//ash/resources" ]

[tonikitoo, 2016-10-11 12:56:28]

On 2016/10/11 10:04:57, fwang wrote:
> On 2016/10/11 09:34:13, fwang wrote:
> > When testing "chrome --mash", many images from
ash/resources/ash_resources.grd
> > fails to load and shows up as yellow rectangles. That's probably because the
> > chrome_*_percent.pak resources loaded at startup miss some files from the
> > ash_resources_*_percent.pak. See also
> > https://bugs.chromium.org/p/chromium/issues/detail?id=628715
> 
> This is maybe what you want to do to bundle the ash_resources_*_percent.pak
into
> the chrome_*_percent.pak:
> 
> diff --git a/chrome/chrome_paks.gni b/chrome/chrome_paks.gni
> index 3e93269..7b47a58f 100644
> --- a/chrome/chrome_paks.gni
> +++ b/chrome/chrome_paks.gni
> @@ -50,7 +50,7 @@ template("chrome_repack_percent") {
>        deps += invoker.deps
>      }
>  
> -    if (use_ash) {
> +    if (use_ash || use_ozone) {
>        sources +=
>            [
"$root_gen_dir/ash/resources/ash_resources_${percent}_percent.pak"
> ]
>        deps += [ "//ash/resources" ]

Great @fwang! I have incorporated your review into patcheset #2.

[tonikitoo, 2016-10-11 12:58:44]

Description was changed from

==========
ozone=1 chromeos=0

GN args:

  is_debug = false
  is_component_build = true
  symbol_level = 1

  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false

  use_sysroot = false
  treat_warnings_as_errors = false

  enable_nacl = false
  remove_webcore_debug_symbols = true

  enable_package_mash_services = true

Launch:

<out>/chrome --mash  --ozone-platform=wayland|x11   --no-sandbox
--user-data-dir=~/tmp

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

to

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0
    
CL provides a first step for running with chrome/chromium
natively on Wayland and X11, through Ozone.
    
Some method are stub'ed for now, intention is implementing
them on follow up patches.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
    
  enable_package_mash_services = true
  use_ash = false
    
Launch:
$ <out>/chrome --mash --ozone-platform=wayland|x11 --no-sandbox
--user-data-dir=~/tmp
==========

[tonikitoo, 2016-10-11 12:58:44]

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

[tonikitoo, 2016-10-11 12:59:03]

PTAL

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1543: # These files rely on Gtk+ calls, which is not
available on Ozone builds.
On 2016/10/11 09:34:12, fwang wrote:
> GTK+ calls, which are not...

Done.

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stu...
File chrome/browser/ui/ozone_stubs.h (right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stu...
chrome/browser/ui/ozone_stubs.h:17: 
On 2016/10/11 07:30:09, fwang wrote:
> Extra blank line at the end of the file.

Done.

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:20:
return nullptr;
On 2016/10/11 09:34:12, fwang wrote:
> I think this should be
> 
> #if !defined(USE_OZONE)
>    return new DesktopBrowserFrameAuraLinux(browser_frame,...
> #else
>    return nullptr;
> #endif
> 
> to avoid duplicate returns when !USE_OZONE

Done.

[fwang, 2016-10-11 13:03:56]

informal l g t m

[rjkroege, 2016-10-11 18:49:47]

I'm amazed how little change is actually needed to build and run with this.
That's pretty awesome.

I think this CL is a reasonable start but will probably require a lot of
discussion. Cool!

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone...
File chrome/browser/ui/ozone_stubs.cc (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone...
chrome/browser/ui/ozone_stubs.cc:17: bool IsFullScreenMode() {
this is weird. It should be in an anonymous block? And it's not used? Delete it?

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:17: #if
!defined(USE_OZONE)
this looks strange. i.e.: something is not right here but I'm not the right
person. +sadrul to comment more about how this should fit?

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:10: gfx::NativeWindow
WindowFinder::GetLocalProcessWindowAtPoint(
mus should tell you this? i.e.: this code should never get invoked?

https://codereview.chromium.org/2408803002/diff/40001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/chrome_paks.gni#...
chrome/chrome_paks.gni:53: if (use_ash || use_ozone) {
I might be persuaded that this is a temporary expedient. But it's definitely not
the right way long term. None of the ash resources should be needed right? So at
an absolute minimum, you need a TODO here.

[sadrul, 2016-10-11 19:14:14]

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:17: #if
!defined(USE_OZONE)
On 2016/10/11 18:49:47, rjkroege wrote:
> this looks strange. i.e.: something is not right here but I'm not the right
> person. +sadrul to comment more about how this should fit?

What happens if you use this piece of code with ozone?

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:10: gfx::NativeWindow
WindowFinder::GetLocalProcessWindowAtPoint(
On 2016/10/11 18:49:47, rjkroege wrote:
> mus should tell you this? i.e.: this code should never get invoked?

There's a window_finder_mus.cc that should be used, yeah.

[sadrul, 2016-10-11 19:26:56]

Some of the changes in this CL fall in the 'obviously correct' category (e.g.
USE_X11 check before using X11-specific code, in browser_frame.cc). Others are
less obvious, and it would be useful to understand those better (e.g. perhaps
add comments in the CL so that we can continue the discussion in here?)

All of these changes will not have any build/test coverage at the moment though.
So when we land this, we need to make sure that we have builders set up so that
we know when something breaks (even if it doesn't cause a tree-closure at first,
maybe) because otherwise, this will continue to break as more work happens on
mus+ozone+ash land.

[spang, 2016-10-11 21:38:04]

Description was changed from

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0
    
CL provides a first step for running with chrome/chromium
natively on Wayland and X11, through Ozone.
    
Some method are stub'ed for now, intention is implementing
them on follow up patches.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
    
  enable_package_mash_services = true
  use_ash = false
    
Launch:
$ <out>/chrome --mash --ozone-platform=wayland|x11 --no-sandbox
--user-data-dir=~/tmp
==========

to

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0
    
CL provides a first step for running with chrome/chromium
natively on Wayland and X11, through Ozone.
    
Some method are stub'ed for now, intention is implementing
them on follow up patches.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
    
  enable_package_mash_services = true
  use_ash = false
    
Launch:
$ <out>/chrome --mash --ozone-platform=wayland|x11 --no-sandbox
--user-data-dir=~/tmp

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

[tonikitoo, 2016-10-12 13:44:23]

Addressed Robert's and Sadrul's reviews.

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone...
File chrome/browser/ui/ozone_stubs.cc (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone...
chrome/browser/ui/ozone_stubs.cc:17: bool IsFullScreenMode() {
On 2016/10/11 18:49:47, rjkroege wrote:
> this is weird. It should be in an anonymous block? And it's not used? Delete
it?

Without this stub implementation, I get this:

(..)
chrome/browser/notifications/fullscreen_notification_blocker.cc:49: error:
undefined reference to 'IsFullScreenMode()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Also note that there are win, linux/x11, mac and chromeos implementations of
this method in:

chrome/browser/fullscreen_win.cc
chrome/browser/fullscreen_aurax11.cc
chrome/browser/fullscreen_mac.mm
chrome/browser/fullscreen_chromeos.cc

.. and all are implemented in the anonymous namespace.

the mash implementation is also missing, and is tracked in
https://bugs.chromium.org/p/chromium/issues/detail?id=640390

I am adding chrome/browser/fullscreen_ozone.cc stub'ed out for now.

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:17: #if
!defined(USE_OZONE)
On 2016/10/11 19:14:14, sadrul wrote:
> On 2016/10/11 18:49:47, rjkroege wrote:
> > this looks strange. i.e.: something is not right here but I'm not the right
> > person. +sadrul to comment more about how this should fit?
> 
> What happens if you use this piece of code with ozone?

In patchset #3, in if you look at chrome/browser/ui/BUILD.gn ,
views/frame/desktop_browser_frame_auralinux.cc is not built for 'use_ozone'.

Some reasons:

- desktop_browser_frame_auralinux.cc contains some X11 code. Well, we could
maybe ifdef it out, but decided not to for now.

- DesktopBrowserFrameAuraLinux ctor calls out
BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost which is only
implemented for win and x11 in
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_{win,x11}.cc

My understanding is that for "mus" we do not want this method implemented.

All in all, since we are targeting launching with "--mash", we would enter the
"if" clause in line 14 above.

patchset 4 adds NOTREACHED with some text to explain it.

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:10: gfx::NativeWindow
WindowFinder::GetLocalProcessWindowAtPoint(
On 2016/10/11 19:14:14, sadrul wrote:
> On 2016/10/11 18:49:47, rjkroege wrote:
> > mus should tell you this? i.e.: this code should never get invoked?

Right. This  should never be invoked, but we need a stub implementation, to link
successfully. I will add a  NOTREACHED().

In case of other platforms, we have window_finder_{win,mac,x11,chromeos}.cc
implementing ::GetLocalProcessWindowAtPoint.
 
> There's a window_finder_mus.cc that should be used, yeah.

In practice, when --mash is passed, WindowFinderMus takes place.

https://codereview.chromium.org/2408803002/diff/40001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/40001/chrome/chrome_paks.gni#...
chrome/chrome_paks.gni:53: if (use_ash || use_ozone) {
On 2016/10/11 18:49:47, rjkroege wrote:
> I might be persuaded that this is a temporary expedient. But it's definitely
not
> the right way long term. None of the ash resources should be needed right? So
at
> an absolute minimum, you need a TODO here.

This is true (it is temporary). Will add a TODO here.

[tonikitoo, 2016-10-12 13:46:30]

On 2016/10/11 19:26:56, sadrul wrote:
> (..)
> 
> All of these changes will not have any build/test coverage at the moment
though.
> So when we land this, we need to make sure that we have builders set up so
that
> we know when something breaks (even if it doesn't cause a tree-closure at
first,
> maybe) because otherwise, this will continue to break as more work happens on
> mus+ozone+ash land.

Fred has emailed ozone-dev about the buildbot situation today:
https://groups.google.com/a/chromium.org/forum/#!topic/ozone-dev/5lRcidwo7ik

In the meanwhile, Fred and I commit ourselves to get the build going at a
reasonable pace.

[fwang, 2016-10-12 14:33:15]

https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscre...
File chrome/browser/fullscreen_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscre...
chrome/browser/fullscreen_ozone.cc:8: NOTIMPLEMENTED();
Maybe add a TODO that references
https://bugs.chromium.org/p/chromium/issues/detail?id=640390

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_paks.gni#...
chrome/chrome_paks.gni:54: # There should be an variable that better represents
this build configuratiton.
'a variable'
'configuration'

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_repack_lo...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_repack_lo...
chrome/chrome_repack_locales.gni:44: # There should be an variable that better
represents this build configuratiton.
Ditto.

[tonikitoo, 2016-10-12 14:45:07]

https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscre...
File chrome/browser/fullscreen_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscre...
chrome/browser/fullscreen_ozone.cc:8: NOTIMPLEMENTED();
On 2016/10/12 14:33:15, fwang wrote:
> Maybe add a TODO that references
> https://bugs.chromium.org/p/chromium/issues/detail?id=640390

Done.

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_paks.gni#...
chrome/chrome_paks.gni:54: # There should be an variable that better represents
this build configuratiton.
On 2016/10/12 14:33:15, fwang wrote:
> 'a variable'
> 'configuration'

Done.

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_repack_lo...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_repack_lo...
chrome/chrome_repack_locales.gni:44: # There should be an variable that better
represents this build configuratiton.
On 2016/10/12 14:33:15, fwang wrote:
> Ditto.

Done.

[tonikitoo, 2016-10-12 15:38:06]

This is how Chrome/mash/ozone/wayland/linux_os looks like, with the missing ash
resources issues fixed:
https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chrome...

[rjkroege, 2016-10-12 18:30:29]

On 2016/10/12 15:38:06, tonikitoo wrote:
> This is how Chrome/mash/ozone/wayland/linux_os looks like, with the missing
ash
> resources issues fixed:
>
https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chrome...

dumb question maybe: why does it have the ash tray still? i.e. you're not using
ash. It shouldn't have the tray and that's why the resources are getting asked
for? This suggests that there is ash code being built that is not being excluded
by !ash?

[sadrul, 2016-10-12 18:31:57]

On 2016/10/12 18:30:29, rjkroege wrote:
> On 2016/10/12 15:38:06, tonikitoo wrote:
> > This is how Chrome/mash/ozone/wayland/linux_os looks like, with the missing
> ash
> > resources issues fixed:
> >
>
https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chrome...
> 
> dumb question maybe: why does it have the ash tray still? i.e. you're not
using
> ash. It shouldn't have the tray and that's why the resources are getting asked
> for? This suggests that there is ash code being built that is not being
excluded
> by !ash?

'chrome --mash' will always launch ash (albeit in a separate process from
chrome)

[rjkroege, 2016-10-12 19:52:08]

On 2016/10/12 18:31:57, sadrul wrote:
> On 2016/10/12 18:30:29, rjkroege wrote:
> > On 2016/10/12 15:38:06, tonikitoo wrote:
> > > This is how Chrome/mash/ozone/wayland/linux_os looks like, with the
missing
> > ash
> > > resources issues fixed:
> > >
> >
>
https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chrome...
> > 
> > dumb question maybe: why does it have the ash tray still? i.e. you're not
> using
> > ash. It shouldn't have the tray and that's why the resources are getting
asked
> > for? This suggests that there is ash code being built that is not being
> excluded
> > by !ash?
> 
> 'chrome --mash' will always launch ash (albeit in a separate process from
> chrome)

but use_ash == false? Why is there an ash to even launch?

[not to use - tonikitoo, 2016-10-12 20:50:23]

On 2016/10/12 19:52:08, rjkroege wrote:
> On 2016/10/12 18:31:57, sadrul wrote:
> > On 2016/10/12 18:30:29, rjkroege wrote:
> > > On 2016/10/12 15:38:06, tonikitoo wrote:
> > > > This is how Chrome/mash/ozone/wayland/linux_os looks like, with the
> missing
> > > ash
> > > > resources issues fixed:
> > > >
> > >
> >
>
https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chrome...
> > > 
> > > dumb question maybe: why does it have the ash tray still? i.e. you're not
> > using
> > > ash. It shouldn't have the tray and that's why the resources are getting
> asked
> > > for? This suggests that there is ash code being built that is not being
> > excluded
> > > by !ash?
> > 
> > 'chrome --mash' will always launch ash (albeit in a separate process from
> > chrome)
> 
> but use_ash == false? Why is there an ash to even launch?

Hi Robert/Sadrul.

We are also working on a use_ash=true builds. There is lots of ash stuff guarded
by chromeos guards today, so it needs some more work.

If you guys agree, we can fix it (use_ash=true) in a follow up patch to this
one.

[fwang, 2016-10-13 16:13:12]

> dumb question maybe: why does it have the ash tray still? i.e. you're not
using ash. It shouldn't have the tray and that's why the resources are getting
asked for? This suggests that there is ash code being built that is not being
excluded by !ash?

Hi Robert. Currently, we are building chrome on Linux with
enable_package_mash_services=true (in order to run chrome --mash) so this
depends on //chrome/app/mash:

https://cs.chromium.org/chromium/src/chrome/BUILD.gn?l=250

Then in turn this requires building ash resources:

https://cs.chromium.org/chromium/src/chrome/app/mash/BUILD.gn?l=13

Many of these resources already exist in the chrome pak files
(https://bugs.chromium.org/p/chromium/issues/detail?id=628715) and we just
bundle more of them to avoid missing images and strings...

> but use_ash == false? Why is there an ash to even launch?

...and then //chrome/app/mash also depends on the ash service:

https://cs.chromium.org/chromium/src/chrome/app/mash/BUILD.gn?l=56

Now mash == mus+ash, so I'm not really surprised that ash is launched. The
question is rather why enable_package_mash_services=true does not force
use_ash==true? Maybe sadrul can explain but note that at the moment use_ash=true
and is_chromeos=true seem strongly connected (see e.g.
https://codereview.chromium.org/2358573002/ where OS_CHROMEOS was used instead
of USE_ASH ; we have more tricky build failures with the
chrome/browser/chromeos/arc/arc_auth_service.h header
chrome/browser/chromeos/arc/arc_auth_service.h and other missing ash resources
only generated on chromeos) so it might not be so easy to make CL works with
use_ash = true and chromeos == 0...

[tonikitoo, 2016-10-13 21:04:50]

tonikitoo@igalia.com changed reviewers:
+ forney@google.com

[Tom (Use chromium acct), 2016-10-14 06:36:06]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 06:36:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 07:01:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 07:01:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[not to use - tonikitoo, 2016-10-14 19:43:15]

FYI: The trybots caught tests failing in chromecast (linux) builds. I could
reproduce it, and 'am working on it.

[rjkroege, 2016-10-16 21:07:38]

On 2016/10/14 19:43:15, not to use - tonikitoo wrote:
> FYI: The trybots caught tests failing in chromecast (linux) builds. I could
> reproduce it, and 'am working on it.

Sorry to be difficult. But if you go with my roadmap towards of external-window
mode of using CrOS chrome but having it not launch ash and hence run in external
window mode, then maybe you need to adjust this CL?

[not to use - tonikitoo, 2016-10-17 16:01:25]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 16:01:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 16:47:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 16:47:55]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-10-17 23:40:52]

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:896: main_parts->AddParts(new
ChromeBrowserMainExtraPartsViewsLinux());
I'm a bit worried about this.  ChromeBrowserMainExtraPartsViewsLinux sets up
LinuxUI with the gtk2 ui, which is used mainly for theming.

I feel like we would still want this for wayland on desktop.  Gtk has a wayland
backend (might be only gtk3 or later), so we should still be able to use it.

Alternatively, we could stub out a LinuxUI for ozone for now.  rjkroege@ what do
you think?

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/media/w...
File chrome/browser/media/webrtc/window_icon_util_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/media/w...
chrome/browser/media/webrtc/window_icon_util_ozone.cc:9: #include
"ui/aura/window.h"
nit: probably don't need this window.h include

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_view_prefs.cc:35: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS) && !defined(USE_OZONE)
why is this needed?

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:47: #if
defined(USE_AURA) && !defined(OS_CHROMEOS) && !defined(USE_OZONE)
We're definitely going to need an ozone screen at some point, maybe stub one
out?

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
nit: No "(c)".  Just "Copyright 2016 The Chromium Authors"

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: #include <vector>
nit: can we remove this include?

[tonikitoo, 2016-10-18 04:55:06]

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:896: main_parts->AddParts(new
ChromeBrowserMainExtraPartsViewsLinux());
Here are the reasons to originally ifdef'out the gtk2 entry point,
ChromeBrowserMainExtraPartsViewsLinux:

1) the gtk2 theming code is already not build for use_ozone (see the related
snippet from chrome/browser/ui/BUILD.gn)

  (..)
  if (use_aura && !use_ozone && is_desktop_linux) {
    deps += [
      # gtk2 is the only component that can interact with gtk2 in our new
      # world.
      "//chrome/browser/ui/libgtk2ui",
    ]
  }
  (..)

2) glib itself is disabled for ozone builds (see the related snippet from
build/config/ui.gni):

  (..)
  # Turn off glib if Ozone is enabled.
  if (use_ozone) {
    use_glib = false
  }
  (..)

PS: should/can gtk{2,3} work with glib support disabled?

I also thought of stub'ing LinuxUI out, but given that LinuxUI class has so many
pure virtual methods, and this CL is a "bring up" CL, it seemed not the best
approach.

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/media/w...
File chrome/browser/media/webrtc/window_icon_util_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/media/w...
chrome/browser/media/webrtc/window_icon_util_ozone.cc:9: #include
"ui/aura/window.h"
On 2016/10/17 23:40:52, Tom Anderson wrote:
> nit: probably don't need this window.h include

Done.

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_view_prefs.cc:35: #if defined(OS_LINUX) &&
!defined(OS_CHROMEOS) && !defined(USE_OZONE)
On 2016/10/17 23:40:52, Tom Anderson wrote:
> why is this needed?

This is needed because ui::GetCustomFramePrefDefault (from ui/base/x/x11_util.cc
| h) is only built for use_x11 or wayland_platform_x11 builds.

Ideally, there should be a ui/base/ozone/ozone_util.cc implementing this
function, but for this bring-up, I decided to simple ifdef it out. Maybe a TODO
would be enough for now?

Note that CHROMEOS builds, even when running with ozone/x11, do not run this
method.

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:47: #if
defined(USE_AURA) && !defined(OS_CHROMEOS) && !defined(USE_OZONE)
On 2016/10/17 23:40:52, Tom Anderson wrote:
> We're definitely going to need an ozone screen at some point, maybe stub one
> out?

we are using screen_mus.

Calling this as is simply hit an assert in 

FATAL:desktop_factory_ozone.cc(21)] Check failed: impl_. DesktopFactoryOzone
accessed before constructed
#0 0x7f15e92f291e base::debug::StackTrace::StackTrace()
#1 0x7f15e9314940 logging::LogMessage::~LogMessage()
#2 0x7f15e49a36f8 views::DesktopFactoryOzone::GetInstance()
#3 0x7f15e49a3736 views::CreateDesktopScreen()
#4 0x55e5736aa332 ChromeBrowserMainExtraPartsViews::PreCreateThreads()
(..)

I think DesktopFactoryOzone is obsolete.

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/10/17 23:40:52, Tom Anderson wrote:
> nit: No "(c)".  Just "Copyright 2016 The Chromium Authors"

Done.

https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: #include <vector>
On 2016/10/17 23:40:52, Tom Anderson wrote:
> nit: can we remove this include?

Done.

[Tom (Use chromium acct), 2016-10-20 04:37:16]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 04:37:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 05:34:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 05:34:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[not to use - tonikitoo, 2016-10-21 04:41:25]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 04:41:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 06:55:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 06:55:17]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-10-25 19:30:31]

Pinging other reviewers on behalf of tonikitoo@

rjkroge@ maybe if this isn't a step in the right direction, we can still land
this anyway, so that we can get an optional linux ozone builder set up?

[rjkroege, 2016-10-25 19:57:43]

On 2016/10/25 19:30:31, Tom Anderson wrote:
> Pinging other reviewers on behalf of tonikitoo@
> 
> rjkroge@ maybe if this isn't a step in the right direction, we can still land
> this anyway, so that we can get an optional linux ozone builder set up?

Sorry for the delay. I'm not sure that we want it end up like this. But with a
builder setup, the path can meander a little bit as we sort out how it's going
to work.

[rjkroege, 2016-10-25 20:10:40]

I've been persuaded that so long as we get an FYI builder, it's reasonable to
continue in this direction and work towards mus convergence.

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/fullscr...
File chrome/browser/fullscreen_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/fullscr...
chrome/browser/fullscreen_ozone.cc:7: bool IsFullScreenMode() {
This can be refactored out of existence: rename fullscreen_chromeos.cc to
fullscreen_ozone.cc. And you'll get the correct behaviour.

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/media/w...
File chrome/browser/media/webrtc/window_icon_util_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/media/w...
chrome/browser/media/webrtc/window_icon_util_ozone.cc:11:
DCHECK_EQ(content::DesktopMediaID::TYPE_WINDOW, id.type);
forthcoming changes in the mash/aura client code should make this unnecessary?

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: 
This should be unnecessary? You will have a service manager with --mash?

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:5: #include <set>
Can you refactor this to use window_finder_mus? It's what you'll want anyway
right? This seems unnecessary.

https://codereview.chromium.org/2408803002/diff/140001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/chrome_paks.gni...
chrome/chrome_paks.gni:54: # TODO(tonikitoo): On Ozone builds, we are (ab)using
some ash resources.
That this build config launches ash (or needs it) is a bug yes?. You could
revise the TODO. This should go away eventually

https://codereview.chromium.org/2408803002/diff/140001/chrome/chrome_repack_l...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/chrome_repack_l...
chrome/chrome_repack_locales.gni:43: # TODO(tonikitoo): On Ozone builds, we are
(ab)using some ash resources.
same comment.

[sadrul, 2016-10-26 04:37:33]

Note that to run chrome with mus (chrome --mash), chrome itself does not need to
use ozone (and in fact, chrome must not use ozone, because chrome shouldn't talk
to the native platform). The mus window server needs to use ozone, of course,
since the ws is the one that talks to the native platform.

[sadrul, 2016-10-26 04:44:10]

On 2016/10/26 04:37:33, sadrul wrote:
> Note that to run chrome with mus (chrome --mash), chrome itself does not need
to
> use ozone (and in fact, chrome must not use ozone, because chrome shouldn't
talk
> to the native platform). The mus window server needs to use ozone, of course,
> since the ws is the one that talks to the native platform.

What we would want is to be able to build chrome without having any dependency
on x11. i.e. a version of chrome that only knows how to talk to mus. Using the
ozone config may be an expedient way to get rid of the x11 dependency, but not
sure if there are no better alternatives.

[tonikitoo, 2016-10-26 05:37:17]

Started to address Robert's request in separate CLs. Will rebase this one
against them, once merged.

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/fullscr...
File chrome/browser/fullscreen_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/fullscr...
chrome/browser/fullscreen_ozone.cc:7: bool IsFullScreenMode() {
On 2016/10/25 20:10:39, rjkroege wrote:
> This can be refactored out of existence: rename fullscreen_chromeos.cc to
> fullscreen_ozone.cc. And you'll get the correct behaviour.

1st take done in https://codereview.chromium.org/2453703002

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:5: #include <set>
On 2016/10/25 20:10:40, rjkroege wrote:
> Can you refactor this to use window_finder_mus? It's what you'll want anyway
> right? This seems unnecessary.

1st take done in https://codereview.chromium.org/2449103004 .

[tonikitoo, 2016-11-02 22:46:01]

On 2016/10/11 19:26:56, sadrul wrote:
> Some of the changes in this CL fall in the 'obviously correct' category (e.g.
> USE_X11 check before using X11-specific code, in browser_frame.cc). Others are
> less obvious, and it would be useful to understand those better (e.g. perhaps
> add comments in the CL so that we can continue the discussion in here?)

Hi all.

Over the past few days, I had split this CL into various smaller ones, applying
the "obviously correct" category, as per @sadrul's comment above.

Namely:

- Only call X11DesktopHandler for USE_X11 builds
  https://codereview.chromium.org/2454743002/

- Build some x11 dependent files only if 'use_x11'
  https://codereview.chromium.org/2457443004/

- Condition the libsecret use on chrome desktop builds to 'use_glib'
  https://codereview.chromium.org/2452063002/

- Refactor WindowFinder definition
  https://codereview.chromium.org/2449103004/

- Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone
  https://codereview.chromium.org/2462423002/

As a result, this CL is much more review-able IMO, and hopefully you guys also
agree.

> All of these changes will not have any build/test coverage at the moment
though.
> So when we land this, we need to make sure that we have builders set up so
that
> we know when something breaks (even if it doesn't cause a tree-closure at
first,
> maybe) because otherwise, this will continue to break as more work happens on
> mus+ozone+ash land.

Actually, @thomasanderson and @dpranke have been working on a linux/ozone bot
lately, and it is up and running (although no green yet):

- Use ozone config on Linux Ozone bot
  https://codereview.chromium.org/2466383003

- Add Linux Ozone fyi compile-only bot
  https://codereview.chromium.org/2463523002

- Add Linux Ozone fyi compile-only bot (src-side changes)
  https://codereview.chromium.org/2470453002

... so, when this CL hopefully gets merged, we can make this bot green, and
continue with the effort of making chrome/mash run in window-mode (in agreement
with @rjkroege's plan).

PTAL

[not to use - tonikitoo, 2016-11-02 22:49:09]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 22:49:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2016-11-02 23:02:04]

All of this makes sense to me
non-owners LGTM

[commit-bot: I haz the power, 2016-11-03 00:05:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 00:05:58]

Dry run: This issue passed the CQ dry run.

[fwang, 2016-11-03 07:56:22]

https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: 
Why this new line?

[tonikitoo, 2016-11-03 12:52:29]

https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: 
On 2016/11/03 07:56:22, fwang wrote:
> Why this new line?

should not be needed. Will fix.

BTW, window_finder_ozone.cc is a new file being added, but codereview shows the
diff of this file (window_finder_ozone.cc) against window_finder_chromeos.cc,
which is confusing.

[not to use - tonikitoo, 2016-11-03 17:25:17]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 17:25:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 18:18:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 18:18:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not to use - tonikitoo, 2016-11-03 18:39:13]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 18:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 19:45:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 19:45:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tonikitoo, 2016-11-04 18:45:08]

Description was changed from

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0
    
CL provides a first step for running with chrome/chromium
natively on Wayland and X11, through Ozone.
    
Some method are stub'ed for now, intention is implementing
them on follow up patches.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
    
  enable_package_mash_services = true
  use_ash = false
    
Launch:
$ <out>/chrome --mash --ozone-platform=wayland|x11 --no-sandbox
--user-data-dir=~/tmp

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

to

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

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

[tonikitoo, 2016-11-04 18:51:36]

tonikitoo@igalia.com changed reviewers:
+ sky@chromium.org

[tonikitoo, 2016-11-04 18:51:37]

sky@chromium.org: Please review changes. I tried to summarize the work so far in
comment #53 (https://codereview.chromium.org/2408803002/#msg53)

[sky, 2016-11-04 21:40:37]

https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:3548: "fullscreen_auramus.cc",
Wouldn't _ozone be a better name to use here? aura-mus is not necessarily
exclusive to ozone.

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni...
chrome/chrome_paks.gni:57: if (use_ash || use_ozone) {
This is fragile and I worry that it's going to be easy for us to break. You need
to find a better way for this.

[tonikitoo, 2016-11-04 22:02:57]

https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:3548: "fullscreen_auramus.cc",
On 2016/11/04 21:40:37, sky wrote:
> Wouldn't _ozone be a better name to use here? aura-mus is not necessarily
> exclusive to ozone.

Done.

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni...
chrome/chrome_paks.gni:57: if (use_ash || use_ozone) {
On 2016/11/04 21:40:37, sky wrote:
> This is fragile and I worry that it's going to be easy for us to break. You
need
> to find a better way for this.

This is true, and the TODO is because of this. But since, this is a bring-up CL,
even with this piece being fragile, it might still be worth to give it a try on
the short term, and fix it in a follow up.

in patchset 12 , I replaced use_ozone by enable_package_mash_services, which
actually makes more sense to me, since it fixes chrome (x11/linux) --mash too.

wdyt?

[tonikitoo, 2016-11-04 23:10:45]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 23:11:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-11-04 23:18:40]

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni
File chrome/chrome_paks.gni (right):

https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni...
chrome/chrome_paks.gni:57: if (use_ash || use_ozone) {
On 2016/11/04 22:02:56, tonikitoo wrote:
> On 2016/11/04 21:40:37, sky wrote:
> > This is fragile and I worry that it's going to be easy for us to break. You
> need
> > to find a better way for this.
> 
> This is true, and the TODO is because of this. But since, this is a bring-up
CL,
> even with this piece being fragile, it might still be worth to give it a try
on
> the short term, and fix it in a follow up.
> 
> in patchset 12 , I replaced use_ozone by enable_package_mash_services, which
> actually makes more sense to me, since it fixes chrome (x11/linux) --mash too.
> 
> wdyt?
> 

I'm still confused by this. What are you trying to compile that is needing ash
resources? You should either be defining use_ash, or don't compile the ash files
that are attempting to pull in ash resources.

[tonikitoo, 2016-11-05 00:06:28]

On 2016/11/04 23:18:40, sky wrote:
> https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni
> File chrome/chrome_paks.gni (right):
> 
>
https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni...
> chrome/chrome_paks.gni:57: if (use_ash || use_ozone) {
> On 2016/11/04 22:02:56, tonikitoo wrote:
> > On 2016/11/04 21:40:37, sky wrote:
> > > This is fragile and I worry that it's going to be easy for us to break.
You
> > need
> > > to find a better way for this.
> > 
> > This is true, and the TODO is because of this. But since, this is a bring-up
> CL,
> > even with this piece being fragile, it might still be worth to give it a try
> on
> > the short term, and fix it in a follow up.
> > 
> > in patchset 12 , I replaced use_ozone by enable_package_mash_services, which
> > actually makes more sense to me, since it fixes chrome (x11/linux) --mash
too.
> > 
> > wdyt?
> > 
> 
> I'm still confused by this. What are you trying to compile that is needing ash
> resources? You should either be defining use_ash, or don't compile the ash
files
> that are attempting to pull in ash resources.

Basically this is first step towards a bigger goal, which is running
chrome/ozone with mus (no ash).

In the current incarnation, CL makes it possible to run "chrome --mash" on a
linux_os build (i.e. non-chromeos). In this case, "mash" does set dependency on
//chrome/app/mash, which does depend on ash.

CL also allows us to have a FYI bot set for chrome/linux/ozone.

In practice, without the ash_resources bundled into chrome, it launches like
this https://goo.gl/9mbQA5 (see the missing resources).
On the other hand, with the ash_resource bundled, it launches like
https://goo.gl/0R9Sl3

Next step would be actually make chrome running independently from ash but still
using mus (probably by adding a launch parameter sibling to --mash). At this
point, we could likely remove this piece of code, and the TODO.

[commit-bot: I haz the power, 2016-11-05 00:29:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 00:29:57]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2016-11-05 03:55:09]

In patchset #13, I am moving out of this CL the part that bundles ash resources
into chrome, as can proceed here without it and discuss it offline.

@sky, PTAL.

[tonikitoo, 2016-11-05 04:23:17]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 04:23:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 05:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 05:28:29]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-07 16:28:00]

https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if
defined(USE_OZONE)
native_browser_frame_factory_ozone?

[tonikitoo, 2016-11-07 16:36:20]

https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if
defined(USE_OZONE)
On 2016/11/07 16:28:00, sky wrote:
> native_browser_frame_factory_ozone?

Done.

[tonikitoo, 2016-11-07 17:01:18]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 17:01:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-11-07 17:18:52]

https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if
defined(USE_OZONE)
You shouldn't need this ifdef anymore, right?

[tonikitoo, 2016-11-07 17:22:18]

https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc
(right):

https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if
defined(USE_OZONE)
On 2016/11/07 17:18:52, sky wrote:
> You shouldn't need this ifdef anymore, right?

Sorry, forgot to git add the change locally. It is gone.

[fwang, 2016-11-07 18:12:16]

informal l g t m

[rjkroege, 2016-11-07 20:07:59]

lgtm  

This seems reasonable to me. Thanks for your persistence. I'm looking forward to
the rest. :-)

https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/fullscr...
File chrome/browser/fullscreen_ozone.cc (right):

https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/fullscr...
chrome/browser/fullscreen_ozone.cc:16: NOTREACHED() << "For Ozone builds, only
--mash launch is supported for now.";
It's conceivable that we never  want anything else. But that's a discussion for
a different day.

[sky, 2016-11-07 21:48:11]

LGTM

https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:3055:
"views/frame/native_browser_frame_factory_auralinux.cc",
For clarity this should be renamed to x11, but that can happen later.

[tonikitoo, 2016-11-07 21:51:46]

The CQ bit was checked by tonikitoo@igalia.com

[tonikitoo, 2016-11-07 21:51:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasanderson@google.com
Link to the patchset: https://codereview.chromium.org/2408803002/#ps280001
(title: "reverted changes to native_browser_frame_factory_auralinux.cc")

[commit-bot: I haz the power, 2016-11-07 21:52:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 23:52:29]

Description was changed from

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

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

to

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

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

[commit-bot: I haz the power, 2016-11-07 23:52:31]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-07 23:58:03]

Description was changed from

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.
    
GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

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

to

==========
Make it possible to launch chrome --mash with ozone_platform={wayland|x11},
chromeos=0

As of now, chrome --mash is possible to get launched on regular
chrome desktop Linux builds (with enable_package_mash_services = true)
and on chromeos builds.

CL provides a first step for running with chrome --mash
natively on both Wayland and X11, through Ozone.
It includes 3 stub'ed methods for now, to be implemented follow up CLs.

GN args:
  use_ozone = true
  ozone_platform_wayland = true
  ozone_platform_x11 = true
  ozone_auto_platforms = false
  enable_package_mash_services = true

Additionally, CL also makes it possible to turn green the FYI
linux/ozone bot, worked out by thomasanderson@ et al.

TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp

BUG=295089

Committed: https://crrev.com/b10ea66fcc43a0eed828eb62ffb2bd141b825fce
Cr-Commit-Position: refs/heads/master@{#430424}
==========

[commit-bot: I haz the power, 2016-11-07 23:58:05]

Patchset 15 (id:??) landed as
https://crrev.com/b10ea66fcc43a0eed828eb62ffb2bd141b825fce
Cr-Commit-Position: refs/heads/master@{#430424}