Fix the mac hangup when force-compositing-mode is enabled

Previously, the DisplayLink would never start until a software draw was done at least once. With force-compositing-mode, the software draw was never done, causing tabs to lockup indefinitely waiting for the swap acknowledgement. This fix ensures that the DisplayLink is running immediately.

The Cocoa setUpGState callback was required to avoid "invalid drawable" errors when setView was called on the NSOpenGLContext.

Another deadlock cause could be the GpuChannel route getting removed while the renderer is blocked on a FlushSync. For this, a method has been added to GpuCommandBufferStub to unblock/cleanup that is called from GpuChannel before the IPC route is removed.

Trace events have been added in places that will help debug related issues in the future.

BUG=84343
TEST=launch with --force-compositing-mode; open about:flags; in the same tab, open ycombinator.com and it should not hang.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88877

[jbates, 2011-06-07 16:47:52]

Just a work in progress, looking for some feedback on the bug fixes.

http://codereview.chromium.org/6993043/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:829: // TODO: Why
does this happen? Does it matter?
Looks like this may be a recent ToT bug. It does not seem to happen in the
latest m13 branch.

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

http://codereview.chromium.org/6993043/diff/2001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:269: stub->CommandBufferWasDestroyed();
If the IPC route is removed, unblock the stub so that it can reply to any
pending calls from the renderer.

http://codereview.chromium.org/6993043/diff/2001/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/6993043/diff/2001/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.cc:402:
scheduler_->SetScheduled(true);
This was a bug that would allow the scheduled_count to get out of sync.

http://codereview.chromium.org/6993043/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6993043/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:398: //num_swapbuffers_complete_pending_--;
The num_swapbuffers_complete_pending_ count was wrong on OSX - perhaps when a
tab goes invisible. In any case, allowing it to EarlyOut here seemed to be
causing RAF-based demos to stop animating. Why not just render even when pending
is zero?

[Nico, 2011-06-07 17:10:29]

It's been a while since I looked at the GPU code, so I won't do a real review.
Two things:

1.) What's %cpu when watching youtube videos with the display link and with the
new approach? When I wrote this, the display link needed half as much %cpu.

2.) It looks like this CL does several independent things (add trace statements,
remove display link, add code to unregister stubs). If these are in fact
independent, can you do them in separate cls?

[jbates, 2011-06-07 19:21:54]

On 2011/06/07 17:10:29, Nico wrote:
> It's been a while since I looked at the GPU code, so I won't do a real review.
> Two things:
> 
> 1.) What's %cpu when watching youtube videos with the display link and with
the
> new approach? When I wrote this, the display link needed half as much %cpu.
There's no significant difference. If anything, the new approach shows a drop
from 1.4% CPU usage to 1.3% by the Chrome process (steady state), with no change
in the Renderer or Flash processes.

> 
> 2.) It looks like this CL does several independent things (add trace
statements,
> remove display link, add code to unregister stubs). If these are in fact
> independent, can you do them in separate cls?
Sure, once feedback is addressed, I can split the final changes up if they are
independent.

[nduca, 2011-06-08 00:08:34]

This bugfix really concerns me. We've measured drawView as taking large amounts
of time, haven't we? By putting this on the main thread, we're blocking the UI
whenever there's heavy GPU content on the screen.

Am I missing something?

[jbates, 2011-06-08 00:24:12]

On 2011/06/08 00:08:34, nduca wrote:
> This bugfix really concerns me. We've measured drawView as taking large
amounts
> of time, haven't we? By putting this on the main thread, we're blocking the UI
> whenever there's heavy GPU content on the screen.
> 
> Am I missing something?

I've been doing tests with 5 heavy WebGL demos open in separate windows, and the
UI remains fluid. Do you have any test pages to stress the final flush?

[jbates, 2011-06-08 00:39:32]

