components: Add Exosphere component.

components: Add Exosphere component.

Exosphere is a set of display server classes for Chromium that
can be used for building a display server on top of Ash, the
Chrome Compositor and the GpuMemoryBuffer framework.

The classes and interfaces are inspired by the Wayland protocol
and reference implementation of a display server that uses
the Wayland protocol and server library is provided in the
wayland/ sub-directory.

BUG=549781
TEST=components_unittests --gtest_filter=ServerTest.*:SurfaceTest.*:BufferTest.*:DisplayTest.*:ShellSurfaceTest.*:SharedMemoryTest.*

Committed: https://crrev.com/b195f41daf755c80d31f964538b1159c81d58e70
Cr-Commit-Position: refs/heads/master@{#360672}

[reveman, 2015-10-28 04:25:19]

Description was changed from

==========
components: Add Exosphere component.

Exosphere is a set of display server classes for Chromium that
can be used for building a display server on top of Ash, the
Chrome Compositor and the GpuMemoryBuffer framework.

The classes and interfaces are inspired by the Wayland protocol
and reference implementation of a display server that uses
the Wayland protocol and server library is provided in the
wayland/ sub-directory.

TEST=components_unittests
--gtest_filter=ServerTest.*:SurfaceTest.*:BufferTest.*:DisplayTest.*
BUG=
==========

to

==========
components: Add Exosphere component.

Exosphere is a set of display server classes for Chromium that
can be used for building a display server on top of Ash, the
Chrome Compositor and the GpuMemoryBuffer framework.

The classes and interfaces are inspired by the Wayland protocol
and reference implementation of a display server that uses
the Wayland protocol and server library is provided in the
wayland/ sub-directory.

TEST=components_unittests
--gtest_filter=ServerTest.*:SurfaceTest.*:BufferTest.*:DisplayTest.*:ShellSurfaceTest.*:SharedMemoryTest.*
BUG=
==========

[reveman, 2015-11-04 02:13:14]

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

[lpique, 2015-11-05 21:25:01]

https://codereview.chromium.org/1412093006/diff/340001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/1412093006/diff/340001/components/exo/surface...
components/exo/surface.cc:107: if (frame_callbacks_.empty() &&
active_frame_callbacks_.empty())
Shouldn't you also check "&& !frame_callbacks_.empty()"?

https://codereview.chromium.org/1412093006/diff/340001/components/exo/surface...
components/exo/surface.cc:111: frame_callbacks_.splice(frame_callbacks_.end(),
pending_frame_callbacks_);
Under the calling sequence:

  Surface::RequestFrameCallback(cb1);
  Surface::Commit();
  Surface::RequestFrameCallback(cb2);
  Surface::Commit();

If these calls all happen before the first commit causes an
OnCompositingDidCommit() to be received for it, the second callback ends up
being called when the first commit completes.

(I observed this happening with my client, as the initial surfaces are committed
this way with no real delay)

[reveman, 2015-11-07 19:59:52]

Description was changed from

==========
components: Add Exosphere component.

Exosphere is a set of display server classes for Chromium that
can be used for building a display server on top of Ash, the
Chrome Compositor and the GpuMemoryBuffer framework.

The classes and interfaces are inspired by the Wayland protocol
and reference implementation of a display server that uses
the Wayland protocol and server library is provided in the
wayland/ sub-directory.

TEST=components_unittests
--gtest_filter=ServerTest.*:SurfaceTest.*:BufferTest.*:DisplayTest.*:ShellSurfaceTest.*:SharedMemoryTest.*
BUG=
==========

to

==========
components: Add Exosphere component.

Exosphere is a set of display server classes for Chromium that
can be used for building a display server on top of Ash, the
Chrome Compositor and the GpuMemoryBuffer framework.

The classes and interfaces are inspired by the Wayland protocol
and reference implementation of a display server that uses
the Wayland protocol and server library is provided in the
wayland/ sub-directory.

BUG=549781
TEST=components_unittests
--gtest_filter=ServerTest.*:SurfaceTest.*:BufferTest.*:DisplayTest.*:ShellSurfaceTest.*:SharedMemoryTest.*
==========

[lpique, 2015-11-11 19:22:08]

LGTM, with one minor nit.

Sorry for the delay. I had too many distractions.

https://codereview.chromium.org/1412093006/diff/520001/components/exo/shared_...
File components/exo/shared_memory.h (right):

https://codereview.chromium.org/1412093006/diff/520001/components/exo/shared_...
components/exo/shared_memory.h:33: int stride);
It's a bit inconsistent that offset is unsigned and stride is not.

