Break //ui/gl/ dependency on //ui/ozone.

Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[kylechar, 2016-06-27 14:37:48]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone.
Invert the dependency so that //ui/ozone now depends on //ui/gl and
Ozone can override GL specific classes. Move GLImageOzoneNativePixmap
into //ui/ozone now.

Create a callback to register a function to get the EGL native display.
This is required for EGL initialization in //ui/gl but changes depending
on the Ozone platform. A callback is ugly but otherwise this presents a
circular dependency problem.

This is the finally CL in step 2 of the associated bug.

BUG=611142
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone.
Invert the dependency so that //ui/ozone now depends on //ui/gl and
Ozone can override GL specific classes. Move GLImageOzoneNativePixmap
into //ui/ozone now.

Create a callback to register a function to get the EGL native display.
This is required for EGL initialization in //ui/gl but changes depending
on the Ozone platform. A callback is ugly but otherwise this presents a
circular dependency problem.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[kylechar, 2016-06-27 14:41:46]

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

[kylechar, 2016-06-27 14:41:47]

Sending to spang@ for Ozone specific bits first. A few things to consider:

1. I still need to fix the tests  for pixmap stuff.
2. The callback for EGL native display is ugly but I can't think of a better way
to do it. Any objections to it or better ideas?
3. What do you think about making //ui/ozone/gl to hold the GL specific classes?
With the GLSurface base classes there might be a couple of things to go in the
new target.

[spang, 2016-06-27 20:37:22]

On 2016/06/27 14:41:47, kylechar wrote:
> Sending to spang@ for Ozone specific bits first. A few things to consider:
> 
> 1. I still need to fix the tests  for pixmap stuff.
> 2. The callback for EGL native display is ugly but I can't think of a better
way
> to do it. Any objections to it or better ideas?

This is happening because there's still initialization code in //ui/gl rather
than //ui/gl/init. Can it be moved?

> 3. What do you think about making //ui/ozone/gl to hold the GL specific
classes?
> With the GLSurface base classes there might be a couple of things to go in the
> new target.

That sounds reasonable. I don't think they should move to ui/ozone/public in
their current form.

[kylechar, 2016-06-28 18:14:13]

On 2016/06/27 20:37:22, spang wrote:
> On 2016/06/27 14:41:47, kylechar wrote:
> > Sending to spang@ for Ozone specific bits first. A few things to consider:
> > 
> > 1. I still need to fix the tests  for pixmap stuff.
> > 2. The callback for EGL native display is ugly but I can't think of a better
> way
> > to do it. Any objections to it or better ideas?
> 
> This is happening because there's still initialization code in //ui/gl rather
> than //ui/gl/init. Can it be moved?

