Loads CastEglPlatform implementation from shared library.

Loads CastEglPlatform implementation from shared library.

Cast Ozone is now fully implemented in chromecast/ozone, including
the Ozone factory function. The CastEglPlatform interface is moved
to public and its implementation will be loaded from shared library.

BUG=

Committed: https://crrev.com/3d0e49f9b24f3496aee8a10262fb87434dcd67a8
Cr-Commit-Position: refs/heads/master@{#323916}

[halliwell, 2015-04-02 22:39:07]

halliwell@chromium.org changed reviewers:
+ byungchul@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[halliwell, 2015-04-02 22:39:54]

Internal review coming also with the vendor side of this.

[gunsch, 2015-04-02 22:44:37]

https://codereview.chromium.org/1059073002/diff/1/chromecast/ozone/DEPS
File chromecast/ozone/DEPS (right):

https://codereview.chromium.org/1059073002/diff/1/chromecast/ozone/DEPS#newcode3
chromecast/ozone/DEPS:3: "+media/ozone",
You'll need a media/ozone/ OWNER to review this CL.

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
chromecast/public/cast_egl_platform.h:22: typedef
ui::SurfaceFactoryOzone::SetGLGetProcAddressProcCallback
A thought: these typedefs seem to be the only dependency on SurfaceFactoryOzone.
Could/should we re-define them here explicitly to avoid any Chromium
dependencies from the shlib?

[halliwell, 2015-04-02 22:50:07]

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
chromecast/public/cast_egl_platform.h:22: typedef
ui::SurfaceFactoryOzone::SetGLGetProcAddressProcCallback
On 2015/04/02 22:44:36, gunsch wrote:
> A thought: these typedefs seem to be the only dependency on
SurfaceFactoryOzone.
> Could/should we re-define them here explicitly to avoid any Chromium
> dependencies from the shlib?

Yes, I think that would be good - although note it won't quite eliminate all
Chromium dependencies, it will replace this with dependencies on base::Callback
and base::NativeLibrary (which these these typedefs use).

[halliwell, 2015-04-02 22:51:15]

halliwell@chromium.org changed reviewers:
+ spang@chromium.org

[halliwell, 2015-04-02 22:51:46]

On 2015/04/02 22:44:37, gunsch wrote:
> You'll need a media/ozone/ OWNER to review this CL.
> 
+spang for media/ozone

[gunsch, 2015-04-02 22:56:47]

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
chromecast/public/cast_egl_platform.h:22: typedef
ui::SurfaceFactoryOzone::SetGLGetProcAddressProcCallback
On 2015/04/02 22:50:07, halliwell wrote:
> On 2015/04/02 22:44:36, gunsch wrote:
> > A thought: these typedefs seem to be the only dependency on
> SurfaceFactoryOzone.
> > Could/should we re-define them here explicitly to avoid any Chromium
> > dependencies from the shlib?
> 
> Yes, I think that would be good - although note it won't quite eliminate all
> Chromium dependencies, it will replace this with dependencies on
base::Callback
> and base::NativeLibrary (which these these typedefs use).

I was thinking more along the lines of potentially adapting the base::Callback
to a function-pointer. base::NativeLibrary is just a void* for OS_POSIX.

[gunsch, 2015-04-02 22:57:43]

overall, however, the chromecast/ build- side and structural changes in this CL
are lgtm

[halliwell, 2015-04-02 23:00:09]

On 2015/04/02 22:56:47, gunsch wrote:
>
https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
> File chromecast/public/cast_egl_platform.h (right):
> 
>
https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
> chromecast/public/cast_egl_platform.h:22: typedef
> ui::SurfaceFactoryOzone::SetGLGetProcAddressProcCallback
> On 2015/04/02 22:50:07, halliwell wrote:
> > On 2015/04/02 22:44:36, gunsch wrote:
> > > A thought: these typedefs seem to be the only dependency on
> > SurfaceFactoryOzone.
> > > Could/should we re-define them here explicitly to avoid any Chromium
> > > dependencies from the shlib?
> > 
> > Yes, I think that would be good - although note it won't quite eliminate all
> > Chromium dependencies, it will replace this with dependencies on
> base::Callback
> > and base::NativeLibrary (which these these typedefs use).
> 
> I was thinking more along the lines of potentially adapting the base::Callback
> to a function-pointer. base::NativeLibrary is just a void* for OS_POSIX.

Yep ok. That would actually be great, I'll take a look.

[halliwell, 2015-04-03 00:24:27]

On 2015/04/02 23:00:09, halliwell wrote:
> On 2015/04/02 22:56:47, gunsch wrote:
> >
>
https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
> > File chromecast/public/cast_egl_platform.h (right):
> > 
> >
>
https://codereview.chromium.org/1059073002/diff/1/chromecast/public/cast_egl_...
> > chromecast/public/cast_egl_platform.h:22: typedef
> > ui::SurfaceFactoryOzone::SetGLGetProcAddressProcCallback
> > On 2015/04/02 22:50:07, halliwell wrote:
> > > On 2015/04/02 22:44:36, gunsch wrote:
> > > > A thought: these typedefs seem to be the only dependency on
> > > SurfaceFactoryOzone.
> > > > Could/should we re-define them here explicitly to avoid any Chromium
> > > > dependencies from the shlib?
> > > 
> > > Yes, I think that would be good - although note it won't quite eliminate
all
> > > Chromium dependencies, it will replace this with dependencies on
> > base::Callback
> > > and base::NativeLibrary (which these these typedefs use).
> > 
> > I was thinking more along the lines of potentially adapting the
base::Callback
> > to a function-pointer. base::NativeLibrary is just a void* for OS_POSIX.
> 
> Yep ok. That would actually be great, I'll take a look.

Patch set 2 removes all Chromium types from the interface.  Actually no need for
callbacks either ... made the API more direct.

[gunsch, 2015-04-03 00:39:43]

lgtm

[spang, 2015-04-03 00:48:22]

I've been meaning to ask, is there a reason to put this in chromecast/ozone/
instead of ui/ozone/platform/chromecast/ ?

It will be less complicated to build if you put it in ui/ozone (you won't have
to use external_ozone_platforms*).

[halliwell, 2015-04-03 00:57:58]

On 2015/04/03 00:48:22, spang wrote:
> I've been meaning to ask, is there a reason to put this in chromecast/ozone/
> instead of ui/ozone/platform/chromecast/ ?
> 
> It will be less complicated to build if you put it in ui/ozone (you won't have
> to use external_ozone_platforms*).

Actually, I didn't consider that option.  Although, note that
* CastEglPlatform _must_ be in chromecast/public (that's where we place
interfaces for hardware vendors to implement)
* The cast ozone platform target depends on our cast_vendor_gyp variable (and it
won't link without that being set up)

So there could be complications in putting it under ui/.  I don't know if people
feel those are reasonable things to overcome, or introduce too many of their own
problems?

Building with external_ozone_platforms isn't a big deal, I think - we've always
built our Ozone platform that way, since it hasn't been upstreamed until just
now.  So we have that working.

[spang, 2015-04-03 01:09:44]

On 2015/04/03 00:57:58, halliwell wrote:
> On 2015/04/03 00:48:22, spang wrote:
> > I've been meaning to ask, is there a reason to put this in chromecast/ozone/
> > instead of ui/ozone/platform/chromecast/ ?
> > 
> > It will be less complicated to build if you put it in ui/ozone (you won't
have
> > to use external_ozone_platforms*).
> 
> Actually, I didn't consider that option.  Although, note that
> * CastEglPlatform _must_ be in chromecast/public (that's where we place
> interfaces for hardware vendors to implement)

If chromecast/public is for vendors then I think adding it to
ui/ozone/platform/chromecast/DEPS would work.

> * The cast ozone platform target depends on our cast_vendor_gyp variable (and
it
> won't link without that being set up)

Ah, interesting. Is that true even with the shared library approach? I was
imagining you were using a vendor-provided .so that you drop in at runtime (and
that isn't even needed at build time). The egltest platform does this.

> 
> So there could be complications in putting it under ui/.  I don't know if
people
> feel those are reasonable things to overcome, or introduce too many of their
own
> problems?
> 
> Building with external_ozone_platforms isn't a big deal, I think - we've
always
> built our Ozone platform that way, since it hasn't been upstreamed until just
> now.  So we have that working.

[halliwell, 2015-04-03 01:22:51]

On 2015/04/03 01:09:44, spang wrote:
> On 2015/04/03 00:57:58, halliwell wrote:
> > On 2015/04/03 00:48:22, spang wrote:
> > > I've been meaning to ask, is there a reason to put this in
chromecast/ozone/
> > > instead of ui/ozone/platform/chromecast/ ?
> > > 
> > > It will be less complicated to build if you put it in ui/ozone (you won't
> have
> > > to use external_ozone_platforms*).
> > 
> > Actually, I didn't consider that option.  Although, note that
> > * CastEglPlatform _must_ be in chromecast/public (that's where we place
> > interfaces for hardware vendors to implement)
> 
> If chromecast/public is for vendors then I think adding it to
> ui/ozone/platform/chromecast/DEPS would work.
> 
> > * The cast ozone platform target depends on our cast_vendor_gyp variable
(and
> it
> > won't link without that being set up)
> 
> Ah, interesting. Is that true even with the shared library approach? I was
> imagining you were using a vendor-provided .so that you drop in at runtime
(and
> that isn't even needed at build time). The egltest platform does this.
> 

We could rewrite it that way, but our current pattern for shared libs is to
declare a dependency on a shared library gyp target.  The idea would be to have
a default/stub so for our build process, and for vendors to drop in a
replacement.  And the difficulty for now is that our "default" vendor gyp isn't
upstream, so I can't depend on that and build upstream.

One option would be to leave it here until we can upstream some of our default
vendor gyp ... if that's in our plans ... and move it to ui/ then?

Open to other suggestions ... would like to hear what byungchul@ and gunsch@
think :)

[gunsch, 2015-04-03 01:31:56]

Couple thoughts:

1) I think we want to get to the point where we've decoupled building the
vendor's shlib from building our tree. Right now, the shlib is built as part of
every "cast_shell" build due to the dependency being discussed. I think removing
that dependency makes more sense once vendors are dropping in prebuilt shlibs
into our tree, and we have a fallback implementation that can be built as part
of building "cast_shell".

2) Once we get to that point, we'll need a way to switch between "use the
vendor's checked-in prebuilt shlib" or "build and use a default Cast
implementation". Right now that is handled by where <(cast_vendor_gyp) points
to, but if we're considering vendor prebuilts then we might want to specify this
differently, e.g. a "use_prebuilt_cast_ozone_shlib" vs. building the Cast
implementation as part of the build. If you agree with this point, then we could
consider adding that *now* and having the default implementation live in public
chromium next to OzonePlatformCast. We could still ensure the internal vendor
shlib gets built other ways, such as building it explicitly from our internal
make system.

[spang, 2015-04-03 01:32:42]

On 2015/04/03 01:22:51, halliwell wrote:
> On 2015/04/03 01:09:44, spang wrote:
> > On 2015/04/03 00:57:58, halliwell wrote:
> > > On 2015/04/03 00:48:22, spang wrote:
> > > > I've been meaning to ask, is there a reason to put this in
> chromecast/ozone/
> > > > instead of ui/ozone/platform/chromecast/ ?
> > > > 
> > > > It will be less complicated to build if you put it in ui/ozone (you
won't
> > have
> > > > to use external_ozone_platforms*).
> > > 
> > > Actually, I didn't consider that option.  Although, note that
> > > * CastEglPlatform _must_ be in chromecast/public (that's where we place
> > > interfaces for hardware vendors to implement)
> > 
> > If chromecast/public is for vendors then I think adding it to
> > ui/ozone/platform/chromecast/DEPS would work.
> > 
> > > * The cast ozone platform target depends on our cast_vendor_gyp variable
> (and
> > it
> > > won't link without that being set up)
> > 
> > Ah, interesting. Is that true even with the shared library approach? I was
> > imagining you were using a vendor-provided .so that you drop in at runtime
> (and
> > that isn't even needed at build time). The egltest platform does this.
> > 
> 
> We could rewrite it that way, but our current pattern for shared libs is to
> declare a dependency on a shared library gyp target.  The idea would be to
have
> a default/stub so for our build process, and for vendors to drop in a
> replacement.  And the difficulty for now is that our "default" vendor gyp
isn't
> upstream, so I can't depend on that and build upstream.
> 
> One option would be to leave it here until we can upstream some of our default
> vendor gyp ... if that's in our plans ... and move it to ui/ then?
> 
> Open to other suggestions ... would like to hear what byungchul@ and gunsch@
> think :)

I don't think having code in the repository that cannot be built upstream is
something that the chromium project will be very happy about, in general. So I
would strongly recommend you consider finding a way to separate the vendor bits
enough to allow this.

OpenGL, Chrome OS, Android, Open GL etc all solve this issue in the same way, by
defining an interface and loading the implementation at runtime.

As far as depending on media/ozone/, though, lgtm.

[halliwell, 2015-04-03 01:40:25]

On 2015/04/03 01:32:42, spang wrote:
> On 2015/04/03 01:22:51, halliwell wrote:
> > On 2015/04/03 01:09:44, spang wrote:
> > > On 2015/04/03 00:57:58, halliwell wrote:
> > > > On 2015/04/03 00:48:22, spang wrote:
> > > > > I've been meaning to ask, is there a reason to put this in
> > chromecast/ozone/
> > > > > instead of ui/ozone/platform/chromecast/ ?
> > > > > 
> > > > > It will be less complicated to build if you put it in ui/ozone (you
> won't
> > > have
> > > > > to use external_ozone_platforms*).
> > > > 
> > > > Actually, I didn't consider that option.  Although, note that
> > > > * CastEglPlatform _must_ be in chromecast/public (that's where we place
> > > > interfaces for hardware vendors to implement)
> > > 
> > > If chromecast/public is for vendors then I think adding it to
> > > ui/ozone/platform/chromecast/DEPS would work.
> > > 
> > > > * The cast ozone platform target depends on our cast_vendor_gyp variable
> > (and
> > > it
> > > > won't link without that being set up)
> > > 
> > > Ah, interesting. Is that true even with the shared library approach? I was
> > > imagining you were using a vendor-provided .so that you drop in at runtime
> > (and
> > > that isn't even needed at build time). The egltest platform does this.
> > > 
> > 
> > We could rewrite it that way, but our current pattern for shared libs is to
> > declare a dependency on a shared library gyp target.  The idea would be to
> have
> > a default/stub so for our build process, and for vendors to drop in a
> > replacement.  And the difficulty for now is that our "default" vendor gyp
> isn't
> > upstream, so I can't depend on that and build upstream.
> > 
> > One option would be to leave it here until we can upstream some of our
default
> > vendor gyp ... if that's in our plans ... and move it to ui/ then?
> > 
> > Open to other suggestions ... would like to hear what byungchul@ and gunsch@
> > think :)
> 
> I don't think having code in the repository that cannot be built upstream is
> something that the chromium project will be very happy about, in general. So I
> would strongly recommend you consider finding a way to separate the vendor
bits
> enough to allow this.
> 
> OpenGL, Chrome OS, Android, Open GL etc all solve this issue in the same way,
by
> defining an interface and loading the implementation at runtime.
> 
> As far as depending on media/ozone/, though, lgtm.

Yep ok, I'm in favour of gunsch's suggestion of solving this now ... I didn't
feel a lot of confidence while writing this that all our build variations would
build :)

Let's talk tomorrow about what needs doing in our internal build setup to enable
this.  Adding a default/stub .so implementation will be trivial.  And then we
could move it to ui/ also.

[byungchul, 2015-04-03 05:56:49]

On 2015/04/03 01:40:25, halliwell wrote:
> On 2015/04/03 01:32:42, spang wrote:
> > On 2015/04/03 01:22:51, halliwell wrote:
> > > On 2015/04/03 01:09:44, spang wrote:
> > > > On 2015/04/03 00:57:58, halliwell wrote:
> > > > > On 2015/04/03 00:48:22, spang wrote:
> > > > > > I've been meaning to ask, is there a reason to put this in
> > > chromecast/ozone/
> > > > > > instead of ui/ozone/platform/chromecast/ ?
> > > > > > 
> > > > > > It will be less complicated to build if you put it in ui/ozone (you
> > won't
> > > > have
> > > > > > to use external_ozone_platforms*).
> > > > > 
> > > > > Actually, I didn't consider that option.  Although, note that
> > > > > * CastEglPlatform _must_ be in chromecast/public (that's where we
place
> > > > > interfaces for hardware vendors to implement)
> > > > 
> > > > If chromecast/public is for vendors then I think adding it to
> > > > ui/ozone/platform/chromecast/DEPS would work.
> > > > 
> > > > > * The cast ozone platform target depends on our cast_vendor_gyp
variable
> > > (and
> > > > it
> > > > > won't link without that being set up)
> > > > 
> > > > Ah, interesting. Is that true even with the shared library approach? I
was
> > > > imagining you were using a vendor-provided .so that you drop in at
runtime
> > > (and
> > > > that isn't even needed at build time). The egltest platform does this.
> > > > 
> > > 
> > > We could rewrite it that way, but our current pattern for shared libs is
to
> > > declare a dependency on a shared library gyp target.  The idea would be to
> > have
> > > a default/stub so for our build process, and for vendors to drop in a
> > > replacement.  And the difficulty for now is that our "default" vendor gyp
> > isn't
> > > upstream, so I can't depend on that and build upstream.
> > > 
> > > One option would be to leave it here until we can upstream some of our
> default
> > > vendor gyp ... if that's in our plans ... and move it to ui/ then?
> > > 
> > > Open to other suggestions ... would like to hear what byungchul@ and
gunsch@
> > > think :)
> > 
> > I don't think having code in the repository that cannot be built upstream is
> > something that the chromium project will be very happy about, in general. So
I
> > would strongly recommend you consider finding a way to separate the vendor
> bits
> > enough to allow this.
> > 
> > OpenGL, Chrome OS, Android, Open GL etc all solve this issue in the same
way,
> by
> > defining an interface and loading the implementation at runtime.
> > 
> > As far as depending on media/ozone/, though, lgtm.
> 
> Yep ok, I'm in favour of gunsch's suggestion of solving this now ... I didn't
> feel a lot of confidence while writing this that all our build variations
would
> build :)
> 
> Let's talk tomorrow about what needs doing in our internal build setup to
enable
> this.  Adding a default/stub .so implementation will be trivial.  And then we
> could move it to ui/ also.

The pattern in other shlibs is
1) gyp target can decide to use static link or dynamic link
2) if dynamic link, we have a default implementation which can cover most
products including chromecast
3) each vendor can replace it by specifying LD_LIBRARY_PATH

So, we don't need vendor prebuilt shlibs in our repository unless we are testing
with their shlibs.
For example, chromecast should have 2 shlibs in the build image: one for default
one (not depending on marvell's component) and the other for marvell's
implementation. The default one should be in /chrome/lib while marvell's one be
in /lib or so. Start cast receiver package with LD_LIBRARY_PATH setting
marvell's one higher priority.

[byungchul, 2015-04-03 05:58:17]

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
File chromecast/ozone/ozone_platform_cast.cc (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
chromecast/ozone/ozone_platform_cast.cc:109: } // namespace ui
2 whitespaces before "//"

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
chromecast/ozone/ozone_platform_cast.cc:117: } // namespace ui
ditto

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:54: // Create/destroy an
EGLNativeDisplayType.  These may be called multiple times
Creates/destroys

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:61: // Create/destroy an EGLNativeWindow. 
There will be at most one window at a
Creates/destroys

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform_shlib.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform_shlib.h:17: static CastEglPlatform*
Create();
Consider passing command line flags. Vendor may want to pass some infos via
command line flags.

[byungchul, 2015-04-03 06:10:07]

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:22: const int height_;
Remove trailing '_' because they are public members.

[byungchul, 2015-04-03 15:28:52]

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:58: virtual intptr_t
CreateDisplayType(const Size& size) = 0;
I understood that it is a subset of ui::SurfaceFactoryOzone. But, at the same
time it must be understood by vendors. Could it be more consistent? For example,
it has 4 pointer types, void*, int32*, void*(*)(const char*), and intptr_t.

[halliwell, 2015-04-04 01:40:06]

Moved our ozone platform to ui/ozone/platform/cast
Default libcast_graphics shared library in chromecast/graphics
gyp variables to disable building the default lib and substitute a different
target - using this for our Marvell build just now.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
File chromecast/ozone/ozone_platform_cast.cc (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
chromecast/ozone/ozone_platform_cast.cc:109: } // namespace ui
On 2015/04/03 05:58:17, byungchul wrote:
> 2 whitespaces before "//"

Done.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/ozone/ozone_...
chromecast/ozone/ozone_platform_cast.cc:117: } // namespace ui
On 2015/04/03 05:58:17, byungchul wrote:
> ditto