On 2011/06/08 00:24:12, jbates wrote:
> On 2011/06/08 00:08:34, nduca wrote:
> > This bugfix really concerns me. We've measured drawView as taking large
> amounts
> > of time, haven't we? By putting this on the main thread, we're blocking the
UI
> > whenever there's heavy GPU content on the screen.
> > 
> > Am I missing something?
> 
> I've been doing tests with 5 heavy WebGL demos open in separate windows, and
the
> UI remains fluid. Do you have any test pages to stress the final flush?

I just tested using your heavylayers test. That slows down the chrome UI, but it
also slows down every other UI equally. For example, the UI of a separate
instance of chrome (release ver 12.0.742.91) appears to go exactly as slow as
the one running heavylayers. I also tested the inverse: Chrome 12 with
heavylayers. Same result.

That means that it doesn't matter what thread does the FlushDrawable, because it
bogs down the entire OS. So it seems to be a big win to do it in the UI thread
and I'm pretty sure we will see a boost in performance on OSX now.

[apatrick_chromium, 2011-06-08 01:09:29]

I don't know about the mac stuff. LGTM for the rest.

[nduca, 2011-06-08 01:10:17]

At a high level, I'm uneasy with ripping out a system that was put in for a
reason basically because its got some race condition in it. Especially when we
need to drover the fix to m13. If you've really thought this though and still
feel like this is the right way to go, I guess I won't raise too much of a
stink, because I know you've looked at more in-depth than me. I guess my
question is, "Are there any ways we can fix this race without embarking on a
last minute structural change to the drawing path?"

Your observation that the OS is bogged down is well made but doesn't sit quite
right with me. In the current displayLink architecture, I think [but I might be
wrong] that INPUT processing does not actually get bogged down. I think that the
user can still send mouse events, e.g. to the close button, even though the
display is lagged. By putting a blocking drawView call on the main thread,
whatever time it blocks is time where we can't process input events. Maybe this
doesn't matter in the end, but it is a change, and one that I think should be
cautiously approached.

For non-attack content, e.g. just a page that happens to be eating 7ms in
drawView because its heavy-ish, my more practical concern is that this CL
chewing up 7ms of the Chrome UI thread, a non-scalable resource that is
important to lots of systems, not just graphics. That is time we can't spend on
input processing, network, etc etc. I think what I'm hearing from you is that in
the test content you tried, it doesn't really matter, but are we 100% convinced
that this is free of side effects?

Why did we chose display link in the first place? Kbr might know.

Does this impact frame rate stability? Do the drawView calls still vsync
properly?

[Nico, 2011-06-08 01:22:37]

On Tue, Jun 7, 2011 at 6:10 PM,  <nduca@chromium.org> wrote:
> At a high level, I'm uneasy with ripping out a system that was put in for a
> reason basically because its got some race condition in it. Especially when
> we
> need to drover the fix to m13. If you've really thought this though and
> still
> feel like this is the right way to go, I guess I won't raise too much of a
> stink, because I know you've looked at more in-depth than me. I guess my
> question is, "Are there any ways we can fix this race without embarking on a
> last minute structural change to the drawing path?"
>
> Your observation that the OS is bogged down is well made but doesn't sit
> quite
> right with me. In the current displayLink architecture, I think [but I might
> be
> wrong] that INPUT processing does not actually get bogged down. I think that
> the
> user can still send mouse events, e.g. to the close button, even though the
> display is lagged. By putting a blocking drawView call on the main thread,
> whatever time it blocks is time where we can't process input events. Maybe
> this
> doesn't matter in the end, but it is a change, and one that I think should
> be
> cautiously approached.
>
> For non-attack content, e.g. just a page that happens to be eating 7ms in
> drawView because its heavy-ish, my more practical concern is that this CL
> chewing up 7ms of the Chrome UI thread, a non-scalable resource that is
> important to lots of systems, not just graphics. That is time we can't spend
> on
> input processing, network, etc etc. I think what I'm hearing from you is
> that in
> the test content you tried, it doesn't really matter, but are we 100%
> convinced
> that this is free of side effects?
>
> Why did we chose display link in the first place? Kbr might know.

