Support dynamic switching between integrated and discrete GPUs on Mac OS X.

Support dynamic switching between integrated and discrete GPUs on Mac OS X.

Change Chrome to allocate most OpenGL contexts with the
kCGLPFAAllowOfflineRenderers flag, and specify
NSSupportsAutomaticGraphicsSwitching in the Info.plist for the main
executable and helper apps. This keeps Chrome on the integrated GPU
except when using WebGL, accelerated 2D Canvas, Pepper 3D, and Core
Animation-based plugins (except Flash).

Chrome shares resources between OpenGL contexts in order to display WebGL
and other content in the compositor, and resource sharing doesn't work
between contexts allocated on different GPUs. Therefore, when the first
context for a given renderer requests the discrete GPU, the channel is
dropped and all contexts are reallocated on the discrete GPU. Similarly,
when the last context requesting the discrete GPU for a given renderer is
shut down, all contexts are dropped and reallocated on the integrated GPU.

Currently dynamic GPU switching is only supported on the latest Mac OS X
10.7 update and MacBook Pros with dual AMD / Intel GPUs, though this will
improve in future OS updates.

Tested with WebGL, CSS 3D, Flash and Unity3D content and observed desired
GPU switching behavior. Also added a layout test to WebKit under
https://bugs.webkit.org/show_bug.cgi?id=69776 which when run in Chrome
catches an assertion failure related to the destruction of contexts. The
intent is to add it as a UI layout test on the GPU bots.

BUG=88788
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105399

[Ken Russell (switch to Gerrit), 2011-10-11 22:55:04]

Please review => apatrick, mark, stuartmorgan

OWNERS review => ananta, darin, sky, vrk

FYI => thakis

Thanks.

[sky, 2011-10-11 23:40:25]

The other reviewers you have no more than I. I'm rubber stamping this: LGTM

[Mark Mentovai, 2011-10-12 00:48:18]

http://codereview.chromium.org/8233027/diff/7012/chrome/app/app-Info.plist
File chrome/app/app-Info.plist (right):

http://codereview.chromium.org/8233027/diff/7012/chrome/app/app-Info.plist#ne...
chrome/app/app-Info.plist:260: <key>NSSupportsAutomaticGraphicsSwitching</key>
Keep the keys in this file sorted. You’ll note that they’re already sorted.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_context_mac.cc#...
ui/gfx/gl/gl_context_mac.cc:51: // particular to detect single-GPU systems.
This is the one thing I think we can do way better on. Are you planning to
address this in a quick follow-up?

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

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:35: // Avoid switching to the discrete GPU just for
this pixel
Fix indentation.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:39: attribs.push_back((CGLPixelFormatAttribute) 0);
Use a proper C++<style>(cast).

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:54: LOG(ERROR) << "pixel_format == 0.";
This isn’t called pixel_format now (and actually wasn’t called pixel_format
before, either.)

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:57: CGLReleasePixelFormat(format);
Is there any way for format to be nonzero but num_pixel_formats to be zero? If
so, this gets leaked when you do the num_pixel_formats == 0 early return.

Even if there’s no possibility for a leak, it’s possible that someone will
insert additional logic that contains an early return and does leak format.

You can mitigate this with a scoper, but if you don’t want to do a whole scoper,
could you do something like this?

if (CGLChoosePixelFormat() != kCGLNoError) {
  return false;
}
if (format) {
  CGLReleasePixelFormat();
} else {
  return false;
}
if (num_pixel_formats == 0) {
  return false;
}

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gpu_preference.h
File ui/gfx/gl/gpu_preference.h (right):

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gpu_preference.h#n...
ui/gfx/gl/gpu_preference.h:23: }
}  // namespace

[Ken Russell (switch to Gerrit), 2011-10-12 01:16:36]

http://codereview.chromium.org/8233027/diff/7012/chrome/app/app-Info.plist
File chrome/app/app-Info.plist (right):