Done.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:22: const int height_;
On 2015/04/03 06:10:07, byungchul wrote:
> Remove trailing '_' because they are public members.

Done.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:54: // Create/destroy an
EGLNativeDisplayType.  These may be called multiple times
On 2015/04/03 05:58:17, byungchul wrote:
> Creates/destroys

Done.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:58: virtual intptr_t
CreateDisplayType(const Size& size) = 0;
On 2015/04/03 15:28:52, byungchul wrote:
> I understood that it is a subset of ui::SurfaceFactoryOzone. But, at the same
> time it must be understood by vendors. Could it be more consistent? For
example,
> it has 4 pointer types, void*, int32*, void*(*)(const char*), and intptr_t.

I switched intptr_t usage to void*, with typedefs to show intent.  And replaced
int32 with int.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:61: // Create/destroy an EGLNativeWindow. 
There will be at most one window at a
On 2015/04/03 05:58:17, byungchul wrote:
> Creates/destroys

Done.

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform_shlib.h (right):

https://codereview.chromium.org/1059073002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_egl_platform_shlib.h:17: static CastEglPlatform*
Create();
On 2015/04/03 05:58:17, byungchul wrote:
> Consider passing command line flags. Vendor may want to pass some infos via
> command line flags.

Done.

[byungchul, 2015-04-04 02:54:38]