I did this because it was recommended in apple docs and because cpu
usage was lower (0.5% vs 1.2% iirc) with the displaylink when I
originally wrote it.

See http://codereview.chromium.org/3010054 and
http://codereview.chromium.org/3176027 – without the displaylink,
things would've been slower than the old hosting technique.

>
> Does this impact frame rate stability? Do the drawView calls still vsync
> properly?
>
> http://codereview.chromium.org/6993043/
>

[Ken Russell (switch to Gerrit), 2011-06-08 01:43:06]

On 2011/06/08 01:22:37, Nico wrote:
> On Tue, Jun 7, 2011 at 6:10 PM,  <mailto:nduca@chromium.org> wrote:
> > At a high level, I'm uneasy with ripping out a system that was put in for a
> > reason basically because its got some race condition in it. Especially when
> > we
> > need to drover the fix to m13. If you've really thought this though and
> > still
> > feel like this is the right way to go, I guess I won't raise too much of a
> > stink, because I know you've looked at more in-depth than me. I guess my
> > question is, "Are there any ways we can fix this race without embarking on a
> > last minute structural change to the drawing path?"
> >
> > Your observation that the OS is bogged down is well made but doesn't sit
> > quite
> > right with me. In the current displayLink architecture, I think [but I might
> > be
> > wrong] that INPUT processing does not actually get bogged down. I think that
> > the
> > user can still send mouse events, e.g. to the close button, even though the
> > display is lagged. By putting a blocking drawView call on the main thread,
> > whatever time it blocks is time where we can't process input events. Maybe
> > this
> > doesn't matter in the end, but it is a change, and one that I think should
> > be
> > cautiously approached.
> >
> > For non-attack content, e.g. just a page that happens to be eating 7ms in
> > drawView because its heavy-ish, my more practical concern is that this CL
> > chewing up 7ms of the Chrome UI thread, a non-scalable resource that is
> > important to lots of systems, not just graphics. That is time we can't spend
> > on
> > input processing, network, etc etc. I think what I'm hearing from you is
> > that in
> > the test content you tried, it doesn't really matter, but are we 100%
> > convinced
> > that this is free of side effects?
> >
> > Why did we chose display link in the first place? Kbr might know.
> 
> I did this because it was recommended in apple docs and because cpu
> usage was lower (0.5% vs 1.2% iirc) with the displaylink when I
> originally wrote it.
> 
> See http://codereview.chromium.org/3010054 and
> http://codereview.chromium.org/3176027 – without the displaylink,
> things would've been slower than the old hosting technique.

Nico's DisplayLink work was excellent but at this point I think we should go
ahead with this change to remove it. Recently many issues with framerate
instability have come up and it seems that some of them can be traced to the
fact that CVDisplayLink internally attempts to sync to vertical refresh, and so
does the OpenGL code that we do on that thread. I think that we should back up
to a known safe state where we do all of our OpenGL work on the UI thread, and
then as follow-on work, investigate adding our own custom thread which does the
OpenGL work on demand.

> >
> > Does this impact frame rate stability? Do the drawView calls still vsync
> > properly?
> >
> > http://codereview.chromium.org/6993043/
> >

[nduca, 2011-06-08 16:10:37]

Thanks Ken & Nico. Based on that information, I feel a lot less scared by this
with your comments. LGTM.

[Nico, 2011-06-08 16:52:46]

As mentioned above, I'd prefer if this landed during calmer times, for better
bisectability. Having said that, I defer the decision for this to you, apatrick,
and nduca.

The cocoa parts LGTM modulo 2 minor comments.

(stuart: fyi, no more displaylink thread in the browser.)

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:68: NSRect frame =
[self frame];
no need for a variable

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:210: }
If you don't need the trace output, remove this method (but chances are you do)

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:214: }
Remove this method?

[jbates, 2011-06-08 17:54:42]