http://codereview.chromium.org/8233027/diff/7012/chrome/app/app-Info.plist#ne...
chrome/app/app-Info.plist:260: <key>NSSupportsAutomaticGraphicsSwitching</key>
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> Keep the keys in this file sorted. You’ll note that they’re already sorted.

Done.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_context_mac.cc#...
ui/gfx/gl/gl_context_mac.cc:51: // particular to detect single-GPU systems.
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> This is the one thing I think we can do way better on. Are you planning to
> address this in a quick follow-up?

I agree, this could be done better. Unfortunately I don't know how to reliably
detect the dual-GPU case and so can't promise a quick fix (i.e., by the M16
branch point), though I am happy to file a follow-on bug now and reference it in
the comment if you like.

Note that the main side-effect is only slightly less efficient startup and
shutdown of WebGL content on single-GPU Lion (and later) systems.

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

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:35: // Avoid switching to the discrete GPU just for
this pixel
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:39: attribs.push_back((CGLPixelFormatAttribute) 0);
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> Use a proper C++<style>(cast).

Done.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:54: LOG(ERROR) << "pixel_format == 0.";
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> This isn’t called pixel_format now (and actually wasn’t called pixel_format
> before, either.)

Fixed.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_surface_cgl.cc#...
ui/gfx/gl/gl_surface_cgl.cc:57: CGLReleasePixelFormat(format);
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> Is there any way for format to be nonzero but num_pixel_formats to be zero? If
> so, this gets leaked when you do the num_pixel_formats == 0 early return.
> 
> Even if there’s no possibility for a leak, it’s possible that someone will
> insert additional logic that contains an early return and does leak format.
> 
> You can mitigate this with a scoper, but if you don’t want to do a whole
scoper,
> could you do something like this?
> 
> if (CGLChoosePixelFormat() != kCGLNoError) {
>   return false;
> }
> if (format) {
>   CGLReleasePixelFormat();
> } else {
>   return false;
> }
> if (num_pixel_formats == 0) {
>   return false;
> }

Thanks for the suggestion. Implemented.

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gpu_preference.h
File ui/gfx/gl/gpu_preference.h (right):

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gpu_preference.h#n...
ui/gfx/gl/gpu_preference.h:23: }
On 2011/10/12 00:48:18, Mark Mentovai wrote:
> }  // namespace

Done.

[Mark Mentovai, 2011-10-12 01:32:09]

LGTM

You might be able to figure out how to detect the GPU configuration you’re
looking for by poking around in ioreg.

[stuartmorgan, 2011-10-12 07:13:24]

LGTM with nits

Also, in the CL description:
"This keeps Chrome on the integrated GPU except when using WebGL, accelerated 2D
Canvas, Pepper 3D, and most plugins (except Flash)."
can you change "most plugins" to "Core Animation-based plugins"?

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:432: --num_contexts_preferring_discrete_gpu_;
Is it worth DCHECKING here that it's not negative?

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.h (right):

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.h:121: bool ShouldPreferDiscreteGpu() const;
Method-level comment?

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

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:34: return NULL;
NULL -> false

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gpu_preference.h
File ui/gfx/gl/gpu_preference.h (right):

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gpu_preference.h#n...
ui/gfx/gl/gpu_preference.h:19: PreferIntegratedGpu,
Two spaces, not four.

http://codereview.chromium.org/8233027/diff/4021/webkit/gpu/webgraphicscontex...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

http://codereview.chromium.org/8233027/diff/4021/webkit/gpu/webgraphicscontex...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:638: //
TODO(kbr): more work will be needed in this implementation to
s/more/More/.

Annotate with a bug link?

[vrk (LEFT CHROMIUM), 2011-10-12 17:21:13]

LGTM for media!

[apatrick_chromium, 2011-10-12 19:01:28]

I have nothing to add. LGTM.

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

http://codereview.chromium.org/8233027/diff/7012/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:34: return NULL;
return false