lgtm

only nits but you should get lgtm again from others.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:16: // Avoiding Chromium types in shared
library interfaces, so no gfx::Size.
This comment might not be useful for vendors.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:51: typedef void*
(*GLGetProcAddressProc)(const char* name);
You should move typedefs before dtor.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:58: typedef void* NativeDisplayType;
ditto

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:64: typedef void* NativeWindowType;
ditto

[byungchul, 2015-04-04 03:02:21]

https://codereview.chromium.org/1059073002/diff/40001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/cast.gypi (right):

https://codereview.chromium.org/1059073002/diff/40001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/cast.gypi:23:
'<(cast_graphics_gyp):libcast_graphics_1.0',
looks like a wrong order.

[spang, 2015-04-04 16:43:54]

lgtm

[halliwell, 2015-04-06 14:32:42]

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_egl_platform.h (right):

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:16: // Avoiding Chromium types in shared
library interfaces, so no gfx::Size.
On 2015/04/04 02:54:38, byungchul wrote:
> This comment might not be useful for vendors.

yep, removed.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:51: typedef void*
(*GLGetProcAddressProc)(const char* name);
On 2015/04/04 02:54:38, byungchul wrote:
> You should move typedefs before dtor.

Done.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:58: typedef void* NativeDisplayType;
On 2015/04/04 02:54:37, byungchul wrote:
> ditto