I'm going to leave it to the more seasoned chromium devs to decide whether its
OK to vsync wait in the UI thread. From my limited understanding, it seems that
the worst case is that handling of input events is delayed by up to 16ms. Since
the DisplayLink thread added up to 16ms of display latency, there should be no
net change to latency from this CL.

Although I prefer the background thread design, if we were to add a new
background thread to process SwapBuffers requests, I fear we are introducing a
race condition similar to the DisplayLink thread. We should investigate that,
but probably not for m13.

For m13 on mac, are we sure that any use of compositing is safe without this
patch? What's special about force-compositing-mode over just having lots of
WebGL and canvas tabs open?

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:68: NSRect frame =
[self frame];
On 2011/06/08 16:52:46, Nico wrote:
> no need for a variable

Done.

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:210: }
On 2011/06/08 16:52:46, Nico wrote:
> If you don't need the trace output, remove this method (but chances are you
do)

Done.

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:214: }
On 2011/06/08 16:52:46, Nico wrote:
> Remove this method?

Done.

[Ken Russell (switch to Gerrit), 2011-06-08 18:01:05]

On 2011/06/08 17:54:42, jbates wrote:
> I'm going to leave it to the more seasoned chromium devs to decide whether its
> OK to vsync wait in the UI thread. From my limited understanding, it seems
that
> the worst case is that handling of input events is delayed by up to 16ms.
Since
> the DisplayLink thread added up to 16ms of display latency, there should be no
> net change to latency from this CL.
> 
> Although I prefer the background thread design, if we were to add a new
> background thread to process SwapBuffers requests, I fear we are introducing a
> race condition similar to the DisplayLink thread. We should investigate that,
> but probably not for m13.

Conceptually I think that this fix is the right thing to do to address this
hang. Adding back in the background thread will add risk and is something that
we should do as follow-on work if we find it's necessary.

I'm not too concerned about blocking on vsync on the UI thread. Normal OpenGL
apps do this all the time and handle mouse and keyboard input on the same thread
just fine.

As with any patch it's impossible for the reviewers to prove that it's safe.
Testing, in particular manual testing in this case, is needed to ensure that the
new behavior feels good.

> For m13 on mac, are we sure that any use of compositing is safe without this
> patch? What's special about force-compositing-mode over just having lots of
> WebGL and canvas tabs open?

I don't know but suspect it's basically a stress test.

http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):
> 
>
http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:68: NSRect frame =
> [self frame];
> On 2011/06/08 16:52:46, Nico wrote:
> > no need for a variable
> 
> Done.
> 
>
http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:210: }
> On 2011/06/08 16:52:46, Nico wrote:
> > If you don't need the trace output, remove this method (but chances are you
> do)
> 
> Done.
> 
>
http://codereview.chromium.org/6993043/diff/14001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:214: }
> On 2011/06/08 16:52:46, Nico wrote:
> > Remove this method?
> 
> Done.

[nduca, 2011-06-08 18:38:05]

Force-accel triggering this is not just a stress test.

Rather, usually when the browser boots up, it gets a chance to draw a few frames
with the software path. For example, the new tab page renders or something. This
has an indirect serializing effect with the browser's UI startup process because
of the software path's updateRect flow.

In contrast, --force-blahblah causes GPU acceleration from the absolute first
paint that the browser processes.

This leads me to believe that there's some race condition just for render-widget
startup.

If we want to force out this fix because we're in a hurry, I'm pragmatically
supportive. However, if we weren't under deadline pressure, I would lean toward
slowing down to figure out what was wrong first.

[Nico, 2011-06-08 18:39:59]

On Wed, Jun 8, 2011 at 11:38 AM,  <nduca@chromium.org> wrote:
> Force-accel triggering this is not just a stress test.
>
> Rather, usually when the browser boots up, it gets a chance to draw a few
> frames
> with the software path. For example, the new tab page renders or something.
> This
> has an indirect serializing effect with the browser's UI startup process
> because
> of the software path's updateRect flow.
>
> In contrast, --force-blahblah causes GPU acceleration from the absolute
> first
> paint that the browser processes.
>
> This leads me to believe that there's some race condition just for
> render-widget
> startup.
>
> If we want to force out this fix because we're in a hurry, I'm pragmatically
> supportive. However, if we weren't under deadline pressure, I would lean
> toward
> slowing down to figure out what was wrong first.