Not easily. The EGL initialization sets up lots of state that is used by //ui/gl
(and will be used //ui/ozone) so there would be problems having it in
//ui/gl/init. I'm sure there is some way to split it into arbitrary pieces but
it seems worse than a callback (both in terms of effort and readability).

> > 3. What do you think about making //ui/ozone/gl to hold the GL specific
> classes?
> > With the GLSurface base classes there might be a couple of things to go in
the
> > new target.
> 
> That sounds reasonable. I don't think they should move to ui/ozone/public in
> their current form.

Will do.

[spang, 2016-06-28 19:38:35]

On 2016/06/28 18:14:13, kylechar wrote:
> On 2016/06/27 20:37:22, spang wrote:
> > On 2016/06/27 14:41:47, kylechar wrote:
> > > Sending to spang@ for Ozone specific bits first. A few things to consider:
> > > 
> > > 1. I still need to fix the tests  for pixmap stuff.
> > > 2. The callback for EGL native display is ugly but I can't think of a
better
> > way
> > > to do it. Any objections to it or better ideas?
> > 
> > This is happening because there's still initialization code in //ui/gl
rather
> > than //ui/gl/init. Can it be moved?
> 
> Not easily. The EGL initialization sets up lots of state that is used by
//ui/gl
> (and will be used //ui/ozone) so there would be problems having it in
> //ui/gl/init. I'm sure there is some way to split it into arbitrary pieces but
> it seems worse than a callback (both in terms of effort and readability).

This is used from GLSurfaceEGL::InitializeOneOff, right?

GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks like you
could add the platform default display as an argument to
GLSurfaceEGL::InitializeOneOff with a fairly small change.

A callback seems excessive. The value is set once at initialization - it doesn't
change.

> > > 3. What do you think about making //ui/ozone/gl to hold the GL specific
> > classes?
> > > With the GLSurface base classes there might be a couple of things to go in
> the
> > > new target.
> > 
> > That sounds reasonable. I don't think they should move to ui/ozone/public in
> > their current form.
> 
> Will do.

[kylechar, 2016-06-28 19:44:33]

On 2016/06/28 19:38:35, spang wrote:
> On 2016/06/28 18:14:13, kylechar wrote:
> > On 2016/06/27 20:37:22, spang wrote:
> > > On 2016/06/27 14:41:47, kylechar wrote:
> > > > Sending to spang@ for Ozone specific bits first. A few things to
consider:
> > > > 
> > > > 1. I still need to fix the tests  for pixmap stuff.
> > > > 2. The callback for EGL native display is ugly but I can't think of a
> better
> > > way
> > > > to do it. Any objections to it or better ideas?
> > > 
> > > This is happening because there's still initialization code in //ui/gl
> rather
> > > than //ui/gl/init. Can it be moved?
> > 
> > Not easily. The EGL initialization sets up lots of state that is used by
> //ui/gl
> > (and will be used //ui/ozone) so there would be problems having it in
> > //ui/gl/init. I'm sure there is some way to split it into arbitrary pieces
but
> > it seems worse than a callback (both in terms of effort and readability).
> 
> This is used from GLSurfaceEGL::InitializeOneOff, right?
> 
> GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks like you
> could add the platform default display as an argument to
> GLSurfaceEGL::InitializeOneOff with a fairly small change.
> 
> A callback seems excessive. The value is set once at initialization - it
doesn't
> change.

It's used

[kylechar, 2016-06-28 19:44:33]

On 2016/06/28 19:38:35, spang wrote:
> On 2016/06/28 18:14:13, kylechar wrote:
> > On 2016/06/27 20:37:22, spang wrote:
> > > On 2016/06/27 14:41:47, kylechar wrote:
> > > > Sending to spang@ for Ozone specific bits first. A few things to
consider:
> > > > 
> > > > 1. I still need to fix the tests  for pixmap stuff.
> > > > 2. The callback for EGL native display is ugly but I can't think of a
> better
> > > way
> > > > to do it. Any objections to it or better ideas?
> > > 
> > > This is happening because there's still initialization code in //ui/gl
> rather
> > > than //ui/gl/init. Can it be moved?
> > 
> > Not easily. The EGL initialization sets up lots of state that is used by
> //ui/gl
> > (and will be used //ui/ozone) so there would be problems having it in
> > //ui/gl/init. I'm sure there is some way to split it into arbitrary pieces
but
> > it seems worse than a callback (both in terms of effort and readability).
> 
> This is used from GLSurfaceEGL::InitializeOneOff, right?
> 
> GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks like you
> could add the platform default display as an argument to
> GLSurfaceEGL::InitializeOneOff with a fairly small change.
> 
> A callback seems excessive. The value is set once at initialization - it
doesn't
> change.

It's used

[kylechar, 2016-06-28 19:48:21]

On 2016/06/28 19:44:33, kylechar wrote:
> On 2016/06/28 19:38:35, spang wrote:
> > On 2016/06/28 18:14:13, kylechar wrote:
> > > On 2016/06/27 20:37:22, spang wrote:
> > > > On 2016/06/27 14:41:47, kylechar wrote:
> > > > > Sending to spang@ for Ozone specific bits first. A few things to
> consider:
> > > > > 
> > > > > 1. I still need to fix the tests  for pixmap stuff.
> > > > > 2. The callback for EGL native display is ugly but I can't think of a
> > better
> > > > way
> > > > > to do it. Any objections to it or better ideas?
> > > > 
> > > > This is happening because there's still initialization code in //ui/gl
> > rather
> > > > than //ui/gl/init. Can it be moved?
> > > 
> > > Not easily. The EGL initialization sets up lots of state that is used by
> > //ui/gl
> > > (and will be used //ui/ozone) so there would be problems having it in
> > > //ui/gl/init. I'm sure there is some way to split it into arbitrary pieces
> but
> > > it seems worse than a callback (both in terms of effort and readability).
> > 
> > This is used from GLSurfaceEGL::InitializeOneOff, right?
> > 
> > GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks like
you
> > could add the platform default display as an argument to
> > GLSurfaceEGL::InitializeOneOff with a fairly small change.
> > 
> > A callback seems excessive. The value is set once at initialization - it
> doesn't
> > change.
> 
> It's used

Whoops. It's used in GLSurfaceEGL::InitializeOneOff() which would be easy to add
an argument for. The problem is it's also use for tests where the one
initialization doesn't run.

There is a callstack that looks something like this:

<Get EGL Native Display>
GLSurfaceEGL::InitializeDisplay()
DriverEGL::GetPlatformExtensions()
DriverEGL::InitializeExtensionBindings()
EGLApiTest::InitializeAPI()

[spang, 2016-06-28 20:00:56]

On 2016/06/28 19:48:21, kylechar wrote:
> On 2016/06/28 19:44:33, kylechar wrote:
> > On 2016/06/28 19:38:35, spang wrote:
> > > On 2016/06/28 18:14:13, kylechar wrote:
> > > > On 2016/06/27 20:37:22, spang wrote:
> > > > > On 2016/06/27 14:41:47, kylechar wrote:
> > > > > > Sending to spang@ for Ozone specific bits first. A few things to
> > consider:
> > > > > > 
> > > > > > 1. I still need to fix the tests  for pixmap stuff.
> > > > > > 2. The callback for EGL native display is ugly but I can't think of
a
> > > better
> > > > > way
> > > > > > to do it. Any objections to it or better ideas?
> > > > > 
> > > > > This is happening because there's still initialization code in //ui/gl
> > > rather
> > > > > than //ui/gl/init. Can it be moved?
> > > > 
> > > > Not easily. The EGL initialization sets up lots of state that is used by
> > > //ui/gl
> > > > (and will be used //ui/ozone) so there would be problems having it in
> > > > //ui/gl/init. I'm sure there is some way to split it into arbitrary
pieces
> > but
> > > > it seems worse than a callback (both in terms of effort and
readability).
> > > 
> > > This is used from GLSurfaceEGL::InitializeOneOff, right?
> > > 
> > > GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks like
> you
> > > could add the platform default display as an argument to
> > > GLSurfaceEGL::InitializeOneOff with a fairly small change.
> > > 
> > > A callback seems excessive. The value is set once at initialization - it
> > doesn't
> > > change.
> > 
> > It's used
> 
> Whoops. It's used in GLSurfaceEGL::InitializeOneOff() which would be easy to
add
> an argument for. The problem is it's also use for tests where the one
> initialization doesn't run.
> 
> There is a callstack that looks something like this:
> 
> <Get EGL Native Display>
> GLSurfaceEGL::InitializeDisplay()
> DriverEGL::GetPlatformExtensions()
> DriverEGL::InitializeExtensionBindings()
> EGLApiTest::InitializeAPI()

Supposing we added an argument, could that test just call

  GLSurfaceEGL::InitializeDisplay(EGL_DEFAULT_DISPLAY) ?

instead of trying to use the real one?

If that doesn't work, it seems inconsistent to say that GL initialization was
moved to //ui/gl/init, and then try to initialize GL from this test without the
component it was moved to. The test should just depend on //ui/gl/init.

[kylechar, 2016-06-28 20:02:56]

On 2016/06/28 20:00:56, spang wrote:
> On 2016/06/28 19:48:21, kylechar wrote:
> > On 2016/06/28 19:44:33, kylechar wrote:
> > > On 2016/06/28 19:38:35, spang wrote:
> > > > On 2016/06/28 18:14:13, kylechar wrote:
> > > > > On 2016/06/27 20:37:22, spang wrote:
> > > > > > On 2016/06/27 14:41:47, kylechar wrote:
> > > > > > > Sending to spang@ for Ozone specific bits first. A few things to
> > > consider:
> > > > > > > 
> > > > > > > 1. I still need to fix the tests  for pixmap stuff.
> > > > > > > 2. The callback for EGL native display is ugly but I can't think
of
> a
> > > > better
> > > > > > way
> > > > > > > to do it. Any objections to it or better ideas?
> > > > > > 
> > > > > > This is happening because there's still initialization code in
//ui/gl
> > > > rather
> > > > > > than //ui/gl/init. Can it be moved?
> > > > > 
> > > > > Not easily. The EGL initialization sets up lots of state that is used
by
> > > > //ui/gl
> > > > > (and will be used //ui/ozone) so there would be problems having it in
> > > > > //ui/gl/init. I'm sure there is some way to split it into arbitrary
> pieces
> > > but
> > > > > it seems worse than a callback (both in terms of effort and
> readability).
> > > > 
> > > > This is used from GLSurfaceEGL::InitializeOneOff, right?
> > > > 
> > > > GLSurfaceEGL::InitializeOneOff is called from //ui/gl/init. It looks
like
> > you
> > > > could add the platform default display as an argument to
> > > > GLSurfaceEGL::InitializeOneOff with a fairly small change.
> > > > 
> > > > A callback seems excessive. The value is set once at initialization - it
> > > doesn't
> > > > change.
> > > 
> > > It's used
> > 
> > Whoops. It's used in GLSurfaceEGL::InitializeOneOff() which would be easy to
> add
> > an argument for. The problem is it's also use for tests where the one
> > initialization doesn't run.
> > 
> > There is a callstack that looks something like this:
> > 
> > <Get EGL Native Display>
> > GLSurfaceEGL::InitializeDisplay()
> > DriverEGL::GetPlatformExtensions()
> > DriverEGL::InitializeExtensionBindings()
> > EGLApiTest::InitializeAPI()
> 
> Supposing we added an argument, could that test just call
> 
>   GLSurfaceEGL::InitializeDisplay(EGL_DEFAULT_DISPLAY) ?
> 
> instead of trying to use the real one?
> 
> If that doesn't work, it seems inconsistent to say that GL initialization was
> moved to //ui/gl/init, and then try to initialize GL from this test without
the
> component it was moved to. The test should just depend on //ui/gl/init.

I'll give it a shot and see if tests pass with that idea (next week when I'm
back in the office).

[kylechar, 2016-07-05 14:57:16]

Patchset #2 (id:20001) has been deleted

[kylechar, 2016-07-05 18:45:54]

So using EGL_DEFAULT_DISPLAY in tests works! I've got rid of the callback
entirely which cleans things up nicely. The only outstanding issue other than a
few GYP compile problems is gl_image_ozone_native_pixmap_drm_unittest.cc and
gl_image_ozone_native_pixmap_unittest.cc.

Both those tests rely on GLImageTest. Moving the tests into ui/ozone/gl/ doesn't
quite work because of some dependency stuff (it ends up pulling //ui/ozone into
//ui/ozone:ozone_unittests a second time and triggering the assert_no_deps). I'm
tempted to leave the tests where they are, even if they're testing something in
in the wrong directory. They're all disabled anyways at the moment..

[kylechar, 2016-07-06 15:57:30]

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

[commit-bot: I haz the power, 2016-07-06 15:57:45]

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

[commit-bot: I haz the power, 2016-07-06 17:28:05]

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

[commit-bot: I haz the power, 2016-07-06 17:28:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kylechar, 2016-07-06 20:19:13]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone.
Invert the dependency so that //ui/ozone now depends on //ui/gl and
Ozone can override GL specific classes. Move GLImageOzoneNativePixmap
into //ui/ozone now.

Create a callback to register a function to get the EGL native display.
This is required for EGL initialization in //ui/gl but changes depending
on the Ozone platform. A callback is ugly but otherwise this presents a
circular dependency problem.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

There are two changes that are made to require this. First,
GetPlatformDefaultEGLNativeDisplay() is removed. This depended on
//ui/ozone as the EGL native display changed for each platform. Instead,
a new static method to set the EGL native display is added to
GLSurfaceEGL. This allows the EGL native display to be passed in instead
of asked for. Tests use EGL_DEFAULT_DISPLAY.

Second, GLImageOzoneNativePixmap is moved into a new target
//ui/ozone/gl. This is only possible now that the dependency problem has
been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[kylechar, 2016-07-06 20:28:23]

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

[kylechar, 2016-07-06 20:28:24]

spang for ui/ozone/*
piman for basically everything else

[piman, 2016-07-06 21:02:16]

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:59: static void SetNativeDisplay(EGLNativeDisplayType
native_display);
Could we instead pass the native display explicitly as a parameter to
InitializeOneOff/InitializeDisplay ?

https://codereview.chromium.org/2102443003/diff/80001/ui/ozone/gl/ozone_gl.gyp
File ui/ozone/gl/ozone_gl.gyp (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/ozone/gl/ozone_gl.gy...
ui/ozone/gl/ozone_gl.gyp:17: '../ozone.gyp:ozone_base',
nit: indent looks wrong

[kylechar, 2016-07-07 13:18:05]

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:59: static void SetNativeDisplay(EGLNativeDisplayType
native_display);
On 2016/07/06 21:02:16, piman wrote:
> Could we instead pass the native display explicitly as a parameter to
> InitializeOneOff/InitializeDisplay ?

The problem with doing that is DriverEGL::GetPlatfromExtensions() also calls
GLSurfaceEGL::InitializeDisplay(). Given where GetPlatfromExtensions() is called
from I think it's less practical to pass the EGL native display into it.

https://cs.chromium.org/chromium/src/ui/gl/gl_bindings.cc?q=gl_bindings.cc&sq...

There are a couple of solutions to make it a bit cleaner though:
1. Have DriverEGL::GetPlatfromExtensions() call  GLSurfaceEGL::GetDisplay()
instead and add the caveat that you need to explicitly initialize GLSurfaceEGL
first.
2. Add a boolean guard set true when you call GLSurfaceEGL::SetNativeDisplay()
and checked when you call GLSurfaceEGL::InitializeOneOff() or
GLSurfaceEGL::InitializeDisplay().

[piman, 2016-07-07 20:37:17]

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:59: static void SetNativeDisplay(EGLNativeDisplayType
native_display);
On 2016/07/07 13:18:05, kylechar wrote:
> On 2016/07/06 21:02:16, piman wrote:
> > Could we instead pass the native display explicitly as a parameter to
> > InitializeOneOff/InitializeDisplay ?
> 
> The problem with doing that is DriverEGL::GetPlatfromExtensions() also calls
> GLSurfaceEGL::InitializeDisplay(). Given where GetPlatfromExtensions() is
called
> from I think it's less practical to pass the EGL native display into it.
> 
>
https://cs.chromium.org/chromium/src/ui/gl/gl_bindings.cc?q=gl_bindings.cc&sq...
> 
> There are a couple of solutions to make it a bit cleaner though:
> 1. Have DriverEGL::GetPlatfromExtensions() call  GLSurfaceEGL::GetDisplay()
> instead and add the caveat that you need to explicitly initialize GLSurfaceEGL
> first.
> 2. Add a boolean guard set true when you call GLSurfaceEGL::SetNativeDisplay()
> and checked when you call GLSurfaceEGL::InitializeOneOff() or
> GLSurfaceEGL::InitializeDisplay().


So, unless I'm reading things incorrectly, it looks like with your change,
GLSurfaceEGL::InitializeDisplay is called before SetNativeDisplay.

InitializeGLOneOffImplementation calls InitializeStaticGLBindings first, then
InitializeGLOneOffPlatform.

Taking ozone as an example (it's similar on other platforms):
InitializeStaticGLBindings calls InitializeStaticEGLInternal ->
InitializeStaticGLBindingsEGL -> DriverEGL::InitializeExtensionBindings ->
DriverEGL::GetPlatformExtensions -> GLSurfaceEGL::InitializeDisplay
However, InitializeGLOneOffPlatform is the one that calls SetNativeDisplay

So that means that InitializeDisplay would be called without the native display
being set, causing issues - and this is hard to track down because of the
dependency on the static state.


My suggestion is to move DriverEGL::InitializeExtensionBindings into
GLSurfaceEGL::InitializeOneOff, after the InitializeDisplay, and then do #1 that
you suggest.

[kylechar, 2016-07-08 14:02:16]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

There are two changes that are made to require this. First,
GetPlatformDefaultEGLNativeDisplay() is removed. This depended on
//ui/ozone as the EGL native display changed for each platform. Instead,
a new static method to set the EGL native display is added to
GLSurfaceEGL. This allows the EGL native display to be passed in instead
of asked for. Tests use EGL_DEFAULT_DISPLAY.

Second, GLImageOzoneNativePixmap is moved into a new target
//ui/ozone/gl. This is only possible now that the dependency problem has
been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

There are two changes that are made to require this. First,
GetPlatformDefaultEGLNativeDisplay() is removed. This depended on
//ui/ozone as the EGL native display changed for each platform. Instead,
a new static method to set the EGL native display is added to
GLSurfaceEGL. This allows the EGL native display to be passed in instead
of asked for. Tests use EGL_DEFAULT_DISPLAY.

Second, GLImageOzoneNativePixmap is moved into a new target
//ui/ozone/gl. This is only possible now that the dependency problem has
been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[kylechar, 2016-07-08 15:50:25]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

There are two changes that are made to require this. First,
GetPlatformDefaultEGLNativeDisplay() is removed. This depended on
//ui/ozone as the EGL native display changed for each platform. Instead,
a new static method to set the EGL native display is added to
GLSurfaceEGL. This allows the EGL native display to be passed in instead
of asked for. Tests use EGL_DEFAULT_DISPLAY.

Second, GLImageOzoneNativePixmap is moved into a new target
//ui/ozone/gl. This is only possible now that the dependency problem has
been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[kylechar, 2016-07-08 15:55:06]

I've addressed the comments and also moved the EGLNativeDisplay parts into a new
CL. I'll send the new CL to you piman once I've double checked it passes CQ.

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:59: static void SetNativeDisplay(EGLNativeDisplayType
native_display);
On 2016/07/07 20:37:17, piman wrote:
> On 2016/07/07 13:18:05, kylechar wrote:
> > On 2016/07/06 21:02:16, piman wrote:
> > > Could we instead pass the native display explicitly as a parameter to
> > > InitializeOneOff/InitializeDisplay ?
> > 
> > The problem with doing that is DriverEGL::GetPlatfromExtensions() also calls
> > GLSurfaceEGL::InitializeDisplay(). Given where GetPlatfromExtensions() is
> called
> > from I think it's less practical to pass the EGL native display into it.
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/gl/gl_bindings.cc?q=gl_bindings.cc&sq...
> > 
> > There are a couple of solutions to make it a bit cleaner though:
> > 1. Have DriverEGL::GetPlatfromExtensions() call  GLSurfaceEGL::GetDisplay()
> > instead and add the caveat that you need to explicitly initialize
GLSurfaceEGL
> > first.
> > 2. Add a boolean guard set true when you call
GLSurfaceEGL::SetNativeDisplay()
> > and checked when you call GLSurfaceEGL::InitializeOneOff() or
> > GLSurfaceEGL::InitializeDisplay().
> 
> 
> So, unless I'm reading things incorrectly, it looks like with your change,
> GLSurfaceEGL::InitializeDisplay is called before SetNativeDisplay.
> 
> InitializeGLOneOffImplementation calls InitializeStaticGLBindings first, then
> InitializeGLOneOffPlatform.
> 
> Taking ozone as an example (it's similar on other platforms):
> InitializeStaticGLBindings calls InitializeStaticEGLInternal ->
> InitializeStaticGLBindingsEGL -> DriverEGL::InitializeExtensionBindings ->
> DriverEGL::GetPlatformExtensions -> GLSurfaceEGL::InitializeDisplay
> However, InitializeGLOneOffPlatform is the one that calls SetNativeDisplay
> 
> So that means that InitializeDisplay would be called without the native
display
> being set, causing issues - and this is hard to track down because of the
> dependency on the static state.
> 
> 
> My suggestion is to move DriverEGL::InitializeExtensionBindings into
> GLSurfaceEGL::InitializeOneOff, after the InitializeDisplay, and then do #1
that
> you suggest.

Yep, I totally missed that. Good catch. I've implemented your suggestion for
moving DriverEGL::InitializeExtensionBindings().

The CL has grown a bit big so I'll actually split it to make review easier.

https://codereview.chromium.org/2102443003/diff/80001/ui/ozone/gl/ozone_gl.gyp
File ui/ozone/gl/ozone_gl.gyp (right):

https://codereview.chromium.org/2102443003/diff/80001/ui/ozone/gl/ozone_gl.gy...
ui/ozone/gl/ozone_gl.gyp:17: '../ozone.gyp:ozone_base',
On 2016/07/06 21:02:16, piman wrote:
> nit: indent looks wrong

Done.

[piman, 2016-07-08 16:48:59]

lgtm

[spang, 2016-07-13 16:05:48]

lgtm

[kylechar, 2016-07-18 17:22:13]

kylechar@chromium.org changed reviewers:
+ fsamuel@chromium.org, sadrul@chromium.org, sandersd@chromium.org

[kylechar, 2016-07-18 17:22:13]

+fsamuel for services/ui/gles2/*
+sanders2 for media/*
+sadrul because I fixed the ordering in ui/ozone/DEPS and it thinks I added
+ui/gfx.

[kylechar, 2016-07-18 17:25:00]

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

[commit-bot: I haz the power, 2016-07-18 17:25:27]

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

[commit-bot: I haz the power, 2016-07-18 17:25:29]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[kylechar, 2016-07-18 17:25:56]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
==========

[kylechar, 2016-07-18 17:26:24]

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

[commit-bot: I haz the power, 2016-07-18 17:31:58]

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

[commit-bot: I haz the power, 2016-07-18 17:43:36]

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

[commit-bot: I haz the power, 2016-07-18 17:43:37]

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

[sandersd (OOO until July 31), 2016-07-18 22:42:17]

media/ lgtm

[sadrul, 2016-07-19 14:56:40]

lgtm

[kylechar, 2016-07-20 19:40:20]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[kylechar, 2016-07-20 19:42:47]

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

[commit-bot: I haz the power, 2016-07-20 19:43:19]

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

[commit-bot: I haz the power, 2016-07-20 19:45:49]

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

[commit-bot: I haz the power, 2016-07-20 19:45:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[kylechar, 2016-07-20 19:49:06]

Patchset #7 (id:140001) has been deleted

[kylechar, 2016-07-20 20:01:11]

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

[commit-bot: I haz the power, 2016-07-20 20:02:17]

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

[Fady Samuel, 2016-07-20 20:18:18]

services/ui lgtm

[commit-bot: I haz the power, 2016-07-20 20:48:31]

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

[commit-bot: I haz the power, 2016-07-20 20:48:32]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[kylechar, 2016-07-21 01:33:47]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-07-21 01:33:47]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org,
piman@chromium.org, sandersd@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2102443003/#ps160001
(title: "Rebase again.")

[commit-bot: I haz the power, 2016-07-21 01:34:08]

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

[commit-bot: I haz the power, 2016-07-21 04:22:50]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-07-21 04:24:32]

Description was changed from

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Break //ui/gl/ dependency on //ui/ozone.

This CL removes the last code in //ui/gl that depends on //ui/ozone. It
then inverts the dependency so that //ui/ozone depends on //ui/gl and
each OzonePlatform can override GL specific classes.

GLImageOzoneNativePixmap is moved into a new target //ui/ozone/gl. This
is only possible now that the dependency problem has been fixed.

There is an issue with tests for GLImageOzoneNativePixmap. They are
still located in //ui/gl:gl_unittests as they depend on
//ui/gl:test_support. Moving the tests into //ui/ozone:ozone_unittests
has additional dependency problems. A new ozone_gl_unittests target
could be added in the future to avoid this but the remaining tests are
currently disabled anyways.

This is the finally CL in step 2 of the associated bug.

BUG=611142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-07-21 04:24:34]

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