Done.

https://codereview.chromium.org/1059073002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_egl_platform.h:64: typedef void* NativeWindowType;
On 2015/04/04 02:54:37, byungchul wrote:
> ditto

Done.

https://codereview.chromium.org/1059073002/diff/40001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/cast.gypi (right):

https://codereview.chromium.org/1059073002/diff/40001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/cast.gypi:23:
'<(cast_graphics_gyp):libcast_graphics_1.0',
On 2015/04/04 03:02:20, byungchul wrote:
> looks like a wrong order.

reordered.

[halliwell, 2015-04-06 14:33:05]

halliwell@chromium.org changed reviewers:
+ dpranke@chromium.org

[halliwell, 2015-04-06 14:33:29]

adding dpranke@ for changes in build/

[gunsch, 2015-04-06 16:05:24]

https://codereview.chromium.org/1059073002/diff/60001/chromecast/ozone/ozone_...
File chromecast/ozone/ozone_platform_cast.cc (left):

https://codereview.chromium.org/1059073002/diff/60001/chromecast/ozone/ozone_...
chromecast/ozone/ozone_platform_cast.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Note: this file's listed as an "M" in Rietveld rather than a "D", can you check
and make sure git thinks it was deleted fully and not just emptied?

https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/DEPS (right):

https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/DEPS:2: "+chromecast/public",
Oof. I didn't realize this would be a consequence of the code living in
ui/ozone/.