http://codereview.chromium.org/8233027/diff/4021/content/renderer/gpu/webgrap...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/8233027/diff/4021/content/renderer/gpu/webgrap...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:88: if
(attributes.noExtensions || !attributes.canRecoverFromContextLoss)
It can't all be done in this CL but it might be reasonable to add a field to
Attributes to indicate the GPU preference.

[Ken Russell (switch to Gerrit), 2011-10-12 22:39:42]

mark: please re-review gl_context_cgl.cc. Implemented the dual-GPU detection
logic and tested it on Lion laptop with WebGL, Flash and Unity3D; working as
expected. (If it fails, we switch to the discrete GPU and stay there, so the
failure mode is what is desired.)

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:432: --num_contexts_preferring_discrete_gpu_;
On 2011/10/12 07:13:24, stuartmorgan wrote:
> Is it worth DCHECKING here that it's not negative?

Sure. Added.

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.h (right):

http://codereview.chromium.org/8233027/diff/4021/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.h:121: bool ShouldPreferDiscreteGpu() const;
On 2011/10/12 07:13:24, stuartmorgan wrote:
> Method-level comment?

Added.

http://codereview.chromium.org/8233027/diff/4021/content/renderer/gpu/webgrap...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/8233027/diff/4021/content/renderer/gpu/webgrap...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:88: if
(attributes.noExtensions || !attributes.canRecoverFromContextLoss)
On 2011/10/12 19:01:28, apatrick_chromium wrote:
> It can't all be done in this CL but it might be reasonable to add a field to
> Attributes to indicate the GPU preference.

Agreed, but yes, this requires back-and-forth commits to WebKit and Chromium and
so can't be done in one CL.

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

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gl_context_cgl.cc#...
ui/gfx/gl/gl_context_cgl.cc:34: return NULL;
On 2011/10/12 07:13:24, stuartmorgan wrote:
> NULL -> false

Oops. Fixed.

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gpu_preference.h
File ui/gfx/gl/gpu_preference.h (right):

http://codereview.chromium.org/8233027/diff/4021/ui/gfx/gl/gpu_preference.h#n...
ui/gfx/gl/gpu_preference.h:19: PreferIntegratedGpu,
On 2011/10/12 07:13:24, stuartmorgan wrote:
> Two spaces, not four.

Fixed.

http://codereview.chromium.org/8233027/diff/4021/webkit/gpu/webgraphicscontex...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

http://codereview.chromium.org/8233027/diff/4021/webkit/gpu/webgraphicscontex...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:638: //
TODO(kbr): more work will be needed in this implementation to
On 2011/10/12 07:13:24, stuartmorgan wrote:
> s/more/More/.

OK.

> Annotate with a bug link?

Not clear to me that it is worth it. I don't have evidence that the GPU
switching code path would be testable in our test harness, outside of the full
browser.