[reveman, 2015-11-11 19:45:19]

thanks

https://codereview.chromium.org/1412093006/diff/520001/components/exo/shared_...
File components/exo/shared_memory.h (right):

https://codereview.chromium.org/1412093006/diff/520001/components/exo/shared_...
components/exo/shared_memory.h:33: int stride);
On 2015/11/11 at 19:22:07, lpique wrote:
> It's a bit inconsistent that offset is unsigned and stride is not.

Negative stride is a valid use-case even though we don't support that yet. it
can be used to flip the scanline order of a buffer from top-down to bottom-up.
Most of the buffer related code in chromium support negative stride and we might
want the implementation of this function support that in the future. The lack of
support here is an implementation detail and I prefer to not expose that in the
API.

[reveman, 2015-11-14 00:58:32]

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

[reveman, 2015-11-14 00:58:33]

piman, please take a look at this when you have a chance.

https://codereview.chromium.org/1412093006/diff/560001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/560001/ash/BUILD.gn#newcode201
ash/BUILD.gn:201: deps += [ "//ui/display" ]
missing dependency. likely missed because ash_unittests has additional code that
requires an explicit dependency at that level.

[piman, 2015-11-17 02:43:53]

Comments so far, I only made it up to surface.cc, I still have to take a look at
the rest.

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/BUILD.gn:43: if (is_linux) {
Do we need this on anything but chromeos at this point? It pulls in a bunch of
deps, and I'd prefer to avoid bloat.

Maybe we could define a use_exosphere option / USE_EXOSPHERE define so that it's
easy to enable selectively, and we can bring it on linux later.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer.cc
File components/exo/buffer.cc (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer....
components/exo/buffer.cc:119: gles2->GenMailboxCHROMIUM(mailbox.name);
mailboxe names are not free to create (crypto-secure random), so, assuming
Buffer are reused, I'd suggest recycling them, e.g. keeping a list of "free"
mailbox names, that get filled in Buffer::Release.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer.h
File components/exo/buffer.h (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer....
components/exo/buffer.h:52: scoped_ptr<cc::SingleReleaseCallback>
GetTextureMailbox(
nit: AcquireTextureMailbox? GetTextureMailbox makes it look idempotent.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/display.h
File components/exo/display.h (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/display...
components/exo/display.h:36: scoped_ptr<ShellSurface> GetShellSurface(Surface*
surface);
nit: maybe CreateShellSurface

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface...
components/exo/surface.cc:48: });
nit:
for (const auto& frame_callback : active_frame_callbacks_)
  frame_callback.Run(base::TimeTicks());

is shorter and more readable.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface...
components/exo/surface.cc:51: void Surface::Attach(Buffer* buffer, const
gfx::Point& point) {
What is |point| used for?

[reveman, 2015-11-17 18:04:04]

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/BUILD.gn:43: if (is_linux) {
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> Do we need this on anything but chromeos at this point? It pulls in a bunch of
deps, and I'd prefer to avoid bloat.

Fyi, this only pulls in //third_party/wayland:wayland_server and that's ~4k
lines of C code and the .a that we produce for that is only 3450 bytes on my x64
machine.

> 
> Maybe we could define a use_exosphere option / USE_EXOSPHERE define so that
it's easy to enable selectively, and we can bring it on linux later.

use_exosphere sgtm. I'll add that as a last step when everything else looks
good. It's nice to not have any artificial build constraints and verify that
this builds and runs on all platforms and all configurations possible while
making changes but it doesn't make sense to land that.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer.cc
File components/exo/buffer.cc (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer....
components/exo/buffer.cc:119: gles2->GenMailboxCHROMIUM(mailbox.name);
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> mailboxe names are not free to create (crypto-secure random), so, assuming
Buffer are reused, I'd suggest recycling them, e.g. keeping a list of "free"
mailbox names, that get filled in Buffer::Release.

Good point. Yes, buffers are meant to be reused. Buffer creation is considered
expensive while buffer reuse is considered cheap. A buffer can only be
associated with one surface at the time so I simply moved this to the ctor in
latest patch.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer.h
File components/exo/buffer.h (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/buffer....
components/exo/buffer.h:52: scoped_ptr<cc::SingleReleaseCallback>
GetTextureMailbox(
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> nit: AcquireTextureMailbox? GetTextureMailbox makes it look idempotent.

Done.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/display.h
File components/exo/display.h (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/display...
components/exo/display.h:36: scoped_ptr<ShellSurface> GetShellSurface(Surface*
surface);
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> nit: maybe CreateShellSurface

Done.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface...
components/exo/surface.cc:48: });
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> nit:
> for (const auto& frame_callback : active_frame_callbacks_)
>   frame_callback.Run(base::TimeTicks());
> 
> is shorter and more readable.

Done.

https://codereview.chromium.org/1412093006/diff/600001/components/exo/surface...
components/exo/surface.cc:51: void Surface::Attach(Buffer* buffer, const
gfx::Point& point) {
On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> What is |point| used for?

This was supposed to be an offset and not a point. Specifies the origin of the
buffer relative to the current buffer's origin and makes it possible to control
in which direction the content is changing size. Removed it for now as this
class should not contain unimplemented functionality. Wayland bindings are now
instead generating an error if the client tries to use this.

[reveman, 2015-11-18 00:21:47]

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/BUILD.gn:43: if (is_linux) {
On 2015/11/17 at 18:04:04, reveman wrote:
> On 2015/11/17 at 02:43:53, piman (slow to review) wrote:
> > Do we need this on anything but chromeos at this point? It pulls in a bunch
of deps, and I'd prefer to avoid bloat.
> 
> Fyi, this only pulls in //third_party/wayland:wayland_server and that's ~4k
lines of C code and the .a that we produce for that is only 3450 bytes on my x64
machine.
> 
> > 
> > Maybe we could define a use_exosphere option / USE_EXOSPHERE define so that
it's easy to enable selectively, and we can bring it on linux later.
> 
> use_exosphere sgtm. I'll add that as a last step when everything else looks
good. It's nice to not have any artificial build constraints and verify that
this builds and runs on all platforms and all configurations possible while
making changes but it doesn't make sense to land that.

Added enable_wayland_server option to latest patch now that I know that this
builds and runs on all possible configurations. I think enable_wayland_server
makes more sense than use_exosphere as exosphere is just a dependency required
to provide wayland server support. enable_wayland_server = is_chromeos by
default but can be enabled on linux.

[reveman, 2015-11-18 00:22:04]

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

[commit-bot: I haz the power, 2015-11-18 00:22:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/700001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/700001

[commit-bot: I haz the power, 2015-11-18 00:38:08]

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

[commit-bot: I haz the power, 2015-11-18 00:38:08]

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

[piman, 2015-11-18 04:31:51]

https://codereview.chromium.org/1412093006/diff/700001/components/exo/test/ex...
File components/exo/test/exo_test_helper.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/test/ex...
components/exo/test/exo_test_helper.cc:28: .Pass();
nit: should not need Pass()

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/server.cc:26: #include
"components/exo/wayland/wayland_bindings.h"
This is surprising to say the least to include from a namespace. Is there
something wrong with simply declaring the contents of wayland_bindings.h inside
the exo::wayland namespace (maybe marking functions as static)?

Or even, you need so little from that .h, maybe move everything into a .cc file
and only expose the few things you need from the .cc? That would also avoid
double-definitions of the global structs, etc.

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/wayland_bindings.h (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/wayland_bindings.h:12: T* DataAs(void* data) {
nit: this is only used in a single place (l.18), maybe just use a
static_cast<T*> there?

[reveman, 2015-11-18 06:48:36]

ptal

https://codereview.chromium.org/1412093006/diff/700001/components/exo/test/ex...
File components/exo/test/exo_test_helper.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/test/ex...
components/exo/test/exo_test_helper.cc:28: .Pass();
On 2015/11/18 at 04:31:51, piman (slow to review) wrote:
> nit: should not need Pass()

Done.

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/server.cc:26: #include
"components/exo/wayland/wayland_bindings.h"
On 2015/11/18 at 04:31:51, piman (slow to review) wrote:
> This is surprising to say the least to include from a namespace. Is there
something wrong with simply declaring the contents of wayland_bindings.h inside
the exo::wayland namespace (maybe marking functions as static)?

Right, this include stuff comes from an attempt to auto-generate this code but
I'm no longer sure that's worth the effort.

> 
> Or even, you need so little from that .h, maybe move everything into a .cc
file and only expose the few things you need from the .cc? That would also avoid
double-definitions of the global structs, etc.

I just removed the header and put the code here in the anonymous namespace for
now. If this grows significantly then it might be worth splitting into separate
files. Does that seem reasonable?

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/wayland_bindings.h (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/wayland_bindings.h:12: T* DataAs(void* data) {
On 2015/11/18 at 04:31:51, piman (slow to review) wrote:
> nit: this is only used in a single place (l.18), maybe just use a
static_cast<T*> there?

Done.

https://codereview.chromium.org/1412093006/diff/720001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/1412093006/diff/720001/components/exo/shell_s...
components/exo/shell_surface.cc:151: views::NonClientFrameView*
ShellSurface::CreateNonClientFrameView(
Note: added this as using TYPE_WINDOW_FRAMELESS makes Ash handle the window in a
way  that is not what we want for apps.

[reveman, 2015-11-18 06:48:50]

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

[commit-bot: I haz the power, 2015-11-18 06:49:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/720001

[commit-bot: I haz the power, 2015-11-18 06:53:57]

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

[commit-bot: I haz the power, 2015-11-18 06:53:58]

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

[reveman, 2015-11-18 07:20:00]

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

[commit-bot: I haz the power, 2015-11-18 07:20:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/740001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/740001

[commit-bot: I haz the power, 2015-11-18 07:29:09]

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

[commit-bot: I haz the power, 2015-11-18 07:29:11]

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

[piman, 2015-11-18 17:35:12]

lgtm

lgtm

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/server.cc:26: #include
"components/exo/wayland/wayland_bindings.h"
On 2015/11/18 06:48:36, reveman wrote:
> On 2015/11/18 at 04:31:51, piman (slow to review) wrote:
> > This is surprising to say the least to include from a namespace. Is there
> something wrong with simply declaring the contents of wayland_bindings.h
inside
> the exo::wayland namespace (maybe marking functions as static)?
> 
> Right, this include stuff comes from an attempt to auto-generate this code but
> I'm no longer sure that's worth the effort.
> 
> > 
> > Or even, you need so little from that .h, maybe move everything into a .cc
> file and only expose the few things you need from the .cc? That would also
avoid
> double-definitions of the global structs, etc.
> 
> I just removed the header and put the code here in the anonymous namespace for
> now. If this grows significantly then it might be worth splitting into
separate
> files. Does that seem reasonable?

Yep, SG, thanks.

[piman, 2015-11-18 17:35:12]

lgtm

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1412093006/diff/700001/components/exo/wayland...
components/exo/wayland/server.cc:26: #include
"components/exo/wayland/wayland_bindings.h"
On 2015/11/18 06:48:36, reveman wrote:
> On 2015/11/18 at 04:31:51, piman (slow to review) wrote:
> > This is surprising to say the least to include from a namespace. Is there
> something wrong with simply declaring the contents of wayland_bindings.h
inside
> the exo::wayland namespace (maybe marking functions as static)?
> 
> Right, this include stuff comes from an attempt to auto-generate this code but
> I'm no longer sure that's worth the effort.
> 
> > 
> > Or even, you need so little from that .h, maybe move everything into a .cc
> file and only expose the few things you need from the .cc? That would also
avoid
> double-definitions of the global structs, etc.
> 
> I just removed the header and put the code here in the anonymous namespace for
> now. If this grows significantly then it might be worth splitting into
separate
> files. Does that seem reasonable?

Yep, SG, thanks.

[reveman, 2015-11-18 18:05:13]

reveman@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org

[reveman, 2015-11-18 18:05:14]

+jochen for build/, components/BUILD.gn and components/*.gyp*
+sky for chrome/browser/ui/ and ash/

[sky, 2015-11-18 21:01:22]

https://codereview.chromium.org/1412093006/diff/740001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/740001/ash/BUILD.gn#newcode201
ash/BUILD.gn:201: deps += [ "//ui/display" ]
Is this specific to wayland? Just a fix that should have been there before?

https://codereview.chromium.org/1412093006/diff/740001/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h (right):

https://codereview.chromium.org/1412093006/diff/740001/chrome/browser/ui/view...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h:34:
scoped_ptr<exo::Display> exo_display_;
Does this code need to be in ChromeBrowserMainExtraPartsAsh? Could it be moved
into your own subclass of ChromeBrowserMainExtraParts, say
ChromeBrowserMainExtraPartsWayland? If that isn't possible, could you create a
class that contains the init code that is owned by this one?

[reveman, 2015-11-18 23:55:18]

ptal

https://codereview.chromium.org/1412093006/diff/740001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/1412093006/diff/740001/ash/BUILD.gn#newcode201
ash/BUILD.gn:201: deps += [ "//ui/display" ]
On 2015/11/18 at 21:01:21, sky wrote:
> Is this specific to wayland? Just a fix that should have been there before?

Yes, this is just fixing a missing dependency, test_support sources use this.

I noticed this when trying to use //ash/test_support without an explicit
//ui/display dependency. Looks like this was not a problem in the past as all
components that depend on //ash/test_support use //ui/display directly..

https://codereview.chromium.org/1412093006/diff/740001/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h (right):

https://codereview.chromium.org/1412093006/diff/740001/chrome/browser/ui/view...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h:34:
scoped_ptr<exo::Display> exo_display_;
On 2015/11/18 at 21:01:21, sky wrote:
> Does this code need to be in ChromeBrowserMainExtraPartsAsh? Could it be moved
into your own subclass of ChromeBrowserMainExtraParts, say
ChromeBrowserMainExtraPartsWayland? If that isn't possible, could you create a
class that contains the init code that is owned by this one?

Sure, moved it to ChromeBrowserMainExtraPartsExo in latest patch. Exo suffix
rather than Wayland as the exo::Display instance used by the wayland bindings
need to be shared with non-wayland code in the near future. The idea is that
...ExtraPartsExo will own the display and initialize all exosphere bindings.

[reveman, 2015-11-18 23:55:26]

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

[commit-bot: I haz the power, 2015-11-18 23:57:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/760001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/760001

[commit-bot: I haz the power, 2015-11-19 00:04:47]

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

[commit-bot: I haz the power, 2015-11-19 00:04:48]

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

[reveman, 2015-11-19 00:14:58]

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

[commit-bot: I haz the power, 2015-11-19 00:16:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/780001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/780001

[commit-bot: I haz the power, 2015-11-19 01:40:24]

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

[commit-bot: I haz the power, 2015-11-19 01:40:25]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[reveman, 2015-11-19 06:22:04]

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

[commit-bot: I haz the power, 2015-11-19 06:22:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/800001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/800001

[commit-bot: I haz the power, 2015-11-19 06:33:16]

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

[commit-bot: I haz the power, 2015-11-19 06:33:17]

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

[reveman, 2015-11-19 06:34:49]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2015-11-19 06:35:33]

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

[commit-bot: I haz the power, 2015-11-19 06:35:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/820001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/820001

[commit-bot: I haz the power, 2015-11-19 06:43:49]

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

[commit-bot: I haz the power, 2015-11-19 06:43:50]

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

[jochen (gone - plz use gerrit), 2015-11-19 08:59:25]

lgtm

[reveman, 2015-11-19 15:09:52]

jochen@, sorry for not noticing this before asking for your review. I have a
large set of follow up patches lined up after this that will add more files to
componenets/exo and to avoid having you stamp each one I added "per-file
exo.gypi=reveman@chromium.org" to components/OWNERS in latest version of this
cl. let me know if you have any concerns with that.

[reveman, 2015-11-19 15:10:07]

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

[commit-bot: I haz the power, 2015-11-19 15:11:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/840001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/840001

[jochen (gone - plz use gerrit), 2015-11-19 15:12:55]

On 2015/11/19 at 15:11:13, commit-bot wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1412093006/840001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1412093006/840001

Still lgtm

[commit-bot: I haz the power, 2015-11-19 15:20:06]

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

[commit-bot: I haz the power, 2015-11-19 15:20:07]

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

[sky, 2015-11-19 17:55:35]

https://codereview.chromium.org/1412093006/diff/840001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_extra_parts_exo.h (right):

https://codereview.chromium.org/1412093006/diff/840001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_extra_parts_exo.h:14: #if
defined(ENABLE_WAYLAND_SERVER)
Isn't this code only compiled if ENABLE_WAYLAND_SERVER is set? In other words,
why do you need the ifdefs?

[reveman, 2015-11-19 18:42:03]

https://codereview.chromium.org/1412093006/diff/840001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_extra_parts_exo.h (right):

https://codereview.chromium.org/1412093006/diff/840001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_extra_parts_exo.h:14: #if
defined(ENABLE_WAYLAND_SERVER)
On 2015/11/19 at 17:55:35, sky wrote:
> Isn't this code only compiled if ENABLE_WAYLAND_SERVER is set? In other words,
why do you need the ifdefs?

No longer needed. Removed them from latest patch. We'll just need to add them
back later when introducing ENABLE_XYZ_SERVER...

FYI, previously I had all code in this patch compiling that could be compiled
but it seemed unnecessary to build all this non-wayland specific code if it was
currently only used when wayland is enabled. Hence, the higher level
ENABLE_WAYLAND_SERVER checks that are not really needed.

[reveman, 2015-11-19 19:07:01]

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

[commit-bot: I haz the power, 2015-11-19 19:08:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/860001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/860001

[commit-bot: I haz the power, 2015-11-19 19:21:46]

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

[commit-bot: I haz the power, 2015-11-19 19:21:47]

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

[sky, 2015-11-19 20:27:56]

LGTM

[reveman, 2015-11-19 20:29:19]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-11-19 20:29:19]

The patchset sent to the CQ was uploaded after l-g-t-m from lpique@chromium.org,
piman@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1412093006/#ps860001
(title: "remove some ifdefs")

[commit-bot: I haz the power, 2015-11-19 20:31:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412093006/860001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412093006/860001

[commit-bot: I haz the power, 2015-11-19 22:16:55]

Committed patchset #44 (id:860001)

[commit-bot: I haz the power, 2015-11-19 22:17:42]

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