(The main point of the 6 week release cycle is to remove "deadline
pressure". Please don't make decisions just because you feel like you
need to hit a particular milestone.)

[jbates, 2011-06-10 01:18:17]

I found what seems to be the root cause of the bug, so I put the DisplayLink
code back in. Essentially, the DisplayLink was never being started, because the
code that starts it was only getting called in sw mode.

Take a look - maybe this is safe enough to put in for m13.

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

Nice work tracking this down. It mostly looks great. A couple of minor issues.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.h (right):

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.h:83: -
(void)setViewForGL;
I find this method name confusing because it sounds like a setter rather than
something which performs setup. What about something like prepareForGLRendering?

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:35: // Locked here
because renderWidgetHostView_ is set to NULL on another thread.
Maybe move this comment next to the call to CGLLockContext.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:200:
CGLLockContext(cglContext_);
This is unnecessary. The first thing drawView does is lock the context.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:202:
CGLUnlockContext(cglContext_);
Unnecessary.

[jbates, 2011-06-11 02:00:43]

It took a lot of hacking to get this exactly right. There are subtle issues
involved with calling setView on the NSOpenGLContext. setView will fail if not
called at exactly the right times. I couldn't find any useful documentation that
explains when it will fail.

This CL seems to work well in my quick tests. However, I think DisplayLink runs
more often than before - I couldn't find any other solution that did not result
in setView occasionally failing.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.h (right):

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.h:83: -
(void)setViewForGL;
On 2011/06/10 01:30:12, kbr wrote:
> I find this method name confusing because it sounds like a setter rather than
> something which performs setup. What about something like
prepareForGLRendering?

Good point, done.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:35: // Locked here
because renderWidgetHostView_ is set to NULL on another thread.
On 2011/06/10 01:30:12, kbr wrote:
> Maybe move this comment next to the call to CGLLockContext.

Done.

http://codereview.chromium.org/6993043/diff/17013/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:200:
CGLLockContext(cglContext_);
On 2011/06/10 01:30:12, kbr wrote:
> This is unnecessary. The first thing drawView does is lock the context.

I meant to remove the drawView mutex locking, since I had to move it outside for
the other case. Although it is recursive, seems best to avoid that when
possible.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (left):

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:38: }
moved this to its own method.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:123: }
moved this to the top so it doesn't have to be in the header.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:241: [glContext_
setView:self];
Moved this to prepareForGLRendering.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:252:
CVDisplayLinkStart(displayLink_);
Moved CVDisplayLink initialization to init method above.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:287: }
setUpGState was the only place [glContext_ setView] could be called without
sometimes failing.

[jbates, 2011-06-13 17:22:13]

Reviews? (Sorry for so many revs)

[Nico, 2011-06-13 18:43:06]

cocoa bits LGTM

wow, how did you find this? :-)

please update the CL description before checking in

as mentioned before, I'd move all the TRACE changes to a separate CL

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:19:
TRACE_EVENT1("browser", "AcceleratedPluginViewMac::drawView",
I don't know if the TRACE_EVENT macros support this, but ObjC methods are
usually spelled "-[AcceleratedPluginViewMac drawView]". Also elsewhere.

http://codereview.chromium.org/6993043/diff/16003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:290: //       in
traces.
"TODO(jbates)" (the name denotes who wrote the TODO, not necessarily who should
fix it)

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

LGTM

http://codereview.chromium.org/6993043/diff/19001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/accelerated_plugin_view_mac.mm (right):

http://codereview.chromium.org/6993043/diff/19001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/accelerated_plugin_view_mac.mm:32: // Note:
cglContext_ lock must be held during this call.
This comment applies to drawView too.