[Mark Mentovai, 2011-10-13 03:50:32]

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

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_cgl.cc...
ui/gfx/gl/gl_context_cgl.cc:57: if (!format) {
This is similar to the other maybe-I-leak-format thing I brought up in the last
review from gl_surface_cgl.cc.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:50: bool GLContext::SupportsDualGpus() {
Found it!

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:55: static bool initializing = false;
You only care about one thread, right?

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:66: return false;
At this early-return point, do you want to set initializing = false and
initialized = true to avoid trying to do the one-off failure busywork on
subsequent reentry?

It seems like you might be able to get away with a single static guarding this:

  static bool initialized = false;
  if (initialized) {
    return supports_dual_gpus;
  } else {
    initialized = true;
    if (!GLSurface::InitializeOneOff()) {
      return false;
    }
  }

I can’t convince myself that having the two statics actually helps.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:101: CGLDestroyRendererInfo(renderer_info);
You’ve been good about not leaking this, but with a couple early-return points
scattered about, a scoper might be better.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:120: found_offline = true;
Will a graphics card without a display attached in a more traditional
multiple-card situation (expecting multiple displays, as opposed to the modern
single-display card-switches-on-the-fly situation) also show up as “offline?” If
so, does that present a problem for the logic?

[Ken Russell (switch to Gerrit), 2011-10-13 20:09:32]

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

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_cgl.cc...
ui/gfx/gl/gl_context_cgl.cc:57: if (!format) {
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> This is similar to the other maybe-I-leak-format thing I brought up in the
last
> review from gl_surface_cgl.cc.

I reviewed the documentation for CGLChoosePixelFormat and it states that 0 is
returned for the number of pixel formats if NULL is returned for the pixel
format object. Based on this I changed this code and that in gl_surface_cgl.cc
to just do DCHECKs on num_pixel_formats.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:50: bool GLContext::SupportsDualGpus() {
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> Found it!

Whoops, sorry. I meant gl_context_mac.cc.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:55: static bool initializing = false;
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> You only care about one thread, right?

Yes, for the time being. gl_surface_mac.cc already doesn't have locking around
InitializeOneOff().

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:66: return false;
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> At this early-return point, do you want to set initializing = false and
> initialized = true to avoid trying to do the one-off failure busywork on
> subsequent reentry?
> 
> It seems like you might be able to get away with a single static guarding
this:
> 
>   static bool initialized = false;
>   if (initialized) {
>     return supports_dual_gpus;
>   } else {
>     initialized = true;
>     if (!GLSurface::InitializeOneOff()) {
>       return false;
>     }
>   }
> 
> I can’t convince myself that having the two statics actually helps.

Sorry, this doesn't work. I need to handle the case where
GLContext::SupportsDualGpus calls GLSurface::InitializeOneOff which calls back
to GLContext::SupportsDualGpus, and I need to complete the rest of the logic in
SupportsDualGpus the second time without calling InitializeOneOff again or an
infinite loop will occur. I reorganized the logic slightly based on your
suggestion but the two booleans are needed.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:101: CGLDestroyRendererInfo(renderer_info);
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> You’ve been good about not leaking this, but with a couple early-return points
> scattered about, a scoper might be better.

OK. I generalized ScopedCFTypeRef to take a freeing functor because
scoped_ptr_malloc doesn't work for types like CGLRendererInfoObj where the
pointer-ness is embedded in the typedef.

http://codereview.chromium.org/8233027/diff/15010/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:120: found_offline = true;
On 2011/10/13 03:50:33, Mark Mentovai wrote:
> Will a graphics card without a display attached in a more traditional
> multiple-card situation (expecting multiple displays, as opposed to the modern
> single-display card-switches-on-the-fly situation) also show up as “offline?”
If
> so, does that present a problem for the logic?

I'm not sure, but I suspect that the card in that case will show up as online
and not offline. Apple's documentation on supporting multiple GPUs indicates
that the kCGLPFAAllowOfflineRenderers flag only affects the behavior of mixed
integrated and discrete GPUs. Anyway, I am 99% sure that all of our logic will
work fine in this rare case.

[Mark Mentovai, 2011-10-13 20:20:48]

http://codereview.chromium.org/8233027/diff/22001/base/mac/scoped_cftyperef.h
File base/mac/scoped_cftyperef.h (right):

http://codereview.chromium.org/8233027/diff/22001/base/mac/scoped_cftyperef.h...
base/mac/scoped_cftyperef.h:40: // is embedded in the type name) but with
specialized releasing routines.
The ) doesn’t close any (.

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:25: CGLDestroyRendererInfo(x);
Is this a retain/release-like interface (reference counted) or a
malloc/free-like interface?

If it’s the latter, specializing ScopedCFTypeRef is wrong, because the .reset()
semantics will be incorrect in the case where you give the ScopedCFTypeRef the
same object it’s already supervising. In that case, I’d recommend writhing a
simple scoper class right here instead of specializing ScopedCFTypeRef.

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:29: }  // anonymous namespace
We usually just write }  // namespace

[Ken Russell (switch to Gerrit), 2011-10-13 21:01:27]

Rebuilt and retested.

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:25: CGLDestroyRendererInfo(x);
On 2011/10/13 20:20:48, Mark Mentovai wrote:
> Is this a retain/release-like interface (reference counted) or a
> malloc/free-like interface?
> 
> If it’s the latter, specializing ScopedCFTypeRef is wrong, because the
.reset()
> semantics will be incorrect in the case where you give the ScopedCFTypeRef the
> same object it’s already supervising. In that case, I’d recommend writhing a
> simple scoper class right here instead of specializing ScopedCFTypeRef.

It is the latter. I see. Based on your suggestion I've added a ScopedGenericObj
to base/mac/ which will handle all kinds of CGL objects, as well as hopefully
others.

http://codereview.chromium.org/8233027/diff/22001/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:29: }  // anonymous namespace
On 2011/10/13 20:20:48, Mark Mentovai wrote:
> We usually just write }  // namespace

Done.

[Mark Mentovai, 2011-10-13 21:13:00]

LGTM with the file moved and an example added to the comment

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj.h
File base/mac/scoped_generic_obj.h (right):

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj...
base/mac/scoped_generic_obj.h:13: namespace mac {
There is nothing Mac-ish about this. Can you move it to base/memory and rename
this namespace and #include guards appropriately?

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj...
base/mac/scoped_generic_obj.h:18: // takes as its second template argument a
functor which frees the object.
The comment should have a sample FreeProc, since unlike scoped_ptr_malloc<>,
there is no default free functor.

http://codereview.chromium.org/8233027/diff/29004/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/29004/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:29: }  // anonymous namespace
I thought you said you switched this to }  // namespace

[Ken Russell (switch to Gerrit), 2011-10-13 21:43:48]

For the record: the try jobs I submitted for this CL after fixing the link
errors in the last set have disappeared. I've built this manually on Linux in
both Debug and Release modes and am confident that it won't break anything, so
am landing this by hand to make the M16 milestone.

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj.h
File base/mac/scoped_generic_obj.h (right):

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj...
base/mac/scoped_generic_obj.h:13: namespace mac {
On 2011/10/13 21:13:01, Mark Mentovai wrote:
> There is nothing Mac-ish about this. Can you move it to base/memory and rename
> this namespace and #include guards appropriately?

Yes. Done.

http://codereview.chromium.org/8233027/diff/29004/base/mac/scoped_generic_obj...
base/mac/scoped_generic_obj.h:18: // takes as its second template argument a
functor which frees the object.
On 2011/10/13 21:13:01, Mark Mentovai wrote:
> The comment should have a sample FreeProc, since unlike scoped_ptr_malloc<>,
> there is no default free functor.

OK. Added.

http://codereview.chromium.org/8233027/diff/29004/ui/gfx/gl/gl_context_mac.cc
File ui/gfx/gl/gl_context_mac.cc (right):

http://codereview.chromium.org/8233027/diff/29004/ui/gfx/gl/gl_context_mac.cc...
ui/gfx/gl/gl_context_mac.cc:29: }  // anonymous namespace
On 2011/10/13 21:13:01, Mark Mentovai wrote:
> I thought you said you switched this to }  // namespace

Sorry, I must have accidentally uploaded the last CL before making that change.

[Mark Mentovai, 2011-10-13 22:06:01]

LGTM

[Ken Russell (switch to Gerrit), 2011-10-13 22:19:27]

+ben for content/ OWNERS review

[Ben Goodger (Google), 2011-10-13 22:22:05]

Assuming Mark, Stuart et al. know what they're doing LGTM for content.

[Mark Mentovai, 2011-10-13 22:23:09]

Know what I’m doing? Me? Good luck with THAT!