native_viewport support for ozone

native_viewport support for ozone

Switches ozone_platform_gbm to surfaceless, so that
initializing graphics doesn't require a gbm device in
GbmSurfaceFactory::GetNativeDisplay.

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/4802bc77a23b721a3cb19a82f62ccafad99e671c

[cdotstout, 2015-08-24 22:13:02]

cstout@chromium.org changed reviewers:
+ jamesr@chromium.org, spang@chromium.org

[jamesr, 2015-08-25 00:30:28]

What does the file_path / file_descriptor passing do?  It seems a bit
un-mojo-ish

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
File mojo/converters/ozone_drm_gpu/BUILD.gn (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
mojo/converters/ozone_drm_gpu/BUILD.gn:5: component("ozone_drm_gpu") {
s/component/source_set/

(type 'component' doesn't make sense for native_viewport either, but that's not
part of this patch)

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
File mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc:6: #include
"mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.h"
include order for foo.cc should be

#include "foo.h"
<blank line>
#include "other stuff foo.cc uses"

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:15: struct
DisplaySnapshot {
how many of these fields do we need today?

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24: bool
has_current_mode;
mixing up bools and other types will not pack well as the mojom serialization
preserves order.  if you put all the bools next to each other they'll pack into
a bitfield

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/ma...
File services/native_viewport/main.cc (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/ma...
services/native_viewport/main.cc:110: #if defined(USE_OZONE)
can this stuff go somewhere other than main.cc?  you could make another
source_set with the ozone specific check and leave gn checking on for all of the
includes within that target

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/display_manager.cc (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:26:
delegate_->RemoveObserver(this);
does this ensure that |delegate_| won't try to call OnDisplaysAquired (sic) /
OnDisplayConfigured on |this| ?

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:41: void
DisplayManager::OnDisplaysAquired(
nit: i think it's spelled 'acquired'

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:76: }  // namespace
ditto

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/display_manager.h (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.h:33: bool is_configuring_ =
false;
guideline for member initialization like this is to do it if *all* members can
be initialized in the declaration.  here delegate_ is initialized to something
else in the constructor, so figuring out the initial state of the members
requires reading both the declaration and the constructor which is a loss. it's
better to initialize all of the members in the constructor definition for this
case

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.h:43: }  // namespace
}  // namespace native_viewport

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/ozone_drm_gpu_impl.h (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/ozone_drm_gpu_impl.h:40:
mojo::StrongBinding<mojo::OzoneDrmGpu> binding_;
nit: put the StrongBinding last

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/ozone_drm_gpu_impl.h:42: const
mojo::OzoneDrmHostPtr& ozone_drm_host_;
const ref to a Ptr is odd. if you don't have an ownership stake you should be
able to make this "const mojo::OzoneDrmHost& ..." and use it as an object rather
than a pointer-to-object

https://codereview.chromium.org/1309273005/diff/1/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1309273005/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:254: : SurfacelessEGL(gfx::Size(1, 1),  // passing
zero size causes a failure
:(. is the failure in something we control?

[spang, 2015-08-25 00:57:58]

It's a bit strange that the DRM types have leaked out of of the platform
component in this patch. In chromium, content/ doesn't the DRM stuff directly -
it only uses the interfaces in ui/ozone/public.

One of those interfaces, GpuPlatformSupport[Host], was meant as a generic pipe
through which the display configuration messages are sent. Is it possible to
provide a generic mojo pipe instead?

Alternatively, do GL & windowing always share a process in mojo? Then the whole
messaging thing can be replaced with some PostTasks.

[jamesr, 2015-08-25 00:59:35]

On 2015/08/25 at 00:57:58, spang wrote:
> It's a bit strange that the DRM types have leaked out of of the platform
component in this patch. In chromium, content/ doesn't the DRM stuff directly -
it only uses the interfaces in ui/ozone/public.
> 
> One of those interfaces, GpuPlatformSupport[Host], was meant as a generic pipe
through which the display configuration messages are sent. Is it possible to
provide a generic mojo pipe instead?
> 
> Alternatively, do GL & windowing always share a process in mojo? Then the
whole messaging thing can be replaced with some PostTasks.

GL & windowing currently share a process in mojo.  I think it's quite possible
we'll want to change this in the future and little code depends on that sharing
right now, but it's the current state.

[spang, 2015-08-25 01:23:16]

On 2015/08/25 00:59:35, jamesr wrote:
> On 2015/08/25 at 00:57:58, spang wrote:
> > It's a bit strange that the DRM types have leaked out of of the platform
> component in this patch. In chromium, content/ doesn't the DRM stuff directly
-
> it only uses the interfaces in ui/ozone/public.
> > 
> > One of those interfaces, GpuPlatformSupport[Host], was meant as a generic
pipe
> through which the display configuration messages are sent. Is it possible to
> provide a generic mojo pipe instead?
> > 
> > Alternatively, do GL & windowing always share a process in mojo? Then the
> whole messaging thing can be replaced with some PostTasks.
> 
> GL & windowing currently share a process in mojo.  I think it's quite possible
> we'll want to change this in the future and little code depends on that
sharing
> right now, but it's the current state.

Ok, this code is already split into a browser & GPU process half in order to
support content/'s process model. It just requires some way to communicate
between the thread doing windowing & display configuration and the thread doing
GL.

This CL uses mojo IPC but in the process of doing so assumes DRM's message types
inside native_viewport, whereas in chromium the messages are encapsulated.

It's only a loss if you care about any of the other platform implementations
(mainly chromecast, but there's also third party ones such as ozone-wayland). If
you build with one of those then any typecasts to DRM classes are undefined
behavior and it will break badly.

[cdotstout, 2015-08-25 18:13:36]

On 2015/08/25 01:23:16, spang wrote:
> On 2015/08/25 00:59:35, jamesr wrote:
> > On 2015/08/25 at 00:57:58, spang wrote:
> > > It's a bit strange that the DRM types have leaked out of of the platform
> > component in this patch. In chromium, content/ doesn't the DRM stuff
directly
> -
> > it only uses the interfaces in ui/ozone/public.
> > > 
> > > One of those interfaces, GpuPlatformSupport[Host], was meant as a generic
> pipe
> > through which the display configuration messages are sent. Is it possible to
> > provide a generic mojo pipe instead?
> > > 
> > > Alternatively, do GL & windowing always share a process in mojo? Then the
> > whole messaging thing can be replaced with some PostTasks.
> > 
> > GL & windowing currently share a process in mojo.  I think it's quite
possible
> > we'll want to change this in the future and little code depends on that
> sharing
> > right now, but it's the current state.
> 
> Ok, this code is already split into a browser & GPU process half in order to
> support content/'s process model. It just requires some way to communicate
> between the thread doing windowing & display configuration and the thread
doing
> GL.
> 
> This CL uses mojo IPC but in the process of doing so assumes DRM's message
types
> inside native_viewport, whereas in chromium the messages are encapsulated.
> 
> It's only a loss if you care about any of the other platform implementations
> (mainly chromecast, but there's also third party ones such as ozone-wayland).
If
> you build with one of those then any typecasts to DRM classes are undefined
> behavior and it will break badly.

Even if it's possible to pass a raw mojo ipc mechanism to ozone (probably is but
I'm not sure), I think it's preferable to use the higher level mojo interface
language.  Because, this is one of the key features of mojo. :)

I think the issue here is encapsulation of the ozone platform code. It's likely
we'll want to support another ozone platform, even if just X11, and it'd be nice
if all or nearly all the code to accomplish that were encapsulated inside
ui/ozone/platform/ so ideally native_viewport is agnostic of the ozone platform.
 I can just add a mojo component to platform/drm and move as much of the mojo
drm implementation over there as makes sense.  It'd be setting a precedent to
have code under //ui depend on //mojo, but I don't see that's a problem.  James
wdyt?

[jamesr, 2015-08-25 18:16:12]

On 2015/08/25 at 18:13:36, cstout wrote:
> I think the issue here is encapsulation of the ozone platform code. It's
likely we'll want to support another ozone platform, even if just X11, and it'd
be nice if all or nearly all the code to accomplish that were encapsulated
inside ui/ozone/platform/ so ideally native_viewport is agnostic of the ozone
platform.  I can just add a mojo component to platform/drm and move as much of
the mojo drm implementation over there as makes sense.  It'd be setting a
precedent to have code under //ui depend on //mojo, but I don't see that's a
problem.  James wdyt?

I don't think there's a problem with parts of //ui/ depending on mojo concepts.

[spang, 2015-08-25 18:17:40]

On 2015/08/25 18:16:12, jamesr wrote:
> On 2015/08/25 at 18:13:36, cstout wrote:
> > I think the issue here is encapsulation of the ozone platform code. It's
> likely we'll want to support another ozone platform, even if just X11, and
it'd
> be nice if all or nearly all the code to accomplish that were encapsulated
> inside ui/ozone/platform/ so ideally native_viewport is agnostic of the ozone
> platform.  I can just add a mojo component to platform/drm and move as much of
> the mojo drm implementation over there as makes sense.  It'd be setting a
> precedent to have code under //ui depend on //mojo, but I don't see that's a
> problem.  James wdyt?
> 
> I don't think there's a problem with parts of //ui/ depending on mojo
concepts.

Ok. We should also consider what it would look like to merge that back to
chromium.

It would make the gardening issues easier at least.

[jamesr, 2015-08-25 18:21:26]

On 2015/08/25 at 18:17:40, spang wrote:
> On 2015/08/25 18:16:12, jamesr wrote:
> > On 2015/08/25 at 18:13:36, cstout wrote:
> > > I think the issue here is encapsulation of the ozone platform code. It's
> > likely we'll want to support another ozone platform, even if just X11, and
it'd
> > be nice if all or nearly all the code to accomplish that were encapsulated
> > inside ui/ozone/platform/ so ideally native_viewport is agnostic of the
ozone
> > platform.  I can just add a mojo component to platform/drm and move as much
of
> > the mojo drm implementation over there as makes sense.  It'd be setting a
> > precedent to have code under //ui depend on //mojo, but I don't see that's a
> > problem.  James wdyt?
> > 
> > I don't think there's a problem with parts of //ui/ depending on mojo
concepts.
> 
> Ok. We should also consider what it would look like to merge that back to
chromium.

We won't.

> 
> It would make the gardening issues easier at least.

[cdotstout, 2015-08-25 18:26:22]

On 2015/08/25 18:21:26, jamesr wrote:
> On 2015/08/25 at 18:17:40, spang wrote:
> > On 2015/08/25 18:16:12, jamesr wrote:
> > > On 2015/08/25 at 18:13:36, cstout wrote:
> > > > I think the issue here is encapsulation of the ozone platform code. It's
> > > likely we'll want to support another ozone platform, even if just X11, and
> it'd
> > > be nice if all or nearly all the code to accomplish that were encapsulated
> > > inside ui/ozone/platform/ so ideally native_viewport is agnostic of the
> ozone
> > > platform.  I can just add a mojo component to platform/drm and move as
much
> of
> > > the mojo drm implementation over there as makes sense.  It'd be setting a
> > > precedent to have code under //ui depend on //mojo, but I don't see that's
a
> > > problem.  James wdyt?
> > > 
> > > I don't think there's a problem with parts of //ui/ depending on mojo
> concepts.
> > 
> > Ok. We should also consider what it would look like to merge that back to
> chromium.
> 
> We won't.
> 
> > 
> > It would make the gardening issues easier at least.

Chromium may not want to take the mojo stuff, but it'd be good if chromium
incorporated the refactoring to allow different ipc implementations.

[jamesr, 2015-08-25 18:30:09]

On 2015/08/25 at 18:26:22, cstout wrote:
> On 2015/08/25 18:21:26, jamesr wrote:
> > On 2015/08/25 at 18:17:40, spang wrote:
> > > On 2015/08/25 18:16:12, jamesr wrote:
> > > > On 2015/08/25 at 18:13:36, cstout wrote:
> > > > > I think the issue here is encapsulation of the ozone platform code.
It's
> > > > likely we'll want to support another ozone platform, even if just X11,
and
> > it'd
> > > > be nice if all or nearly all the code to accomplish that were
encapsulated
> > > > inside ui/ozone/platform/ so ideally native_viewport is agnostic of the
> > ozone
> > > > platform.  I can just add a mojo component to platform/drm and move as
much
> > of
> > > > the mojo drm implementation over there as makes sense.  It'd be setting
a
> > > > precedent to have code under //ui depend on //mojo, but I don't see
that's a
> > > > problem.  James wdyt?
> > > > 
> > > > I don't think there's a problem with parts of //ui/ depending on mojo
> > concepts.
> > > 
> > > Ok. We should also consider what it would look like to merge that back to
> > chromium.
> > 
> > We won't.
> > 
> > > 
> > > It would make the gardening issues easier at least.
> 
> Chromium may not want to take the mojo stuff, but it'd be good if chromium
incorporated the refactoring to allow different ipc implementations.

That's entirely up to Chromium.  In this repo we are building things that make
sense for this project, not any other.  If stuff ends up being useful in
Chromium that's fine but it is explicitly not a goal.

[cdotstout, 2015-08-25 18:41:42]

will attempt to move mojo ipc under ui/ozone in the next patch

[cdotstout, 2015-08-27 19:10:22]

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
File mojo/converters/ozone_drm_gpu/BUILD.gn (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
mojo/converters/ozone_drm_gpu/BUILD.gn:5: component("ozone_drm_gpu") {
On 2015/08/25 00:30:28, jamesr wrote:
> s/component/source_set/
> 
> (type 'component' doesn't make sense for native_viewport either, but that's
not
> part of this patch)

Done.

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
File mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/converters/ozone_drm_g...
mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc:6: #include
"mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.h"
On 2015/08/25 00:30:28, jamesr wrote:
> include order for foo.cc should be
> 
> #include "foo.h"
> <blank line>
> #include "other stuff foo.cc uses"

Done.

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/08/25 00:30:28, jamesr wrote:
> nit: 2015

Done.

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:15: struct
DisplaySnapshot {
On 2015/08/25 00:30:28, jamesr wrote:
> how many of these fields do we need today?

Removed unused fields.

https://codereview.chromium.org/1309273005/diff/1/mojo/services/ozone_drm_gpu...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24: bool
has_current_mode;
On 2015/08/25 00:30:28, jamesr wrote:
> mixing up bools and other types will not pack well as the mojom serialization
> preserves order.  if you put all the bools next to each other they'll pack
into
> a bitfield

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/ma...
File services/native_viewport/main.cc (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/ma...
services/native_viewport/main.cc:110: #if defined(USE_OZONE)
On 2015/08/25 00:30:28, jamesr wrote:
> can this stuff go somewhere other than main.cc?  you could make another
> source_set with the ozone specific check and leave gn checking on for all of
the
> includes within that target

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/display_manager.cc (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:26:
delegate_->RemoveObserver(this);
On 2015/08/25 00:30:28, jamesr wrote:
> does this ensure that |delegate_| won't try to call OnDisplaysAquired (sic) /
> OnDisplayConfigured on |this| ?

It ensures that OnConfigurationChanged won't be called.

OnDisplaysAcquired and OnDisplayConfigured are called via callback.  Switched to
using weak ptrs.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:41: void
DisplayManager::OnDisplaysAquired(
On 2015/08/25 00:30:28, jamesr wrote:
> nit: i think it's spelled 'acquired'

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.cc:76: }  // namespace
On 2015/08/25 00:30:28, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/display_manager.h (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.h:33: bool is_configuring_ =
false;
On 2015/08/25 00:30:28, jamesr wrote:
> guideline for member initialization like this is to do it if *all* members can
> be initialized in the declaration.  here delegate_ is initialized to something
> else in the constructor, so figuring out the initial state of the members
> requires reading both the declaration and the constructor which is a loss.
it's
> better to initialize all of the members in the constructor definition for this
> case

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/display_manager.h:43: }  // namespace
On 2015/08/25 00:30:28, jamesr wrote:
> }  // namespace native_viewport

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
File services/native_viewport/ozone/ozone_drm_gpu_impl.h (right):

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/ozone_drm_gpu_impl.h:40:
mojo::StrongBinding<mojo::OzoneDrmGpu> binding_;
On 2015/08/25 00:30:28, jamesr wrote:
> nit: put the StrongBinding last

Done.

https://codereview.chromium.org/1309273005/diff/1/services/native_viewport/oz...
services/native_viewport/ozone/ozone_drm_gpu_impl.h:42: const
mojo::OzoneDrmHostPtr& ozone_drm_host_;
On 2015/08/25 00:30:28, jamesr wrote:
> const ref to a Ptr is odd. if you don't have an ownership stake you should be
> able to make this "const mojo::OzoneDrmHost& ..." and use it as an object
rather
> than a pointer-to-object

Done.

https://codereview.chromium.org/1309273005/diff/1/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1309273005/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:254: : SurfacelessEGL(gfx::Size(1, 1),  // passing
zero size causes a failure
On 2015/08/25 00:30:28, jamesr wrote:
> :(. is the failure in something we control?

No, it's gbm/mesa.  Could move the workaround down closer to the if you prefer.

[jamesr, 2015-08-27 19:35:48]

https://codereview.chromium.org/1309273005/diff/60001/mojo/converters/ozone_d...
File mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/mojo/converters/ozone_d...
mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc:40: out.type =
static_cast<ui::DisplayConnectionType>(in->type);
is this type interpreted using the ui::DisplayConnectionType value on both ends?
if we wanted the mojom to be really general (i.e. be able to use a different
impl on the other end of the mojom interface) we should specify the possible
values in a mojo enum or with constants or something and then either
static_assert() that the values match and cast or use a switch to set values. i
don't think we need to do that in this patch necessarily but at least leave a
TODO to revisit

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:19: int32
type;
is this an enum?

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:20: bool
has_current_mode;
could we put some basic documentation here? most of these fields are pretty
self-evident, but i'm not sure what 'has_current_mode' means or what the
relationship of display_id / product_id is

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24:
DisplayMode native_mode;
are current / native always members of |modes|? maybe these could be indices
instead?

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/BUILD.gn
File ui/ozone/BUILD.gn (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/BUILD.gn#newco...
ui/ozone/BUILD.gn:91: component("ozone_common_message_types") {
s/component/source_set/

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_gpu_delegate.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_delegate.h:16:
MojoDrmGpuDelegate(mojo::OzoneDrmHost* ozone_drm_host);
explicit

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_delegate.h:19: //
DrmGpuPlatformSupportDelegate
this could be in the private: section

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_gpu_impl.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_impl.h:17:
MojoDrmGpuImpl(mojo::InterfaceRequest<mojo::OzoneDrmGpu> request);
explicit

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_impl.h:20: // OzoneDrmGpu.
these could all be in private:

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_delegate.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_delegate.h:16:
MojoDrmHostDelegate(mojo::OzoneDrmGpu* ozone_drm_gpu);
explicit, private below

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_impl.cc:20:
platform_support_->OnChannelEstablished(base::Process::Current().Pid());
does this need the actual pid? what does it use it for?

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_impl.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_impl.h:17:
MojoDrmHostImpl(mojo::InterfaceRequest<mojo::OzoneDrmHost> request);
ditto

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_ipc_init_helper.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_ipc_init_helper.cc:44: public:
is this just default-constructed? might be useful to have explicit c'tor/d'tor

[cdotstout, 2015-08-27 21:28:34]

https://codereview.chromium.org/1309273005/diff/60001/mojo/converters/ozone_d...
File mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/mojo/converters/ozone_d...
mojo/converters/ozone_drm_gpu/ozone_drm_gpu_type_converters.cc:40: out.type =
static_cast<ui::DisplayConnectionType>(in->type);
On 2015/08/27 19:35:47, jamesr wrote:
> is this type interpreted using the ui::DisplayConnectionType value on both
ends?
> if we wanted the mojom to be really general (i.e. be able to use a different
> impl on the other end of the mojom interface) we should specify the possible
> values in a mojo enum or with constants or something and then either
> static_assert() that the values match and cast or use a switch to set values.
i
> don't think we need to do that in this patch necessarily but at least leave a
> TODO to revisit

Done.

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:19: int32
type;
On 2015/08/27 19:35:47, jamesr wrote:
> is this an enum?

Done.

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:20: bool
has_current_mode;
On 2015/08/27 19:35:47, jamesr wrote:
> could we put some basic documentation here? most of these fields are pretty
> self-evident, but i'm not sure what 'has_current_mode' means or what the
> relationship of display_id / product_id is

Added some docs.  Michael please advise if I didn't get it right.

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24:
DisplayMode native_mode;
On 2015/08/27 19:35:47, jamesr wrote:
> are current / native always members of |modes|? maybe these could be indices
> instead?

I think so.  Michael, any reason not to make the change in chromium as well?

Added TODO for now.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/BUILD.gn
File ui/ozone/BUILD.gn (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/BUILD.gn#newco...
ui/ozone/BUILD.gn:91: component("ozone_common_message_types") {
On 2015/08/27 19:35:47, jamesr wrote:
> s/component/source_set/

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_gpu_delegate.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_delegate.h:16:
MojoDrmGpuDelegate(mojo::OzoneDrmHost* ozone_drm_host);
On 2015/08/27 19:35:47, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_delegate.h:19: //
DrmGpuPlatformSupportDelegate
On 2015/08/27 19:35:47, jamesr wrote:
> this could be in the private: section

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_gpu_impl.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_impl.h:17:
MojoDrmGpuImpl(mojo::InterfaceRequest<mojo::OzoneDrmGpu> request);
On 2015/08/27 19:35:47, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_gpu_impl.h:20: // OzoneDrmGpu.
On 2015/08/27 19:35:47, jamesr wrote:
> these could all be in private:

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_delegate.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_delegate.h:16:
MojoDrmHostDelegate(mojo::OzoneDrmGpu* ozone_drm_gpu);
On 2015/08/27 19:35:47, jamesr wrote:
> explicit, private below

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_impl.cc:20:
platform_support_->OnChannelEstablished(base::Process::Current().Pid());
On 2015/08/27 19:35:47, jamesr wrote:
> does this need the actual pid? what does it use it for?

Some ipc mechanisms may need it.  Drm/mojo doesn't use it.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_impl.h (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_impl.h:17:
MojoDrmHostImpl(mojo::InterfaceRequest<mojo::OzoneDrmHost> request);
On 2015/08/27 19:35:48, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_ipc_init_helper.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_ipc_init_helper.cc:44: public:
On 2015/08/27 19:35:48, jamesr wrote:
> is this just default-constructed? might be useful to have explicit c'tor/d'tor

Yes. Done.

[spang, 2015-08-28 19:38:20]

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:20: bool
has_current_mode;
On 2015/08/27 21:28:33, cdotstout wrote:
> On 2015/08/27 19:35:47, jamesr wrote:
> > could we put some basic documentation here? most of these fields are pretty
> > self-evident, but i'm not sure what 'has_current_mode' means or what the
> > relationship of display_id / product_id is
> 
> Added some docs.  Michael please advise if I didn't get it right.

display_id identifies a display attached to the system. We try to make it stable
across reboots.

product_id is a manufacturer provided id for the hardware model, not unique per
unit

https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24:
DisplayMode native_mode;
On 2015/08/27 21:28:33, cdotstout wrote:
> On 2015/08/27 19:35:47, jamesr wrote:
> > are current / native always members of |modes|? maybe these could be indices
> > instead?
> 
> I think so.  Michael, any reason not to make the change in chromium as well?
> 
> Added TODO for now.

I think so as well.

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):

https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mojo/drm_host_impl.cc:20:
platform_support_->OnChannelEstablished(base::Process::Current().Pid());
On 2015/08/27 19:35:47, jamesr wrote:
> does this need the actual pid? what does it use it for?

The number is used for handling GPU process restarts. Upon restart,
OnChannelDestroyed(old_host_id) and OnChannelEstablished(new_host_id) are both
called, but content/ does not guarantee the ordering of the calls. See also
GpuProcessHostUIShim.

Since we can't rely on order, the id resolves an ambiguity on what to do in
OnChannelDestroyed. It either does nothing (new channel already open) or cleans
up (new channel not available yet).

The only thing that matters is that each new channel uses a different number.

[cdotstout, 2015-08-28 20:37:57]

On 2015/08/28 19:38:20, spang wrote:
>
https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
> File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom
(right):
> 
>
https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
> mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:20: bool
> has_current_mode;
> On 2015/08/27 21:28:33, cdotstout wrote:
> > On 2015/08/27 19:35:47, jamesr wrote:
> > > could we put some basic documentation here? most of these fields are
pretty
> > > self-evident, but i'm not sure what 'has_current_mode' means or what the
> > > relationship of display_id / product_id is
> > 
> > Added some docs.  Michael please advise if I didn't get it right.
> 
> display_id identifies a display attached to the system. We try to make it
stable
> across reboots.
> 
> product_id is a manufacturer provided id for the hardware model, not unique
per
> unit
> 
>
https://codereview.chromium.org/1309273005/diff/60001/mojo/services/ozone_drm...
> mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:24:
> DisplayMode native_mode;
> On 2015/08/27 21:28:33, cdotstout wrote:
> > On 2015/08/27 19:35:47, jamesr wrote:
> > > are current / native always members of |modes|? maybe these could be
indices
> > > instead?
> > 
> > I think so.  Michael, any reason not to make the change in chromium as well?
> > 
> > Added TODO for now.
> 
> I think so as well.
> 
>
https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
> File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1309273005/diff/60001/ui/ozone/platform/drm/m...
> ui/ozone/platform/drm/mojo/drm_host_impl.cc:20:
> platform_support_->OnChannelEstablished(base::Process::Current().Pid());
> On 2015/08/27 19:35:47, jamesr wrote:
> > does this need the actual pid? what does it use it for?
> 
> The number is used for handling GPU process restarts. Upon restart,
> OnChannelDestroyed(old_host_id) and OnChannelEstablished(new_host_id) are both
> called, but content/ does not guarantee the ordering of the calls. See also
> GpuProcessHostUIShim.
> 
> Since we can't rely on order, the id resolves an ambiguity on what to do in
> OnChannelDestroyed. It either does nothing (new channel already open) or
cleans
> up (new channel not available yet).
> 
> The only thing that matters is that each new channel uses a different number.

I think it's probably too soon for us to think too hard about gpu process
restart in the turquoise world.

So I'd like to proceed, but let me know if you disagree or would like any
further refinement in this CL.

Thanks guys.

[jamesr, 2015-08-28 20:41:29]

I think what this patch does is fine.

[jamesr, 2015-08-28 20:49:44]

lgtm

https://codereview.chromium.org/1309273005/diff/160001/mojo/services/ozone_dr...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/160001/mojo/services/ozone_dr...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:37: // TODO:
can this be just an index into the modes array?
you should stick your name in the TODO thusly: "TODO(cstout): ..." so anyone
reading this comment knows who to contact about it

https://codereview.chromium.org/1309273005/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):

https://codereview.chromium.org/1309273005/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/mojo/drm_host_impl.cc:13: const int FAKE_PROCESS_ID = -1;
constants should be named kFakeProcessID and this should be static (for linkage
purposes) or in an anonymous namespace

[cdotstout, 2015-08-28 20:59:14]

https://codereview.chromium.org/1309273005/diff/160001/mojo/services/ozone_dr...
File mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom (right):

https://codereview.chromium.org/1309273005/diff/160001/mojo/services/ozone_dr...
mojo/services/ozone_drm_gpu/public/interfaces/ozone_drm_gpu.mojom:37: // TODO:
can this be just an index into the modes array?
On 2015/08/28 20:49:44, jamesr wrote:
> you should stick your name in the TODO thusly: "TODO(cstout): ..." so anyone
> reading this comment knows who to contact about it

Done.

https://codereview.chromium.org/1309273005/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/mojo/drm_host_impl.cc (right):

https://codereview.chromium.org/1309273005/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/mojo/drm_host_impl.cc:13: const int FAKE_PROCESS_ID = -1;
On 2015/08/28 20:49:44, jamesr wrote:
> constants should be named kFakeProcessID and this should be static (for
linkage
> purposes) or in an anonymous namespace

Done.

[cdotstout, 2015-08-28 22:00:20]

Committed patchset #11 (id:200001) manually as
4802bc77a23b721a3cb19a82f62ccafad99e671c (presubmit successful).