This change adds the apple software renderer as an option on Chrome/Mac. It's enabled by passing...

This change adds the apple software renderer as an option on Chrome/Mac.  It's enabled by passing --use-gl=apple on the command line.  It also removes the default pbuffer creation on Mac since it's not required to create GL contexts and is incompatible with the generic float renderer.

BUG=106584
TEST=--use-gl=apple on chrome/mac cmdline
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113436

[apatrick_chromium, 2011-12-05 22:34:31]

Try jobs? LGTM if they pass. Sorry for the delay.

[Stephen White, 2011-12-06 18:57:01]

On 2011/12/05 22:34:31, apatrick_chromium wrote:
> Try jobs? LGTM if they pass. Sorry for the delay.

win/mac/linux passed.  Some unrelated failures on layout test bots.  Now all
trybot results cleared because I reupladed w/lint errors fixed.  :(

[Ken Russell (switch to Gerrit), 2011-12-06 19:23:47]

LGTM assuming it's been tested. Couple of questions/comments.

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc
File ui/gfx/gl/gl_context_cgl.cc (right):

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:49: attribs.push_back((CGLPixelFormatAttribute)
kCGLRendererGenericFloatID);
What are the interactions with (a) the dual-GPU code and (b) the IOSurface
support in the browser? In what environments is this new code path expected to
work (DRT, browser/UI tests, full Chrome)?

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_surface_cgl.cc
File ui/gfx/gl/gl_surface_cgl.cc (right):

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:71: PbufferGLSurfaceCGL::PbufferGLSurfaceCGL(const
gfx::Size& size)
Can we just delete the PbufferGLSurfaceCGL at this point? Perhaps create a
NullGLSurfaceCGL or similar that's compatible with GLContextCGL? Fine to do this
as a subsequent cleanup but would be nice to do.

[Ken Russell (switch to Gerrit), 2011-12-06 19:24:27]

BTW, should file a bug and point this at it. Also indicate tests in CL
description.

[Nico, 2011-12-06 19:44:33]

If apatrick and kbr are happy, I'm happy too.

[Stephen White, 2011-12-06 19:49:04]

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc
File ui/gfx/gl/gl_context_cgl.cc (right):

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:49: attribs.push_back((CGLPixelFormatAttribute)
kCGLRendererGenericFloatID);
On 2011/12/06 19:23:47, kbr wrote:
> What are the interactions with (a) the dual-GPU code and (b) the IOSurface
> support in the browser?

Since this is strictly a software renderer, it shouldn't be affected by the
dual-gpu code, so perhaps it should also skip the kCGLPFAAllowOfflineRenderers
flag above it in that case (not sure).  Is there a test I can run to mock a
dual-GPU mac?  

Regardless, since this new code only executes if --use-gl=apple is passed, I
don't think it will affect existing usage.

> In what environments is this new code path expected to
> work (DRT, browser/UI tests, full Chrome)?

It works in DRT and full Chrome, but it's not the default on any of those,
obviously (any more than --use-gl=desktop is on Windows, for example).  Don't
know about browser/ui tests.

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_surface_cgl.cc
File ui/gfx/gl/gl_surface_cgl.cc (right):

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:71: PbufferGLSurfaceCGL::PbufferGLSurfaceCGL(const
gfx::Size& size)
On 2011/12/06 19:23:47, kbr wrote:
> Can we just delete the PbufferGLSurfaceCGL at this point? Perhaps create a
> NullGLSurfaceCGL or similar that's compatible with GLContextCGL? Fine to do
this
> as a subsequent cleanup but would be nice to do.

Good idea.

[Ken Russell (switch to Gerrit), 2011-12-06 22:49:45]

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc
File ui/gfx/gl/gl_context_cgl.cc (right):

http://codereview.chromium.org/8698008/diff/8001/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:49: attribs.push_back((CGLPixelFormatAttribute)
kCGLRendererGenericFloatID);
On 2011/12/06 19:49:04, Stephen White wrote:
> On 2011/12/06 19:23:47, kbr wrote:
> > What are the interactions with (a) the dual-GPU code and (b) the IOSurface
> > support in the browser?
> 
> Since this is strictly a software renderer, it shouldn't be affected by the
> dual-gpu code, so perhaps it should also skip the kCGLPFAAllowOfflineRenderers
> flag above it in that case (not sure).  Is there a test I can run to mock a
> dual-GPU mac?  

Sorry, there isn't currently. I don't know how to mock out system level APIs
like these. Presumably we could do this by reimplementing the CGL layer and
using e.g. Mesa as the backend renderer. If you have suggestions on how we can
test this behavior in isolation I would appreciate them. In the interim we can
test on dual-GPU machines running 10.7.x if you can provide the test scenarios.

> Regardless, since this new code only executes if --use-gl=apple is passed, I
> don't think it will affect existing usage.

Sounds reasonable. Was mainly wondering if the test setup would be affected.

> > In what environments is this new code path expected to
> > work (DRT, browser/UI tests, full Chrome)?
> 
> It works in DRT and full Chrome, but it's not the default on any of those,
> obviously (any more than --use-gl=desktop is on Windows, for example).  Don't
> know about browser/ui tests.

Interesting. Good to know that it works with the other IOSurface infrastructure.

[commit-bot: I haz the power, 2011-12-07 14:17:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/8698008/8001