Implement GLX for Ozone X11.

Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137, 640613
Committed: https://crrev.com/47699347f0d4decc0d1ef082a2597e399c57f636
Cr-Commit-Position: refs/heads/master@{#418575}

[kylechar, 2016-02-23 19:16:02]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. It will attempt to
create GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Small changes are
necessary to split the GLSurface implementation due to event handling.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. It will attempt to
create GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Small changes are
necessary to split the GLSurface implementation due to event handling.

Needs changes to ANGLE before it can be compiled.

BUG=361137
==========

[kylechar, 2016-02-23 20:05:58]

Patchset #2 (id:20001) has been deleted

[kylechar, 2016-03-17 18:04:16]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. It will attempt to
create GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Small changes are
necessary to split the GLSurface implementation due to event handling.

Needs changes to ANGLE before it can be compiled.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. It will attempt to
create GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Small changes are
necessary to split the GLSurface implementation due to event handling
for Expose events.

BUG=361137
==========

[kylechar, 2016-03-23 13:46:34]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. It will attempt to
create GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Small changes are
necessary to split the GLSurface implementation due to event handling
for Expose events.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

A bit of refactoring is done with regards to ui/gl/BUILD.gn so that GLX
source files are include if ozone_platform_x11 is true. I also introduce
USE_GLX and USE_EGL definitions to make necessary ifdefs cleaner. The
GYP is modified in a similar fashion.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

[kylechar, 2016-03-23 15:40:07]

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

[kylechar, 2016-03-23 15:40:09]

Working GLX for Ozone X11. I left two comments on things to possibly change.

https://codereview.chromium.org/1723303002/diff/160001/ui/gl/gl_implementatio...
File ui/gl/gl_implementation_ozone.cc (right):

https://codereview.chromium.org/1723303002/diff/160001/ui/gl/gl_implementatio...
ui/gl/gl_implementation_ozone.cc:46: // TODO: Discuss/change this before
submitting CL.
This isn't great. GetAllowedGLImplementations is called before
InitializeStaticGLBindings where OzonePlatform::InitializeForUI() was called
before. So we need to initialize it earlier. The call to
GetAllowedGLImplementation is basically the first thing in the generic
GLSurface::InitializeOneOff(), which comes before the platform specific
GLSurface::InitializeOneOffImplementation() call.

https://codereview.chromium.org/1723303002/diff/160001/ui/gl/gl_surface_x11.cc
File ui/gl/gl_surface_x11.cc (right):

https://codereview.chromium.org/1723303002/diff/160001/ui/gl/gl_surface_x11.c...
ui/gl/gl_surface_x11.cc:250: class GL_EXPORT NativeViewGLSurfaceGLXX11 : public
NativeViewGLSurfaceGLX,
NativeViewGLSurfaceGLXX11 is a bit.. terrible?

[halliwell, 2016-04-18 15:32:16]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

A bit of refactoring is done with regards to ui/gl/BUILD.gn so that GLX
source files are include if ozone_platform_x11 is true. I also introduce
USE_GLX and USE_EGL definitions to make necessary ifdefs cleaner. The
GYP is modified in a similar fashion.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

A bit of refactoring is done with regards to ui/gl/BUILD.gn so that GLX
source files are include if ozone_platform_x11 is true. I also introduce
USE_GLX and USE_EGL definitions to make necessary ifdefs cleaner. The
GYP is modified in a similar fashion.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

[sadrul, 2016-04-19 14:49:49]

sadrul@chromium.org changed reviewers:
+ rjkroege@chromium.org

[sadrul, 2016-04-19 14:49:50]

+rjkroege@

I haven't worked with ui/gl/ much, so I wouldn't be a good reviewer for those
changes. So I will leave that up to spang@/rjkroege@ and/or a ui/gl owner. The
rest looks reasonable; left a few comments.

https://codereview.chromium.org/1723303002/diff/180001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/180001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(OS_LINUX) && defined(USE_OZONE)
Do you need defined(OS_LINUX) here?

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

https://codereview.chromium.org/1723303002/diff/180001/ui/gl/gl_surface_ozone...
ui/gl/gl_surface_ozone.cc:666: public ui::XEventDispatcher {
Can this not use the PlatformEventDispatcher interface instead?

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
ui/ozone/platform/x11/ozone_platform_x11.cc:104: if (!event_source_)
The null-check here (and above) implies we may be calling InitializeUI() and
InitializeGPU() on the same OzonePlatform instance. Is that expected?

[kylechar, 2016-04-19 16:00:20]

Thanks sadrul. Updated based on your comments.

There are also some comments I left on patch 8 to see if anyone has better ideas
for things.

https://codereview.chromium.org/1723303002/diff/180001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/180001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(OS_LINUX) && defined(USE_OZONE)
On 2016/04/19 14:49:49, sadrul wrote:
> Do you need defined(OS_LINUX) here?

Umm. I'm not actually sure. I don't think think it's necessary but did it like
that because the one above is similar (#elif defined(OS_LINUX) &&
defined(USE_X11)). I'm wondering if there is a reason for the defined(OS_LINUX)
there?

I've removed it from here. I don't think that will be an issue.

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

https://codereview.chromium.org/1723303002/diff/180001/ui/gl/gl_surface_ozone...
ui/gl/gl_surface_ozone.cc:666: public ui::XEventDispatcher {
On 2016/04/19 14:49:49, sadrul wrote:
> Can this not use the PlatformEventDispatcher interface instead?

No, Expose events are platform specific and we don't have any way to encode them
as a ui::Event under Ozone.

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
ui/ozone/platform/x11/ozone_platform_x11.cc:104: if (!event_source_)
On 2016/04/19 14:49:49, sadrul wrote:
> The null-check here (and above) implies we may be calling InitializeUI() and
> InitializeGPU() on the same OzonePlatform instance. Is that expected?

Yep. In MUS they are both in the same process.

[sadrul, 2016-04-19 16:13:37]

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
ui/ozone/platform/x11/ozone_platform_x11.cc:104: if (!event_source_)
On 2016/04/19 16:00:20, kylechar wrote:
> On 2016/04/19 14:49:49, sadrul wrote:
> > The null-check here (and above) implies we may be calling InitializeUI() and
> > InitializeGPU() on the same OzonePlatform instance. Is that expected?
> 
> Yep. In MUS they are both in the same process.

Oh, that's interesting.

PlatformEventSource (and related things) are not thread-safe. So if both the UI
and GPU threads in mus are trying to do things (e.g. add/remove observers), then
that's going to pose a problem. Can we send the expose events to the GPU side
via some other (more explicit, mus/ozone-specific) mechanism?

[kylechar, 2016-04-19 16:45:59]

On 2016/04/19 16:13:37, sadrul wrote:
>
https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
> File ui/ozone/platform/x11/ozone_platform_x11.cc (right):
> 
>
https://codereview.chromium.org/1723303002/diff/180001/ui/ozone/platform/x11/...
> ui/ozone/platform/x11/ozone_platform_x11.cc:104: if (!event_source_)
> On 2016/04/19 16:00:20, kylechar wrote:
> > On 2016/04/19 14:49:49, sadrul wrote:
> > > The null-check here (and above) implies we may be calling InitializeUI()
and
> > > InitializeGPU() on the same OzonePlatform instance. Is that expected?
> > 
> > Yep. In MUS they are both in the same process.
> 
> Oh, that's interesting.
> 
> PlatformEventSource (and related things) are not thread-safe. So if both the
UI
> and GPU threads in mus are trying to do things (e.g. add/remove observers),
then
> that's going to pose a problem. Can we send the expose events to the GPU side
> via some other (more explicit, mus/ozone-specific) mechanism?

Ah good catch. Although I think that might not be a problem here since the only
event related things in the GPU process already use an Ozone X11 specific
mechanism. The GPU process uses the XEventDispatcher list instead of
PlatformEventDispatcher list, so we can just make that list thread safe.

[kylechar, 2016-04-25 16:47:38]

This CL should be a bit simpler now. I've moved the //ui/gl refactor parts out
into a separate CL.

[kylechar, 2016-04-27 15:21:13]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

A bit of refactoring is done with regards to ui/gl/BUILD.gn so that GLX
source files are include if ozone_platform_x11 is true. I also introduce
USE_GLX and USE_EGL definitions to make necessary ifdefs cleaner. The
GYP is modified in a similar fashion.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

[kylechar, 2016-04-27 15:27:16]

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

[kylechar, 2016-04-27 15:27:16]

Adding piman for gpu/* and ui/gl/*.

[rjkroege, 2016-04-27 19:08:13]

I'd prefer a different approach. I'm not sure what that should be. Maybe we can
discuss.

https://codereview.chromium.org/1723303002/diff/200001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/200001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
Can you use base::SysInfo::IsRunningOnChromeOS() to force OZONE on CrOS back to
the previous kind of message loop?

https://codereview.chromium.org/1723303002/diff/200001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1723303002/diff/200001/ui/gl/BUILD.gn#newcode9
ui/gl/BUILD.gn:9: use_egl = is_win || is_android || is_linux
So: what do ozone configurations that are !ozone_platform_x11  use? Can this be
made explicit?

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_context_ozone.cc
File ui/gl/gl_context_ozone.cc (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_context_ozone...
ui/gl/gl_context_ozone.cc:32: #if defined(USE_GLX)
The #ifdef and configuration is unfortunate as it breaks the promise of the
ozone platform abstraction. In an ideal world, adding or removing an ozone
platform (e.g. ozone x11) shouldn't require recompiling any code from ui/gl. 

One of our goals is to eventually make Linux Chrome Ozone run identically on
desktop and chrome books with the only difference being which ozone platforms
have been compiled. So, is it possible to restructure this so that the context
creation is pushed into the platform implementation?

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_implementatio...
File ui/gl/gl_implementation_ozone.cc (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_implementatio...
ui/gl/gl_implementation_ozone.cc:46: // TODO: Discuss/change this before
submitting CL.
See my previous mutter. I don't really like this.

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_surface_glx.h
File ui/gl/gl_surface_glx.h (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_surface_glx.h...
ui/gl/gl_surface_glx.h:76: // Handle registering and unregistering for Expose
events.
This seems unfortunate. Can you handle this at a lower level? i.e. in the ozone
platform and leave this be? An ozone consumer should not know what platform
implementation sits beneath it.

https://codereview.chromium.org/1723303002/diff/220001/ui/ozone/public/surfac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/ozone/public/surfac...
ui/ozone/public/surface_factory_ozone.h:118: // Returns true if the Ozone
platform supports GLX.
I think that I'm opposed to introducing this into the top-level API. It is a
platform-specific detail that should ideally be abstracted.

[kylechar, 2016-04-27 19:36:33]

An offline discussion might be useful. I've replied with some more questions of
my own and some clarifications.

https://codereview.chromium.org/1723303002/diff/200001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/200001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/04/27 19:08:12, rjkroege wrote:
> Can you use base::SysInfo::IsRunningOnChromeOS() to force OZONE on CrOS back
to
> the previous kind of message loop?

If you look at patch 5, I was checking the ozone platform. Alternatively I could
use base::SysInfo::IsRunningOnChromeOS() too. I like both of those approaches
better.

https://codereview.chromium.org/1723303002/diff/200001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1723303002/diff/200001/ui/gl/BUILD.gn#newcode9
ui/gl/BUILD.gn:9: use_egl = is_win || is_android || is_linux
On 2016/04/27 19:08:12, rjkroege wrote:
> So: what do ozone configurations that are !ozone_platform_x11  use? Can this
be
> made explicit?

They use EGL or osemsa. What do you mean made explicit?

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_context_ozone.cc
File ui/gl/gl_context_ozone.cc (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_context_ozone...
ui/gl/gl_context_ozone.cc:32: #if defined(USE_GLX)
On 2016/04/27 19:08:12, rjkroege wrote:
> The #ifdef and configuration is unfortunate as it breaks the promise of the
> ozone platform abstraction. In an ideal world, adding or removing an ozone
> platform (e.g. ozone x11) shouldn't require recompiling any code from ui/gl. 
> 
> One of our goals is to eventually make Linux Chrome Ozone run identically on
> desktop and chrome books with the only difference being which ozone platforms
> have been compiled. So, is it possible to restructure this so that the context
> creation is pushed into the platform implementation?
> 
> 

So, it's definitely possible with one very large refactor. Right now //ui/gl has
a dep of //ui/ozone. That means ozone can't create the GLContext/GLSurface.
spang@ had the idea to split //ui/gl into //ui/gl + //ui/gl:gl_base. The
GLContext/GLSurfaces go into //ui/gl:gl_base, then //ui/ozone depends on
//ui/gl:gl_base and finally //ui/gl depends on //ui/ozone. The static methods to
create a new GLContext/GLSurface are in //ui/gl which can then delegate it to
the Ozone platform.

I started the refactor to see how feasible it will be. It's going to modify a
couple hundred files and make some big changes to how //ui/gl is structured, so
I'm weary to continue without buy in.

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_surface_glx.h
File ui/gl/gl_surface_glx.h (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/gl/gl_surface_glx.h...
ui/gl/gl_surface_glx.h:76: // Handle registering and unregistering for Expose
events.
On 2016/04/27 19:08:12, rjkroege wrote:
> This seems unfortunate. Can you handle this at a lower level? i.e. in the
ozone
> platform and leave this be? An ozone consumer should not know what platform
> implementation sits beneath it.

Not totally sure I understand this part. This abstract base class has no idea
what is happening to get Expose events, it just has some methods to handle them.
Only the subclass which is Ozone implementation specific knows.

https://codereview.chromium.org/1723303002/diff/220001/ui/ozone/public/surfac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1723303002/diff/220001/ui/ozone/public/surfac...
ui/ozone/public/surface_factory_ozone.h:118: // Returns true if the Ozone
platform supports GLX.
On 2016/04/27 19:08:12, rjkroege wrote:
> I think that I'm opposed to introducing this into the top-level API. It is a
> platform-specific detail that should ideally be abstracted.

Yes, I think that is the better approach. There is a way to do it but it
requires big refactoring, see my other comment.

[kylechar, 2016-09-06 19:11:38]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX surface and context by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

There is an added method on SurfaceFactoryOzone to check if GLX is
supported. Since the Ozone platform is picked at runtime we need a way
to check if we should use GLX.

BUG=361137
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

[kylechar, 2016-09-06 19:16:08]

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

[commit-bot: I haz the power, 2016-09-06 19:16:47]

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

[kylechar, 2016-09-06 20:41:51]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

to

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

[kylechar, 2016-09-06 20:41:52]

kylechar@chromium.org changed reviewers:
- rjkroege@chromium.org, sadrul@chromium.org

[commit-bot: I haz the power, 2016-09-06 20:41:55]

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

[commit-bot: I haz the power, 2016-09-06 20:41:56]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-09-06 20:43:48]

Alright, this is finally back at the point where adding GLX support to Ozone X11
is possible without excessive #ifdefs.

+piman for gpu/*, ui/gl/* and general review.

[kylechar, 2016-09-06 21:14:27]

Description was changed from

==========
Implement GLX with Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use a GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

to

==========
Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

[piman, 2016-09-06 22:17:55]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
Is it the case that Ozone + X11 doesn't have USE_X11 set?

Note, you will go through this path on Chrome OS (which has USE_OZONE=1), so you
are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it really what
you want? Shouldn't we have a OZONE+X11 case that is distinguished from, e.g.
OZONE+Freon (Chrome OS)?

[kylechar, 2016-09-07 13:47:16]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/09/06 22:17:55, piman wrote:
> Is it the case that Ozone + X11 doesn't have USE_X11 set?
> 
> Note, you will go through this path on Chrome OS (which has USE_OZONE=1), so
you
> are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it really what
> you want? Shouldn't we have a OZONE+X11 case that is distinguished from, e.g.
> OZONE+Freon (Chrome OS)?

Yep, Ozone X11 has USE_OZONE=1 and USE_X11=0.

It's definitely the case I don't totally understand the implications of changing
the message loop type. Does TYPE_UI use significant additional resources so it
can poll for native events? Is there some other drawback to using TYPE_UI?

The only issue with checking what Ozone platform we're running before picking
the type of message loop is that Ozone hasn't been initialized yet. I think the
Ozone initialization could be moved before here, or if that isn't possible then
I could add a different way to check what message loop type is needed for an
Ozone platform before it's initialized.

[piman, 2016-09-07 17:07:09]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/09/07 13:47:16, kylechar wrote:
> On 2016/09/06 22:17:55, piman wrote:
> > Is it the case that Ozone + X11 doesn't have USE_X11 set?
> > 
> > Note, you will go through this path on Chrome OS (which has USE_OZONE=1), so
> you
> > are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it really
what
> > you want? Shouldn't we have a OZONE+X11 case that is distinguished from,
e.g.
> > OZONE+Freon (Chrome OS)?
> 
> Yep, Ozone X11 has USE_OZONE=1 and USE_X11=0.
> 
> It's definitely the case I don't totally understand the implications of
changing
> the message loop type. Does TYPE_UI use significant additional resources so it
> can poll for native events? Is there some other drawback to using TYPE_UI?
> 
> The only issue with checking what Ozone platform we're running before picking
> the type of message loop is that Ozone hasn't been initialized yet. I think
the
> Ozone initialization could be moved before here, or if that isn't possible
then
> I could add a different way to check what message loop type is needed for an
> Ozone platform before it's initialized.

Depending on the platform, there are constraints such as:
1- there can be only 1 UI loop per process
2- it requires a connection to the UI framework (e.g. X11) which can take longer
to initialize
3- posting tasks to the loop is more costly because it needs to integrate into
the poll() loop, i.e. write on a pipe (requiring syscalls) as opposed to just
adding a task to the queue which can be done with a simple mutex/cv (futex).

1 doesn't apply here (or, it's already a constraint anyway, so nbd)
2 probably is a noop on Ozone+Freon
3 is probably the bigger deal. It's probably hard to quantify.

Surely Ozone must compile without X present (since we removed X headers on
Chrome OS Freon boards) - Is there no compile-time define that we can use to
limit this to builds that have X11?

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:198: base::MessageLoop
main_message_loop(base::MessageLoop::TYPE_DEFAULT);
If the answer to the question above is no, this is probably unreached at this
point (maybe #error)

[kylechar, 2016-09-07 21:10:42]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/09/07 17:07:09, piman wrote:
> On 2016/09/07 13:47:16, kylechar wrote:
> > On 2016/09/06 22:17:55, piman wrote:
> > > Is it the case that Ozone + X11 doesn't have USE_X11 set?
> > > 
> > > Note, you will go through this path on Chrome OS (which has USE_OZONE=1),
so
> > you
> > > are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it really
> what
> > > you want? Shouldn't we have a OZONE+X11 case that is distinguished from,
> e.g.
> > > OZONE+Freon (Chrome OS)?
> > 
> > Yep, Ozone X11 has USE_OZONE=1 and USE_X11=0.
> > 
> > It's definitely the case I don't totally understand the implications of
> changing
> > the message loop type. Does TYPE_UI use significant additional resources so
it
> > can poll for native events? Is there some other drawback to using TYPE_UI?
> > 
> > The only issue with checking what Ozone platform we're running before
picking
> > the type of message loop is that Ozone hasn't been initialized yet. I think
> the
> > Ozone initialization could be moved before here, or if that isn't possible
> then
> > I could add a different way to check what message loop type is needed for an
> > Ozone platform before it's initialized.
> 
> Depending on the platform, there are constraints such as:
> 1- there can be only 1 UI loop per process
> 2- it requires a connection to the UI framework (e.g. X11) which can take
longer
> to initialize
> 3- posting tasks to the loop is more costly because it needs to integrate into
> the poll() loop, i.e. write on a pipe (requiring syscalls) as opposed to just
> adding a task to the queue which can be done with a simple mutex/cv (futex).
> 
> 1 doesn't apply here (or, it's already a constraint anyway, so nbd)
> 2 probably is a noop on Ozone+Freon
> 3 is probably the bigger deal. It's probably hard to quantify.
> 
> Surely Ozone must compile without X present (since we removed X headers on
> Chrome OS Freon boards) - Is there no compile-time define that we can use to
> limit this to builds that have X11?

Thanks! That makes more sense about the message loop types.

Yep, Ozone compiles with X present. There is the GN args ozone_platform_x11 that
is only true when compiling Ozone X11. We could add some preprocessor define
based on that flag and check it here. Although, you can build Ozone GBM and X11
in the same binary. In that case you don't know until runtime what type of
message loop is needed.

[piman, 2016-09-07 21:44:30]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/09/07 21:10:42, kylechar wrote:
> On 2016/09/07 17:07:09, piman wrote:
> > On 2016/09/07 13:47:16, kylechar wrote:
> > > On 2016/09/06 22:17:55, piman wrote:
> > > > Is it the case that Ozone + X11 doesn't have USE_X11 set?
> > > > 
> > > > Note, you will go through this path on Chrome OS (which has
USE_OZONE=1),
> so
> > > you
> > > > are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it really
> > what
> > > > you want? Shouldn't we have a OZONE+X11 case that is distinguished from,
> > e.g.
> > > > OZONE+Freon (Chrome OS)?
> > > 
> > > Yep, Ozone X11 has USE_OZONE=1 and USE_X11=0.
> > > 
> > > It's definitely the case I don't totally understand the implications of
> > changing
> > > the message loop type. Does TYPE_UI use significant additional resources
so
> it
> > > can poll for native events? Is there some other drawback to using TYPE_UI?
> > > 
> > > The only issue with checking what Ozone platform we're running before
> picking
> > > the type of message loop is that Ozone hasn't been initialized yet. I
think
> > the
> > > Ozone initialization could be moved before here, or if that isn't possible
> > then
> > > I could add a different way to check what message loop type is needed for
an
> > > Ozone platform before it's initialized.
> > 
> > Depending on the platform, there are constraints such as:
> > 1- there can be only 1 UI loop per process
> > 2- it requires a connection to the UI framework (e.g. X11) which can take
> longer
> > to initialize
> > 3- posting tasks to the loop is more costly because it needs to integrate
into
> > the poll() loop, i.e. write on a pipe (requiring syscalls) as opposed to
just
> > adding a task to the queue which can be done with a simple mutex/cv (futex).
> > 
> > 1 doesn't apply here (or, it's already a constraint anyway, so nbd)
> > 2 probably is a noop on Ozone+Freon
> > 3 is probably the bigger deal. It's probably hard to quantify.
> > 
> > Surely Ozone must compile without X present (since we removed X headers on
> > Chrome OS Freon boards) - Is there no compile-time define that we can use to
> > limit this to builds that have X11?
> 
> Thanks! That makes more sense about the message loop types.
> 
> Yep, Ozone compiles with X present. There is the GN args ozone_platform_x11
that
> is only true when compiling Ozone X11. We could add some preprocessor define
> based on that flag and check it here. Although, you can build Ozone GBM and
X11
> in the same binary. In that case you don't know until runtime what type of
> message loop is needed.

I think the important case is that if X11 is not present (for Chrome OS), we
keep a DEFAULT message loop. So, testing the absence of X11 (since it wouldn't
be there on Chrome OS) to trigger DEFAULT, and using UI otherwise.

[kylechar, 2016-09-13 19:59:12]

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:194: #elif defined(USE_OZONE)
On 2016/09/07 21:44:30, piman OOO back 2016-09-12 wrote:
> On 2016/09/07 21:10:42, kylechar wrote:
> > On 2016/09/07 17:07:09, piman wrote:
> > > On 2016/09/07 13:47:16, kylechar wrote:
> > > > On 2016/09/06 22:17:55, piman wrote:
> > > > > Is it the case that Ozone + X11 doesn't have USE_X11 set?
> > > > > 
> > > > > Note, you will go through this path on Chrome OS (which has
> USE_OZONE=1),
> > so
> > > > you
> > > > > are changing the message loop from TYPE_DEFAULT to TYPE_UI. Is it
really
> > > what
> > > > > you want? Shouldn't we have a OZONE+X11 case that is distinguished
from,
> > > e.g.
> > > > > OZONE+Freon (Chrome OS)?
> > > > 
> > > > Yep, Ozone X11 has USE_OZONE=1 and USE_X11=0.
> > > > 
> > > > It's definitely the case I don't totally understand the implications of
> > > changing
> > > > the message loop type. Does TYPE_UI use significant additional resources
> so
> > it
> > > > can poll for native events? Is there some other drawback to using
TYPE_UI?
> > > > 
> > > > The only issue with checking what Ozone platform we're running before
> > picking
> > > > the type of message loop is that Ozone hasn't been initialized yet. I
> think
> > > the
> > > > Ozone initialization could be moved before here, or if that isn't
possible
> > > then
> > > > I could add a different way to check what message loop type is needed
for
> an
> > > > Ozone platform before it's initialized.
> > > 
> > > Depending on the platform, there are constraints such as:
> > > 1- there can be only 1 UI loop per process
> > > 2- it requires a connection to the UI framework (e.g. X11) which can take
> > longer
> > > to initialize
> > > 3- posting tasks to the loop is more costly because it needs to integrate
> into
> > > the poll() loop, i.e. write on a pipe (requiring syscalls) as opposed to
> just
> > > adding a task to the queue which can be done with a simple mutex/cv
(futex).
> > > 
> > > 1 doesn't apply here (or, it's already a constraint anyway, so nbd)
> > > 2 probably is a noop on Ozone+Freon
> > > 3 is probably the bigger deal. It's probably hard to quantify.
> > > 
> > > Surely Ozone must compile without X present (since we removed X headers on
> > > Chrome OS Freon boards) - Is there no compile-time define that we can use
to
> > > limit this to builds that have X11?
> > 
> > Thanks! That makes more sense about the message loop types.
> > 
> > Yep, Ozone compiles with X present. There is the GN args ozone_platform_x11
> that
> > is only true when compiling Ozone X11. We could add some preprocessor define
> > based on that flag and check it here. Although, you can build Ozone GBM and
> X11
> > in the same binary. In that case you don't know until runtime what type of
> > message loop is needed.
> 
> I think the important case is that if X11 is not present (for Chrome OS), we
> keep a DEFAULT message loop. So, testing the absence of X11 (since it wouldn't
> be there on Chrome OS) to trigger DEFAULT, and using UI otherwise.

That's fair. Added a #define in content/gpu/BUILD.gn and I'm checking it here.

https://codereview.chromium.org/1723303002/diff/260001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:198: base::MessageLoop
main_message_loop(base::MessageLoop::TYPE_DEFAULT);
On 2016/09/07 17:07:09, piman OOO back 2016-09-12 wrote:
> If the answer to the question above is no, this is probably unreached at this
> point (maybe #error)

Done.

[piman, 2016-09-13 22:42:32]

lgtm

[kylechar, 2016-09-14 13:34:15]

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

[kylechar, 2016-09-14 13:34:17]

+sadrul ui/events/*

[kylechar, 2016-09-14 14:35:17]

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

[commit-bot: I haz the power, 2016-09-14 14:35:39]

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

[sadrul, 2016-09-14 14:59:22]

lgtm

[commit-bot: I haz the power, 2016-09-14 15:48:59]

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

[commit-bot: I haz the power, 2016-09-14 15:49:00]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-09-14 15:57:34]

The CQ bit was checked by kylechar@chromium.org

[commit-bot: I haz the power, 2016-09-14 15:57:51]

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-09-14 16:04:13]

Description was changed from

==========
Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

to

==========
Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

[commit-bot: I haz the power, 2016-09-14 16:04:16]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-09-14 16:06:39]

Description was changed from

==========
Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613
==========

to

==========
Implement GLX for Ozone X11.

This CL adds GLX support to the Ozone X11 platform. Ozone X11 will
attempt to use GLX by default.

It reuses the existing GLX GLSurface and GLContext. Changes to
NativeViewSurfaceGLX are necessary as it handles Expose events from the
PlatformEventSource. The class is split into an abstract base class with
two small event handling implementations for USE_X11 and USE_OZONE.

BUG=361137,640613

Committed: https://crrev.com/47699347f0d4decc0d1ef082a2597e399c57f636
Cr-Commit-Position: refs/heads/master@{#418575}
==========

[commit-bot: I haz the power, 2016-09-14 16:06:40]

Patchset 14 (id:??) landed as
https://crrev.com/47699347f0d4decc0d1ef082a2597e399c57f636
Cr-Commit-Position: refs/heads/master@{#418575}