Support ozone=1 compositor_unittests with OSMesa

Support ozone=1 compositor_unittestsr OSMesa

The compositor_unittests has some tests that expect a real
framebuffer. ozone does not yet have a real framebuffer
implementation that can run in the test infrastructure.

This CL adds a simulated framebuffer sufficient to build and
run the compositor_unittests.

BUG=25242
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209254

[rjkroege, 2013-06-26 22:25:51]

ptal.

[jonathan.backer, 2013-06-27 14:07:56]

https://codereview.chromium.org/17932004/diff/2001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/17932004/diff/2001/ui/gl/gl.gyp#newcode83
ui/gl/gl.gyp:83: 'gl_implementation_linux.h',
Where are these files? I don't see them upstream and I don't see them in this
CL.

[rjkroege, 2013-06-27 14:32:00]

On 2013/06/27 14:07:56, jonathan.backer wrote:
> https://codereview.chromium.org/17932004/diff/2001/ui/gl/gl.gyp
> File ui/gl/gl.gyp (right):
> 
> https://codereview.chromium.org/17932004/diff/2001/ui/gl/gl.gyp#newcode83
> ui/gl/gl.gyp:83: 'gl_implementation_linux.h',
> Where are these files? I don't see them upstream and I don't see them in this
> CL.


doh. Fixed. PTAL.

[jonathan.backer, 2013-06-27 17:13:28]

Just nits.

https://codereview.chromium.org/17932004/diff/7001/ui/compositor/test/test_co...
File ui/compositor/test/test_compositor_host_ozone.cc (right):

https://codereview.chromium.org/17932004/diff/7001/ui/compositor/test/test_co...
ui/compositor/test/test_compositor_host_ozone.cc:57: // available.
Maybe file a crbug and reference it from the TODO?

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

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_context_ozone.cc#...
ui/gl/gl_context_ozone.cc:23: GpuPreference gpu_preference) {
Could you use a switch here? That way we'll validate that GetGLImplementation is
returning something sane.

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_context_ozone.cc#...
ui/gl/gl_context_ozone.cc:38: }
I'm curious why we have this check here. It seems like something that should be
performed in the context initialization (if at all).

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc#ne...
ui/gl/gl_surface_egl.cc:585: scoped_refptr<GLSurface> surface(new
GLSurfaceOSMesaHeadless(window));
Can we define this class in an anonymous namespace in this file? There's no need
for the rest of the world to know about it. This is what we do for
NativeViewGLSurfaceOSMesa.

[rjkroege, 2013-06-28 00:25:01]

https://codereview.chromium.org/17932004/diff/7001/ui/compositor/test/test_co...
File ui/compositor/test/test_compositor_host_ozone.cc (right):

https://codereview.chromium.org/17932004/diff/7001/ui/compositor/test/test_co...
ui/compositor/test/test_compositor_host_ozone.cc:57: // available.
On 2013/06/27 17:13:28, jonathan.backer wrote:
> Maybe file a crbug and reference it from the TODO?

Done.

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

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_context_ozone.cc#...
ui/gl/gl_context_ozone.cc:23: GpuPreference gpu_preference) {
On 2013/06/27 17:13:28, jonathan.backer wrote:
> Could you use a switch here? That way we'll validate that GetGLImplementation
is
> returning something sane.

I assume that you mean structure it with the case statement scheme. Done.

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_context_ozone.cc#...
ui/gl/gl_context_ozone.cc:38: }
On 2013/06/27 17:13:28, jonathan.backer wrote:
> I'm curious why we have this check here. It seems like something that should
be
> performed in the context initialization (if at all).

It was unnecessary code. I've removed.

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc#ne...
ui/gl/gl_surface_egl.cc:585: scoped_refptr<GLSurface> surface(new
GLSurfaceOSMesaHeadless(window));
On 2013/06/27 17:13:28, jonathan.backer wrote:
> Can we define this class in an anonymous namespace in this file? There's no
need
> for the rest of the world to know about it. This is what we do for
> NativeViewGLSurfaceOSMesa.