In general, "chromecast/" shouldn't be depended on from externally; it's a
top-level embedder implementation, the same way "chrome/" is.

Though this search does show that a *couple* things depend on chrome/:

https://code.google.com/p/chromium/codesearch#search/&q=%5C%2Bchrome%5Cb%20fi...

so maybe it's okay in limited circumstances, and "+chromecast/public" is one of
those. I'll think about this.

https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/OWNERS (right):

https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/OWNERS:1: gunsch@chromium.org
Luke: feel free to add yourself as an OWNER here.

[Dirk Pranke, 2015-04-06 16:13:34]

Shouldn't there be changes in GN files as well?

[gunsch, 2015-04-06 16:14:34]

@Dirk: Cast doesn't have any GN files yet (see my email last week: "Chromecast
GN migration").

[Dirk Pranke, 2015-04-06 16:20:25]

On 2015/04/06 16:14:34, gunsch wrote:
> @Dirk: Cast doesn't have any GN files yet (see my email last week: "Chromecast
> GN migration").

Ah, you're right. Are blocked on anything from that thread, or can you make
progress?

Regardless, the other changes lgtm.

[halliwell, 2015-04-06 16:35:02]

On 2015/04/06 16:05:24, gunsch wrote:
>
https://codereview.chromium.org/1059073002/diff/60001/chromecast/ozone/ozone_...
> File chromecast/ozone/ozone_platform_cast.cc (left):
> 
>
https://codereview.chromium.org/1059073002/diff/60001/chromecast/ozone/ozone_...
> chromecast/ozone/ozone_platform_cast.cc:1: // Copyright 2015 The Chromium
> Authors. All rights reserved.
> Note: this file's listed as an "M" in Rietveld rather than a "D", can you
check
> and make sure git thinks it was deleted fully and not just emptied?
> 
Definitely a delete. Maybe this Rietveld bug?
https://code.google.com/p/chromium/issues/detail?id=342566

