Added command buffer implementation of WebGL which runs in the sandbox....

Added command buffer implementation of WebGL which runs in the sandbox.
Added synchronous initialization of the channel to the GPU process, needed
to obey WebGL startup semantics. There are problems with this on the
Windows platform which will be addressed via refactoring in the
GpuProcessHost in a subsequent CL. Implemented offscreen rendering code
path in GGL / GLES2CmdDecoder for Mac OS X.

This new code path is not yet complete for all platforms and is still being
stress tested. The previous in-process WebGL implementation is currently
used when the sandbox is disabled; it will be removed in a subsequent CL.

A one-line code change in WebKit is needed after this CL lands to enable
the new code path.

BUG=29120
TEST=ran WebGL demos on command buffer implementation on Mac


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

[Ken Russell (switch to Gerrit), 2010-03-25 00:39:18]

This is the bulk of the remaining code needed to make WebGL work in the sandbox.
It is not yet working on all platforms, but it seems best to checkpoint it here.
This CL will also need to be merged with Al's addition of GGL
(http://codereview.chromium.org/1136006), which I assume will be checked in
first, at which point the WebGraphicsContext3DCommandBufferImpl can just use
GGL.

[apatrick, 2010-03-25 00:58:01]

http://codereview.chromium.org/1328001/diff/1/4
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/4#newcode195
chrome/browser/gpu_process_host.cc:195: CHECK(process_host->Send(msg));
I'm going to remove this CHECK in my GGL changelist. See darin's comment in that
CL. You might want to do the same in OnSynchronizeReply.

http://codereview.chromium.org/1328001/diff/1/5
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/5#newcode874
chrome/browser/renderer_host/render_widget_host.cc:874: void
RenderWidgetHost::OnMsgEstablishGpuChannel() {
I moved this BrowserRenderProcessHost in my GGL changelist. You will need to do
this for OnMsgSynchronizeGpu after I land.

http://codereview.chromium.org/1328001/diff/1/18
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/1328001/diff/1/18#newcode915
chrome/renderer/render_widget.cc:915: CHECK(Send(new
ViewHostMsg_SynchronizeGpu(routing_id_)));
See previous comment about CHECK. Also, in this case you can return NULL if Send
fails.

http://codereview.chromium.org/1328001/diff/1/13
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/1/13#newcode1
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
Is your plan to refactor all this to use the new GGL offscreen contexts when
that is checked in?

http://codereview.chromium.org/1328001/diff/1/14
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.h (right):

http://codereview.chromium.org/1328001/diff/1/14#newcode198
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:198: virtual long
getVertexAttribOffset(unsigned long index, unsigned long pname);
80 chars

http://codereview.chromium.org/1328001/diff/1/14#newcode342
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:342:
WebKit::WebGraphicsContext3D::Attributes m_attributes;
shouldn't this be google style?

http://codereview.chromium.org/1328001/diff/1/17
File chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc
(right):

http://codereview.chromium.org/1328001/diff/1/17#newcode43
chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc:43:
RenderView* view = RenderView::FromWebView(frame->view());
You won't need to get the RenderView after my GGL CL lands.

http://codereview.chromium.org/1328001/diff/1/17#newcode55
chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc:55:
return false;
command_buffer leaks here. If you're going to refactor to use GGL I don't care
though.

[brettw, 2010-03-25 20:56:44]

http://codereview.chromium.org/1328001/diff/1/4
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/4#newcode149
chrome/browser/gpu_process_host.cc:149:
queued_synchronization_replies_.push(reply);
What does it mean if the GpuProcessHost is getting deleted and there are pending
replies? Should we should either send all of them (possibly indicating some
error) or delete them in the destructor?

http://codereview.chromium.org/1328001/diff/1/5
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/5#newcode145
chrome/browser/renderer_host/render_widget_host.cc:145: OnMsgSynchronizeGpu)
Probably this should be indented to the ( like the other ones below. Can you fix
EstablishGpuChannel right above this?

http://codereview.chromium.org/1328001/diff/1/7
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/1328001/diff/1/7#newcode32
chrome/common/gpu_messages_internal.h:32: // asynchronously.)
Can you mention this will result in a GpuHostMsg_SynchronizeReply message?

http://codereview.chromium.org/1328001/diff/1/16
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/1328001/diff/1/16#newcode70
chrome/renderer/renderer_webkitclient_impl.h:70: virtual
WebKit::WebGraphicsContext3D* createGraphicsContext3D();
We don't put blank lines between functions in the block where you implement
another interface. Can you also remove the one before sharedWorkerRepository?

http://codereview.chromium.org/1328001/diff/1/13
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/1/13#newcode42
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:42:
WebGraphicsContext3DCommandBufferImpl::~WebGraphicsContext3DCommandBufferImpl()
{
Check google style.

http://codereview.chromium.org/1328001/diff/1/13#newcode51
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:51: delete context_;
Does it also make sense to make this a scoped_ptr?

http://codereview.chromium.org/1328001/diff/1/13#newcode70
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:70: switch (type) {
Check indenting is Google style.

http://codereview.chromium.org/1328001/diff/1/13#newcode183
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:183: unsigned int
rowBytes = width * 4;
row_bytes.

http://codereview.chromium.org/1328001/diff/1/13#newcode186
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:186: unsigned char*
rowA = framebuffer + i * rowBytes;
row_a, etc.

http://codereview.chromium.org/1328001/diff/1/13#newcode591
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:591: GLchar* log =
new GLchar[logLength];
Can you make this a scoped_array? Also check other places here you manually have
to free pointers.

http://codereview.chromium.org/1328001/diff/1/14
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.h (right):

http://codereview.chromium.org/1328001/diff/1/14#newcode35
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:35: public:
Check indenting, I assume you copied this from WebKit due to the 4-space
indents?

http://codereview.chromium.org/1328001/diff/1/14#newcode355
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:355: unsigned char*
m_scanline;
Can you make this a scoped_array to avoid having to manually free this buffer?

http://codereview.chromium.org/1328001/diff/1/14#newcode356
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:356: void
flipVertically(unsigned char* framebuffer,
Functions should be named with caps when they're ours.

[Ken Russell (switch to Gerrit), 2010-03-26 21:01:22]

Merged with Al's CL and changed initialization path to use GGL, which simplifies
it considerably and is working well (on Mac).

http://codereview.chromium.org/1328001/diff/1/4
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/4#newcode149
chrome/browser/gpu_process_host.cc:149:
queued_synchronization_replies_.push(reply);
On 2010/03/25 20:56:45, brettw wrote:
> What does it mean if the GpuProcessHost is getting deleted and there are
pending
> replies? Should we should either send all of them (possibly indicating some
> error) or delete them in the destructor?

I've added deletion code to the destructor. The queued_messages_ should probably
be similarly cleaned up.

http://codereview.chromium.org/1328001/diff/1/4#newcode195
chrome/browser/gpu_process_host.cc:195: CHECK(process_host->Send(msg));
On 2010/03/25 00:58:02, apatrick wrote:
> I'm going to remove this CHECK in my GGL changelist. See darin's comment in
that
> CL. You might want to do the same in OnSynchronizeReply.

I didn't see the removal of the CHECK so leaving this in place for now.

http://codereview.chromium.org/1328001/diff/1/5
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/1328001/diff/1/5#newcode145
chrome/browser/renderer_host/render_widget_host.cc:145: OnMsgSynchronizeGpu)
On 2010/03/25 20:56:45, brettw wrote:
> Probably this should be indented to the ( like the other ones below. Can you
fix
> EstablishGpuChannel right above this?

Done.

http://codereview.chromium.org/1328001/diff/1/5#newcode874
chrome/browser/renderer_host/render_widget_host.cc:874: void
RenderWidgetHost::OnMsgEstablishGpuChannel() {
On 2010/03/25 00:58:02, apatrick wrote:
> I moved this BrowserRenderProcessHost in my GGL changelist. You will need to
do
> this for OnMsgSynchronizeGpu after I land.

Done.

http://codereview.chromium.org/1328001/diff/1/7
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/1328001/diff/1/7#newcode32
chrome/common/gpu_messages_internal.h:32: // asynchronously.)
On 2010/03/25 20:56:45, brettw wrote:
> Can you mention this will result in a GpuHostMsg_SynchronizeReply message?

Done.

http://codereview.chromium.org/1328001/diff/1/18
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/1328001/diff/1/18#newcode915
chrome/renderer/render_widget.cc:915: CHECK(Send(new
ViewHostMsg_SynchronizeGpu(routing_id_)));
On 2010/03/25 00:58:02, apatrick wrote:
> See previous comment about CHECK. Also, in this case you can return NULL if
Send
> fails.

Done.

http://codereview.chromium.org/1328001/diff/1/16
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/1328001/diff/1/16#newcode70
chrome/renderer/renderer_webkitclient_impl.h:70: virtual
WebKit::WebGraphicsContext3D* createGraphicsContext3D();
On 2010/03/25 20:56:45, brettw wrote:
> We don't put blank lines between functions in the block where you implement
> another interface. Can you also remove the one before sharedWorkerRepository?

Done.

http://codereview.chromium.org/1328001/diff/1/13
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/1/13#newcode1
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
On 2010/03/25 00:58:02, apatrick wrote:
> Is your plan to refactor all this to use the new GGL offscreen contexts when
> that is checked in?

Yes. The first step (already done in the updated code) is to use a 1x1 GGL
offscreen context and to continue to create FBOs manually. The next step will be
to complete GGL's offscreen context support on all platforms and completely
switch over to using it.

http://codereview.chromium.org/1328001/diff/1/13#newcode42
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:42:
WebGraphicsContext3DCommandBufferImpl::~WebGraphicsContext3DCommandBufferImpl()
{
On 2010/03/25 20:56:45, brettw wrote:
> Check google style.

The { is on column 80.

http://codereview.chromium.org/1328001/diff/1/13#newcode51
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:51: delete context_;
On 2010/03/25 20:56:45, brettw wrote:
> Does it also make sense to make this a scoped_ptr?

This doesn't exist any more in the current version of the code.

http://codereview.chromium.org/1328001/diff/1/13#newcode70
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:70: switch (type) {
On 2010/03/25 20:56:45, brettw wrote:
> Check indenting is Google style.

Fixed.

http://codereview.chromium.org/1328001/diff/1/13#newcode183
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:183: unsigned int
rowBytes = width * 4;
On 2010/03/25 20:56:45, brettw wrote:
> row_bytes.

Fixed.

http://codereview.chromium.org/1328001/diff/1/13#newcode186
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:186: unsigned char*
rowA = framebuffer + i * rowBytes;
On 2010/03/25 20:56:45, brettw wrote:
> row_a, etc.

Done.

http://codereview.chromium.org/1328001/diff/1/13#newcode591
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:591: GLchar* log =
new GLchar[logLength];
On 2010/03/25 20:56:45, brettw wrote:
> Can you make this a scoped_array? Also check other places here you manually
have
> to free pointers.

Fixed throughout file.

http://codereview.chromium.org/1328001/diff/1/14
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.h (right):

http://codereview.chromium.org/1328001/diff/1/14#newcode35
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:35: public:
On 2010/03/25 20:56:45, brettw wrote:
> Check indenting, I assume you copied this from WebKit due to the 4-space
> indents?

You're right. Indenting fixed.

http://codereview.chromium.org/1328001/diff/1/14#newcode198
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:198: virtual long
getVertexAttribOffset(unsigned long index, unsigned long pname);
On 2010/03/25 00:58:02, apatrick wrote:
> 80 chars

Done.

http://codereview.chromium.org/1328001/diff/1/14#newcode342
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:342:
WebKit::WebGraphicsContext3D::Attributes m_attributes;
On 2010/03/25 00:58:02, apatrick wrote:
> shouldn't this be google style?

Right. Fixed all of these.

http://codereview.chromium.org/1328001/diff/1/14#newcode355
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:355: unsigned char*
m_scanline;
On 2010/03/25 20:56:45, brettw wrote:
> Can you make this a scoped_array to avoid having to manually free this buffer?

Done.

http://codereview.chromium.org/1328001/diff/1/14#newcode356
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:356: void
flipVertically(unsigned char* framebuffer,
On 2010/03/25 20:56:45, brettw wrote:
> Functions should be named with caps when they're ours.

Fixed.

http://codereview.chromium.org/1328001/diff/1/17
File chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc
(right):

http://codereview.chromium.org/1328001/diff/1/17#newcode43
chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc:43:
RenderView* view = RenderView::FromWebView(frame->view());
On 2010/03/25 00:58:02, apatrick wrote:
> You won't need to get the RenderView after my GGL CL lands.

Correct. It's been removed.

http://codereview.chromium.org/1328001/diff/1/17#newcode55
chrome/renderer/webgraphicscontext3d_command_buffer_impl_initializer.cc:55:
return false;
On 2010/03/25 00:58:02, apatrick wrote:
> command_buffer leaks here. If you're going to refactor to use GGL I don't care
> though.

Already refactored to use GGL.

[apatrick, 2010-03-26 21:27:41]

lgtm

http://codereview.chromium.org/1328001/diff/20001/21013
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21013#newcode709
chrome/renderer/render_thread.cc:709: // TODO(kbr): the GPU channel is still in
the unconnected state at this point.
Agreed. I think there might be a race condition here.

http://codereview.chromium.org/1328001/diff/20001/21017
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21017#newcode356
chrome/renderer/renderer_webkitclient_impl.cc:356: return 0;
0 -> NULL

http://codereview.chromium.org/1328001/diff/20001/21015
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21015#newcode18
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:18: GLenum error =
glGetError();
OpenGL can return multiple errors though consecutive calls to glGetError. I
think it doesn't need to go in this CL. TODO.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode57
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:57: return true;
return true is redundant.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode61
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:61: return
cachedWidth_;
not google style

http://codereview.chromium.org/1328001/diff/20001/21015#newcode65
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:65: return
cachedHeight_;
not google style

http://codereview.chromium.org/1328001/diff/20001/21015#newcode140
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:140:
glBindRenderbuffer(GL_RENDERBUFFER, depthBuffer_);
depthBuffer_ is not google style.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode165
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:165:
scanline_.reset(new unsigned char[width * 4]);
unsinged char -> uint8

http://codereview.chromium.org/1328001/diff/20001/21015#newcode174
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:174: unsigned char*
framebuffer,
unsigned char -> uint8. and below.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode449
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:449:
DELEGATE_TO_GL_1(enableVertexAttribArray, EnableVertexAttribArray, unsigned
long)
80 chars

http://codereview.chromium.org/1328001/diff/20001/21015#newcode471
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:471: GLint
maxNameLength = -1;
google style everywhere!

http://codereview.chromium.org/1328001/diff/20001/21015#newcode483
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:483:
glGetActiveAttrib(program, index, maxNameLength,
I'm not familiar. Should maxNameLength be a length or a size including the null
terminator?

http://codereview.chromium.org/1328001/diff/20001/21015#newcode532
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:532: unsigned long
WebGraphicsContext3DCommandBufferImpl::getError() {
is unsigned long dictated by the interface?

http://codereview.chromium.org/1328001/diff/20001/21015#newcode615
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:615: return
WebKit::WebString::fromUTF8(reinterpret_cast<const char*>(glGetString(name)));
80 chars

http://codereview.chromium.org/1328001/diff/20001/21002
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21002#newcode1807
gpu/command_buffer/service/gles2_cmd_decoder.cc:1807: pbuffer_ = NULL;
Good catch.

[Ken Russell (switch to Gerrit), 2010-03-26 21:47:51]

http://codereview.chromium.org/1328001/diff/20001/21017
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21017#newcode356
chrome/renderer/renderer_webkitclient_impl.cc:356: return 0;
On 2010/03/26 21:27:42, apatrick wrote:
> 0 -> NULL

Done.

http://codereview.chromium.org/1328001/diff/20001/21015
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21015#newcode18
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:18: GLenum error =
glGetError();
On 2010/03/26 21:27:42, apatrick wrote:
> OpenGL can return multiple errors though consecutive calls to glGetError. I
> think it doesn't need to go in this CL. TODO.

TODO added. All of the code which calls checkGLError is going to go away in a
subsequent CL anyway.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode57
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:57: return true;
On 2010/03/26 21:27:42, apatrick wrote:
> return true is redundant.

Oops, that was left over. (Not sure why it compiled successfully.) Removed.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode61
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:61: return
cachedWidth_;
On 2010/03/26 21:27:42, apatrick wrote:
> not google style

Fixed.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode65
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:65: return
cachedHeight_;
On 2010/03/26 21:27:42, apatrick wrote:
> not google style

Fixed.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode140
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:140:
glBindRenderbuffer(GL_RENDERBUFFER, depthBuffer_);
On 2010/03/26 21:27:42, apatrick wrote:
> depthBuffer_ is not google style.

Changed.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode165
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:165:
scanline_.reset(new unsigned char[width * 4]);
On 2010/03/26 21:27:42, apatrick wrote:
> unsinged char -> uint8

Done.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode174
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:174: unsigned char*
framebuffer,
On 2010/03/26 21:27:42, apatrick wrote:
> unsigned char -> uint8. and below.

Done.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode449
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:449:
DELEGATE_TO_GL_1(enableVertexAttribArray, EnableVertexAttribArray, unsigned
long)
On 2010/03/26 21:27:42, apatrick wrote:
> 80 chars

The ) is on column 80.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode471
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:471: GLint
maxNameLength = -1;
On 2010/03/26 21:27:42, apatrick wrote:
> google style everywhere!

This code came from the WebKit repository, which is why it conformed to that
style and not Google style. Fixed.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode483
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:483:
glGetActiveAttrib(program, index, maxNameLength,
On 2010/03/26 21:27:42, apatrick wrote:
> I'm not familiar. Should maxNameLength be a length or a size including the
null
> terminator?

max_name_length includes the null terminator. length does not.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode532
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:532: unsigned long
WebGraphicsContext3DCommandBufferImpl::getError() {
On 2010/03/26 21:27:42, apatrick wrote:
> is unsigned long dictated by the interface?

Yes.

http://codereview.chromium.org/1328001/diff/20001/21015#newcode615
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:615: return
WebKit::WebString::fromUTF8(reinterpret_cast<const char*>(glGetString(name)));
On 2010/03/26 21:27:42, apatrick wrote:
> 80 chars

Fixed.

http://codereview.chromium.org/1328001/diff/20001/21002
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1328001/diff/20001/21002#newcode1807
gpu/command_buffer/service/gles2_cmd_decoder.cc:1807: pbuffer_ = NULL;
On 2010/03/26 21:27:42, apatrick wrote:
> Good catch.

No, it was already there. It's just how diff interpreted my additions.

[apatrick, 2010-03-26 22:08:28]

lgtm

[brettw, 2010-03-26 22:18:11]

LGTM, I only spot checked the OpenGL commands.

http://codereview.chromium.org/1328001/diff/1/13
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/1/13#newcode42
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:42:
WebGraphicsContext3DCommandBufferImpl::~WebGraphicsContext3DCommandBufferImpl()
{
Me, vi & Reitveld count 81.

http://codereview.chromium.org/1328001/diff/42001/22030#newcode122
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:122: // Generate the
depth buffer
What is the memory management for these fbo and the render buffers. Do you need
to free them?

http://codereview.chromium.org/1328001/diff/42001/22031
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.h (right):

http://codereview.chromium.org/1328001/diff/42001/22031#newcode25
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:25: namespace gles2 {
Don't indent things in namespaces.

[Ken Russell (switch to Gerrit), 2010-03-26 22:36:57]

http://codereview.chromium.org/1328001/diff/1/13
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/1328001/diff/1/13#newcode42
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:42:
WebGraphicsContext3DCommandBufferImpl::~WebGraphicsContext3DCommandBufferImpl()
{
On 2010/03/26 22:18:11, brettw wrote:
> Me, vi & Reitveld count 81.

Shoot, emacs counts columns starting at 0. Fixed.

http://codereview.chromium.org/1328001/diff/42001/22030#newcode122
chrome/renderer/webgraphicscontext3d_command_buffer_impl.cc:122: // Generate the
depth buffer
On 2010/03/26 22:18:11, brettw wrote:
> What is the memory management for these fbo and the render buffers. Do you
need
> to free them?

Their lifetime is that of the GGL context, so they are automatically cleaned up
in the destructor.

http://codereview.chromium.org/1328001/diff/42001/22031
File chrome/renderer/webgraphicscontext3d_command_buffer_impl.h (right):

http://codereview.chromium.org/1328001/diff/42001/22031#newcode25
chrome/renderer/webgraphicscontext3d_command_buffer_impl.h:25: namespace gles2 {
On 2010/03/26 22:18:11, brettw wrote:
> Don't indent things in namespaces.

Fixed.

[darin (slow to review), 2010-03-29 15:24:31]

http://codereview.chromium.org/1328001/diff/56001/57017
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/1328001/diff/56001/57017#newcode350
chrome/renderer/renderer_webkitclient_impl.cc:350: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoSandbox)) {
nit: it seems like a more specific command line switch would be better here long
term.  much as we have --in-process-plugins, a chrome developer might like to
run webgl with the command buffer code eliminated in order to reduce complexity
in the hopes of more quickly ruling out possible sources of a bug.

[Ken Russell (switch to Gerrit), 2010-03-29 19:56:30]

http://codereview.chromium.org/1328001/diff/56001/57017
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/1328001/diff/56001/57017#newcode350
chrome/renderer/renderer_webkitclient_impl.cc:350: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoSandbox)) {
On 2010/03/29 15:24:32, darin wrote:
> nit: it seems like a more specific command line switch would be better here
long
> term.  much as we have --in-process-plugins, a chrome developer might like to
> run webgl with the command buffer code eliminated in order to reduce
complexity
> in the hopes of more quickly ruling out possible sources of a bug.

Good suggestion. Filed as http://crbug.com/39721 .