Yes. But there is a difficulty around this. gl_surface_osmesa.cc needs a #define
from osmesa.h. But osmesa.h cannot be compiled in if it follows gl_bindings.h as
the EGL include files destructively alter the necessary pre-reqs. So: I cannot
put GLSurfaceOSMesaHeadless in gl_surface_egl.cc without either:

*  hard-coding a OSMesa constant in gl_surface_egl.cc
*  egregious include order violation

or I can leave it like this. Thoughts?

[jonathan.backer, 2013-06-28 13:13:46]

LGTM

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc#ne...
ui/gl/gl_surface_egl.cc:585: scoped_refptr<GLSurface> surface(new
GLSurfaceOSMesaHeadless(window));
On 2013/06/28 00:25:01, rjkroege wrote:
> On 2013/06/27 17:13:28, jonathan.backer wrote:
> > Can we define this class in an anonymous namespace in this file? There's no
> need
> > for the rest of the world to know about it. This is what we do for
> > NativeViewGLSurfaceOSMesa.
> 
> Yes. But there is a difficulty around this. gl_surface_osmesa.cc needs a
#define
> from osmesa.h. But osmesa.h cannot be compiled in if it follows gl_bindings.h
as
> the EGL include files destructively alter the necessary pre-reqs. So: I cannot
> put GLSurfaceOSMesaHeadless in gl_surface_egl.cc without either:
> 
> *  hard-coding a OSMesa constant in gl_surface_egl.cc
> *  egregious include order violation
> 
> or I can leave it like this. Thoughts?
> 
Thanks for the explanation. I have a slight preference for an include order
violation (it'll be in a .cc file) because I don't like exposing more than
necessary in a .h. We've had similar ordering violations with X includes and
we've just lumped it. If you choose to violate include order, just add a comment
why so no one bothers to try to clean it up.

That's just my dogma. Feel free to choose your own.

[rjkroege, 2013-06-28 18:25:36]

On 2013/06/28 13:13:46, jonathan.backer wrote:
> LGTM
> 
> https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/17932004/diff/7001/ui/gl/gl_surface_egl.cc#ne...
> ui/gl/gl_surface_egl.cc:585: scoped_refptr<GLSurface> surface(new
> GLSurfaceOSMesaHeadless(window));
> On 2013/06/28 00:25:01, rjkroege wrote:
> > On 2013/06/27 17:13:28, jonathan.backer wrote:
> > > Can we define this class in an anonymous namespace in this file? There's
no
> > need
> > > for the rest of the world to know about it. This is what we do for
> > > NativeViewGLSurfaceOSMesa.
> > 
> > Yes. But there is a difficulty around this. gl_surface_osmesa.cc needs a
> #define
> > from osmesa.h. But osmesa.h cannot be compiled in if it follows
gl_bindings.h
> as
> > the EGL include files destructively alter the necessary pre-reqs. So: I
cannot
> > put GLSurfaceOSMesaHeadless in gl_surface_egl.cc without either:
> > 
> > *  hard-coding a OSMesa constant in gl_surface_egl.cc
> > *  egregious include order violation
> > 
> > or I can leave it like this. Thoughts?
> > 
> Thanks for the explanation. I have a slight preference for an include order
> violation (it'll be in a .cc file) because I don't like exposing more than
> necessary in a .h. We've had similar ordering violations with X includes and
> we've just lumped it. If you choose to violate include order, just add a
comment
> why so no one bothers to try to clean it up.
> 
> That's just my dogma. Feel free to choose your own.

I changed it to do the include order violation.

[commit-bot: I haz the power, 2013-06-28 18:29:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/17932004/25001

[commit-bot: I haz the power, 2013-06-29 00:26:50]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-29 00:28:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/17932004/25001

[commit-bot: I haz the power, 2013-06-29 01:13:28]

Change committed as 209254