>
https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
> File ui/ozone/platform/cast/DEPS (right):
> 
>
https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
> ui/ozone/platform/cast/DEPS:2: "+chromecast/public",
> Oof. I didn't realize this would be a consequence of the code living in
> ui/ozone/.
> 
> In general, "chromecast/" shouldn't be depended on from externally; it's a
> top-level embedder implementation, the same way "chrome/" is.
> 
> Though this search does show that a *couple* things depend on chrome/:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%5C%2Bchrome%5Cb%20fi...
> 
> so maybe it's okay in limited circumstances, and "+chromecast/public" is one
of
> those. I'll think about this.
> 
Yep, this is unfortunate (I did highlight it earlier before moving the code),
although at least it's just the public/ stuff ... which I guess isn't strictly
top-level embedder code itself ... 

>
https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
> File ui/ozone/platform/cast/OWNERS (right):
> 
>
https://codereview.chromium.org/1059073002/diff/60001/ui/ozone/platform/cast/...
> ui/ozone/platform/cast/OWNERS:1: mailto:gunsch@chromium.org
> Luke: feel free to add yourself as an OWNER here.
Done.

[gunsch, 2015-04-06 16:39:55]

@Dirk: not blocked yet, I think we'll quickly run into difficulty not having an
"is_chromecast" flag but we'll see how it plays out :) we have someone who's
going to be starting on it shortly.

I think I'm okay with the chromecast/public dependency only.

lgtm

[halliwell, 2015-04-06 16:55:59]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-04-06 16:55:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
byungchul@chromium.org, spang@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1059073002/#ps80001
(title: "Add myself to OWNERS for cast ozone platform.")

[commit-bot: I haz the power, 2015-04-06 16:56:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059073002/80001

[commit-bot: I haz the power, 2015-04-06 18:29:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-06 18:35:27]

Patchset 5 (id:??) landed as
https://crrev.com/3d0e49f9b24f3496aee8a10262fb87434dcd67a8
Cr-Commit-Position: refs/heads/master@{#323916}