[GPU] Get GPU process running on the mac...

[GPU] Get GPU process running on the mac

Initial port of GPU process / plugin to Mac OS X. Uses new IOSurface
APIs and therefore currently runs only on 10.6. Alternate strategy
will need to be devised for 10.5.

Slight UI issues remain such as GPU plugins initially showing up in
the wrong place on the page. These will be fixed in follow-on bugs.

Minimal changes made to command buffer code to get it to compile on
Mac OS X. Commented out use of nested anonymous namespaces in
gles2_cmd_decoder.cc which were causing the linker to crash with a seg
fault.

Refactored gyp files so the OS test enabling the GPU plugin is in one
place, common.gypi, and other files test only the variable enable_gpu.

Slight change to gles2_demo_cc.cc to add some simple animation to
verify that updates from the GPU plugin are reaching the screen.
Changed Pepper test plugin to use 3D view by default and commented out
use of audio context because of recent issues.

TEST=none (ran Pepper Test Plugin with 3D view enabled)
BUG=http://crbug.com/25988


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

[Ken Russell (switch to Gerrit), 2010-01-29 02:45:06]

Please review. Thanks.

[greggman, 2010-01-29 04:06:45]

I'm not sure if I'm the guy that should be reviewing this as I know nothing
about OSX but I reviewed it as much as I think I understand.

http://codereview.chromium.org/558035/diff/1/2
File app/gfx/native_widget_types.h (right):

http://codereview.chromium.org/558035/diff/1/2#newcode139
app/gfx/native_widget_types.h:139: #else
If this part is OSX should it say #elif (OSX) or whatever is appropriate?

If it was me I'd follow that with

#else
#error unknown platform
#endif

http://codereview.chromium.org/558035/diff/1/39
File chrome/common/io_surface_support_mac.cc (right):

http://codereview.chromium.org/558035/diff/1/39#newcode210
chrome/common/io_surface_support_mac.cc:210: ,
k_io_surface_height_(k_io_surface_height)
unfortunately the style guide has the comma's on the right
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/558035/diff/1/44
File chrome/plugin/command_buffer_stub.cc (right):

http://codereview.chromium.org/558035/diff/1/44#newcode18
chrome/plugin/command_buffer_stub.cc:18: ,
plugin_host_route_id_(plugin_host_route_id)
unfortunately the style guide as the commas on the right :-(

http://codereview.chromium.org/558035/diff/1/49
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/558035/diff/1/49#newcode157
chrome/renderer/webplugin_delegate_pepper.cc:157: // Send the new window size to
the command buffer service code so it
I'm not sure this is related but...

Is there any possible way this can effect the canvas tag?

Bascially if I go like this

canvas3d.width = 150; // resized from 200
canvas2d.drawImage(canvas3d, 0, 0);

The result of that draw has to have a width of 150.  Or similarly

canvas3d.width = 150; // resized from 200
url = canvas3d.toDataURL();

That data URL has be a png with a width of 150.

My point is only the word "async" has popped up a light in my head. I assume
everything just magically works out?

http://codereview.chromium.org/558035/diff/1/20
File gpu/command_buffer/common/cmd_buffer_common.cc (right):

http://codereview.chromium.org/558035/diff/1/20#newcode12
gpu/command_buffer/common/cmd_buffer_common.cc:12: const int32
CommandHeader::kMaxSize = (1 << 21) - 1;
Why did this have to get moved to the .c??  I'd prefer it be a compile time
constant if possible. Is this the linker bug?

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
What happened here?

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode20
gpu/command_buffer/service/gles2_cmd_decoder.cc:20: #if defined(OS_LINUX)
I'm pretty sure this needs to be

#if defined(UNIT_TEST)
#elif defined(OS_LINUX)

The unit_test version of this should include no platform specific code.

http://codereview.chromium.org/558035/diff/1/18#newcode44
gpu/command_buffer/service/gles2_cmd_decoder.cc:44: // namespace {
then please put "static" in front of the function below.

(Although strange as we're using anonymous namespaces all over the place as they
are part of the google c++ style guide and even encourages)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Is it possible the error is actually something else?

http://codereview.chromium.org/558035/diff/1/18#newcode499
gpu/command_buffer/service/gles2_cmd_decoder.cc:499: #if defined(OS_MACOSX)
Same here
#if defined(UNIT_TEST)
#elif defined(OS_MACOSX)

or 

#if !defined(UNIT_TEST) && defined(OS_MACOSX)

either one

http://codereview.chromium.org/558035/diff/1/18#newcode1088
gpu/command_buffer/service/gles2_cmd_decoder.cc:1088: CGLPixelFormatAttribute
attribs[] = {
can this be static?

http://codereview.chromium.org/558035/diff/1/18#newcode1094
gpu/command_buffer/service/gles2_cmd_decoder.cc:1094: if
(CGLChoosePixelFormat(attribs, &pixelFormat, &numPixelFormats) != kCGLNoError) {
> 80 chars

note: you can do

gcl lint <changename>

to get lint checks.

http://codereview.chromium.org/558035/diff/1/18#newcode1109
gpu/command_buffer/service/gles2_cmd_decoder.cc:1109: if (CGLCreatePBuffer(1, 1,
GL_TEXTURE_2D, GL_RGBA, 0, &pbuffer) != kCGLNoError) {
> 80 chars

http://codereview.chromium.org/558035/diff/1/18#newcode1185
gpu/command_buffer/service/gles2_cmd_decoder.cc:1185: #if defined(OS_MACOSX)
Needs the #if UNIT_TEST treatment

http://codereview.chromium.org/558035/diff/1/18#newcode1200
gpu/command_buffer/service/gles2_cmd_decoder.cc:1200: static IOSurfaceSupport*
io_surface_support = NULL;
A global variable will not work will it?

Multiple processes will share this variable?

Do we need to worry that IOSurfaceSuppport::Initalize won't be called?

Also, if this is the correct solution, how about moving the variable inside the
function. No need for it to be in the file level namespace?

http://codereview.chromium.org/558035/diff/1/6
File webkit/glue/plugins/mac_gpu_plugin_container.cc (right):

http://codereview.chromium.org/558035/diff/1/6#newcode11
webkit/glue/plugins/mac_gpu_plugin_container.cc:11: static IOSurfaceSupport*
io_surface_support = NULL;
Same question I had before. Is this global going to cause problems?

http://codereview.chromium.org/558035/diff/1/6#newcode15
webkit/glue/plugins/mac_gpu_plugin_container.cc:15: , y_(0)
comma on right (unless this is webkit code) I'm guessing from the copyright it's
not.

http://codereview.chromium.org/558035/diff/1/6#newcode68
webkit/glue/plugins/mac_gpu_plugin_container.cc:68:
io_surface_support->CGLTexImageIOSurface2D(context,
hmmm, It sounds like the GLES2 code needs to hide the fact that there is a
texture bound.  Yea? I guess I should add that to my list. Otherwise the user
can query the currently bound texture, get that ID and manipulation it.

WebGL needs that as a conformance test. That at init time it appears no textures
or render buffers are bound.

http://codereview.chromium.org/558035/diff/1/6#newcode81
webkit/glue/plugins/mac_gpu_plugin_container.cc:81: glBindTexture(target,
texture_);
Does this state need to be saved? I'm guessing the idea is that this is a
separate but GL context from the one the client code is using, they just share
resources?

http://codereview.chromium.org/558035/diff/1/9
File webkit/glue/plugins/mac_gpu_plugin_container_manager.cc (right):

http://codereview.chromium.org/558035/diff/1/9#newcode51
webkit/glue/plugins/mac_gpu_plugin_container_manager.cc:51: glClearColor(0, 0,
0, 0);
do these states need to be saved and restored?

http://codereview.chromium.org/558035/diff/1/10
File webkit/glue/plugins/mac_gpu_plugin_container_manager.h (right):

http://codereview.chromium.org/558035/diff/1/10#newcode59
webkit/glue/plugins/mac_gpu_plugin_container_manager.h:59: typedef
std::map<gfx::PluginWindowHandle, MacGPUPluginContainer*>
PluginWindowToContainerMap;
80 chars? or is this webkit code? I'm guessing from the copyright it's not

[apatrick, 2010-01-29 18:59:09]

Awesome! There might be some duplicates with gman's comments. I haven't reviewed
the mac specific calls, the OpenGL calls or the objective C code.

http://codereview.chromium.org/558035/diff/1/29
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/558035/diff/1/29#newcode168
chrome/browser/renderer_host/render_widget_host_view.h:168: virtual void
DestroyFakePluginWindowHandle(gfx::PluginWindowHandle window) = 0;
80 chars

http://codereview.chromium.org/558035/diff/1/32
File chrome/browser/renderer_host/test/test_render_view_host.cc (right):

http://codereview.chromium.org/558035/diff/1/32#newcode96
chrome/browser/renderer_host/test/test_render_view_host.cc:96:
TestRenderWidgetHostView::AllocateFakePluginWindowHandle() {
4 chars indent I think

http://codereview.chromium.org/558035/diff/1/32#newcode101
chrome/browser/renderer_host/test/test_render_view_host.cc:101:
gfx::PluginWindowHandle window) {
should these be nop or NOTREACHED()?

http://codereview.chromium.org/558035/diff/1/39
File chrome/common/io_surface_support_mac.cc (right):

http://codereview.chromium.org/558035/diff/1/39#newcode76
chrome/common/io_surface_support_mac.cc:76: IOSurfaceSupportImpl*
IOSurfaceSupportImpl::sole_instance_ = NULL;
I think there is a class in base for managing singletons.

http://codereview.chromium.org/558035/diff/1/40
File chrome/common/io_surface_support_mac.h (right):

http://codereview.chromium.org/558035/diff/1/40#newcode1
chrome/common/io_surface_support_mac.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
2010

http://codereview.chromium.org/558035/diff/1/23
File gpu/command_buffer/client/esTransform.h (right):

http://codereview.chromium.org/558035/diff/1/23#newcode2
gpu/command_buffer/client/esTransform.h:2: // Book:      OpenGL(R) ES 2.0
Programming Guide
Does this change need to go in with this CL? Perhaps it would be better to move
the gles2_demo with Alok's demos so that these files do not have to be added
here. They should really be in third_party. Alternatively, just retire this
demo. Alok has some better ones now.

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode1094
gpu/command_buffer/service/gles2_cmd_decoder.cc:1094: if
(CGLChoosePixelFormat(attribs, &pixelFormat, &numPixelFormats) != kCGLNoError) {
80 chars

http://codereview.chromium.org/558035/diff/1/18#newcode1109
gpu/command_buffer/service/gles2_cmd_decoder.cc:1109: if (CGLCreatePBuffer(1, 1,
GL_TEXTURE_2D, GL_RGBA, 0, &pbuffer) != kCGLNoError) {
80 chars

http://codereview.chromium.org/558035/diff/1/15
File gpu/command_buffer/service/gpu_processor_mac.cc (right):

http://codereview.chromium.org/558035/diff/1/15#newcode1
gpu/command_buffer/service/gpu_processor_mac.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
2010

http://codereview.chromium.org/558035/diff/1/9
File webkit/glue/plugins/mac_gpu_plugin_container_manager.cc (right):

http://codereview.chromium.org/558035/diff/1/9#newcode16
webkit/glue/plugins/mac_gpu_plugin_container_manager.cc:16:
MacGPUPluginContainerManager::AllocateFakePluginWindowHandle() {
4 char indent

http://codereview.chromium.org/558035/diff/1/10
File webkit/glue/plugins/mac_gpu_plugin_container_manager.h (right):

http://codereview.chromium.org/558035/diff/1/10#newcode59
webkit/glue/plugins/mac_gpu_plugin_container_manager.h:59: typedef
std::map<gfx::PluginWindowHandle, MacGPUPluginContainer*>
PluginWindowToContainerMap;
80 chars

http://codereview.chromium.org/558035/diff/1/13
File webkit/tools/pepper_test_plugin/plugin_object.cc (right):

http://codereview.chromium.org/558035/diff/1/13#newcode311
webkit/tools/pepper_test_plugin/plugin_object.cc:311: // TODO(kbr): put back in
audio portion of test
#if !defined(OS_MACOSX)

http://codereview.chromium.org/558035/diff/1/13#newcode351
webkit/tools/pepper_test_plugin/plugin_object.cc:351: // TODO(kbr): put back in
audio portion of test
#if !defined(OS_MACOSX)

http://codereview.chromium.org/558035/diff/1/13#newcode394
webkit/tools/pepper_test_plugin/plugin_object.cc:394: // TODO(kbr): put back in
audio portion of test
#if !defined(OS_MACOSX)

http://codereview.chromium.org/558035/diff/1/14
File webkit/tools/pepper_test_plugin/test_page.html (right):

http://codereview.chromium.org/558035/diff/1/14#newcode32
webkit/tools/pepper_test_plugin/test_page.html:32: <object id="plugin"
type="pepper-application/x-pepper-test-plugin" width="400" height="400"
dimensions="3" />
I don't think this change needs to be part of the CL.

http://codereview.chromium.org/558035/diff/1/3
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/558035/diff/1/3#newcode185
webkit/tools/test_shell/mac/test_webview_delegate.mm:185: containing_view);
isn't containing_view always going to be NULL here?

[stuartmorgan, 2010-01-29 21:02:18]

Mostly just looked at the Mac plugin stuff; I'm not familiar with all the GL and
IOSurface calls, so it's unlikely I'd notice any mistakes in the
graphics-intensive part of the code.

http://codereview.chromium.org/558035/diff/1/31
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/1/31#newcode100
chrome/browser/renderer_host/render_widget_host_view_mac.mm:100:
-(void)drawInCGLContext:(CGLContextObj)glContext
pixelFormat:(CGLPixelFormatObj)pixelFormat
forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp
*)timeStamp
80 chars, and { should be on the last line of the signature.

http://codereview.chromium.org/558035/diff/1/31#newcode529
chrome/browser/renderer_host/render_widget_host_view_mac.mm:529: [our_view
setWantsLayer: YES];
No spaces after : (needs fixing throughout this code)

http://codereview.chromium.org/558035/diff/1/31#newcode569
chrome/browser/renderer_host/render_widget_host_view_mac.mm:569: if
(gpu_plugin_layer_ != nil)
This check isn't necessary.

http://codereview.chromium.org/558035/diff/1/31#newcode1023
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1023: if
(renderWidgetHostView_->gpu_plugin_layer_ != nil)
Again, no need for the check.

http://codereview.chromium.org/558035/diff/1/52
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/558035/diff/1/52#newcode149
chrome/renderer/webplugin_delegate_proxy.h:149: // work. For now we pass an ID
from IOSurfaceGetID.
This comment seems to get repeated a lot. How about having it just on the
message declaration comment?

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode679
gpu/command_buffer/service/gles2_cmd_decoder.cc:679: CFTypeRef io_surface_;
scoped_cftyperef? (The last one created doesn't appear to be released, but I
assume that's a bug.)

http://codereview.chromium.org/558035/diff/1/18#newcode1246
gpu/command_buffer/service/gles2_cmd_decoder.cc:1246: CFMutableDictionaryRef
properties;
This appears to be leaked; use a scoped_cftyperef instead of a raw reference.

http://codereview.chromium.org/558035/diff/1/11
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/558035/diff/1/11#newcode410
webkit/glue/webplugin_impl.cc:410: // if (window) DCHECK(!window_);
Remove?

[Ken Russell (switch to Gerrit), 2010-01-29 23:34:11]

Addressed all review feedback. Fixed all lint errors. Fixed build failures
(waiting for try bots to confirm).

http://codereview.chromium.org/558035/diff/1/2
File app/gfx/native_widget_types.h (right):

http://codereview.chromium.org/558035/diff/1/2#newcode139
app/gfx/native_widget_types.h:139: #else
Agree, but I'd rather make minimal changes and this is a preexisting #ifdef.

http://codereview.chromium.org/558035/diff/1/29
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/558035/diff/1/29#newcode168
chrome/browser/renderer_host/render_widget_host_view.h:168: virtual void
DestroyFakePluginWindowHandle(gfx::PluginWindowHandle window) = 0;
On 2010/01/29 18:59:10, apatrick wrote:
> 80 chars

Done.

http://codereview.chromium.org/558035/diff/1/31
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/1/31#newcode100
chrome/browser/renderer_host/render_widget_host_view_mac.mm:100:
-(void)drawInCGLContext:(CGLContextObj)glContext
pixelFormat:(CGLPixelFormatObj)pixelFormat
forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp
*)timeStamp
On 2010/01/29 21:02:18, stuartmorgan wrote:
> 80 chars, and { should be on the last line of the signature.

Done.

http://codereview.chromium.org/558035/diff/1/31#newcode529
chrome/browser/renderer_host/render_widget_host_view_mac.mm:529: [our_view
setWantsLayer: YES];
On 2010/01/29 21:02:18, stuartmorgan wrote:
> No spaces after : (needs fixing throughout this code)

Done.

http://codereview.chromium.org/558035/diff/1/31#newcode569
chrome/browser/renderer_host/render_widget_host_view_mac.mm:569: if
(gpu_plugin_layer_ != nil)
On 2010/01/29 21:02:18, stuartmorgan wrote:
> This check isn't necessary.

I see. Removed.

http://codereview.chromium.org/558035/diff/1/31#newcode1023
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1023: if
(renderWidgetHostView_->gpu_plugin_layer_ != nil)
On 2010/01/29 21:02:18, stuartmorgan wrote:
> Again, no need for the check.

Done.

http://codereview.chromium.org/558035/diff/1/32
File chrome/browser/renderer_host/test/test_render_view_host.cc (right):

http://codereview.chromium.org/558035/diff/1/32#newcode96
chrome/browser/renderer_host/test/test_render_view_host.cc:96:
TestRenderWidgetHostView::AllocateFakePluginWindowHandle() {
On 2010/01/29 18:59:10, apatrick wrote:
> 4 chars indent I think

This case doesn't seem to be covered in the Google C++ style guide. The Google
C++ emacs lisp doesn't indent this, so I'll leave it.

http://codereview.chromium.org/558035/diff/1/32#newcode101
chrome/browser/renderer_host/test/test_render_view_host.cc:101:
gfx::PluginWindowHandle window) {
On 2010/01/29 18:59:10, apatrick wrote:
> should these be nop or NOTREACHED()?

I'm not really sure, but other places in these test harnesses use empty
functions.

http://codereview.chromium.org/558035/diff/1/39
File chrome/common/io_surface_support_mac.cc (right):

http://codereview.chromium.org/558035/diff/1/39#newcode76
chrome/common/io_surface_support_mac.cc:76: IOSurfaceSupportImpl*
IOSurfaceSupportImpl::sole_instance_ = NULL;
On 2010/01/29 18:59:10, apatrick wrote:
> I think there is a class in base for managing singletons.

Thanks, I've restructured this code to use it.

http://codereview.chromium.org/558035/diff/1/39#newcode210
chrome/common/io_surface_support_mac.cc:210: ,
k_io_surface_height_(k_io_surface_height)
On 2010/01/29 04:06:45, greggman wrote:
> unfortunately the style guide has the comma's on the right
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

I've changed how this code initializes so this constructor goes away.

http://codereview.chromium.org/558035/diff/1/40
File chrome/common/io_surface_support_mac.h (right):

http://codereview.chromium.org/558035/diff/1/40#newcode1
chrome/common/io_surface_support_mac.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/01/29 18:59:10, apatrick wrote:
> 2010

Done.

http://codereview.chromium.org/558035/diff/1/44
File chrome/plugin/command_buffer_stub.cc (right):

http://codereview.chromium.org/558035/diff/1/44#newcode18
chrome/plugin/command_buffer_stub.cc:18: ,
plugin_host_route_id_(plugin_host_route_id)
On 2010/01/29 04:06:45, greggman wrote:
> unfortunately the style guide as the commas on the right :-(

Done.

http://codereview.chromium.org/558035/diff/1/49
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/558035/diff/1/49#newcode157
chrome/renderer/webplugin_delegate_pepper.cc:157: // Send the new window size to
the command buffer service code so it
It is definitely possible that more work will be needed here. I was most
concerned about not introducing cross-process deadlocks which is why only one of
the new IPC messages (ViewHostMsg_AllocateFakePluginWindowHandle) is
synchronous. We will need to write test cases to make sure that the rendering
results are correct if the plugin or canvas is resized while drawing is
occurring. For the time being, the cross-process reference counting of the
IOSurface objects should be correct using the asynchronous messages.

http://codereview.chromium.org/558035/diff/1/52
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/558035/diff/1/52#newcode149
chrome/renderer/webplugin_delegate_proxy.h:149: // work. For now we pass an ID
from IOSurfaceGetID.
On 2010/01/29 21:02:18, stuartmorgan wrote:
> This comment seems to get repeated a lot. How about having it just on the
> message declaration comment?

Done.

http://codereview.chromium.org/558035/diff/1/23
File gpu/command_buffer/client/esTransform.h (right):

http://codereview.chromium.org/558035/diff/1/23#newcode2
gpu/command_buffer/client/esTransform.h:2: // Book:      OpenGL(R) ES 2.0
Programming Guide
It was very important to test even just simple animation in this CL. I don't
mind refactoring the content of the pepper test plugin but would rather do that
in a separate CL, so would appreciate being able to put this in place even
temporarily.

http://codereview.chromium.org/558035/diff/1/20
File gpu/command_buffer/common/cmd_buffer_common.cc (right):

http://codereview.chromium.org/558035/diff/1/20#newcode12
gpu/command_buffer/common/cmd_buffer_common.cc:12: const int32
CommandHeader::kMaxSize = (1 << 21) - 1;
This isn't the linker crash. When linking the Pepper Test Plugin on Mac OS X,
the symbol gpu::CommandHeader::kMaxSize was listed as missing. I'll put this
back in the header in a way that works with this toolchain
(gpu::kCommandHeaderMaxSize).

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
When compiling the Pepper Test Plugin on Mac OS X the linker complained about
the missing symbol gpu::CommandHeader::kMaxSize. Apparently this toolchain
doesn't like static constants in structs / classes.

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode20
gpu/command_buffer/service/gles2_cmd_decoder.cc:20: #if defined(OS_LINUX)
On 2010/01/29 04:06:45, greggman wrote:
> I'm pretty sure this needs to be
> 
> #if defined(UNIT_TEST)
> #elif defined(OS_LINUX)
> 
> The unit_test version of this should include no platform specific code.

Done.

http://codereview.chromium.org/558035/diff/1/18#newcode44
gpu/command_buffer/service/gles2_cmd_decoder.cc:44: // namespace {
On 2010/01/29 04:06:45, greggman wrote:
> then please put "static" in front of the function below.

Done.

> (Although strange as we're using anonymous namespaces all over the place as
they
> are part of the google c++ style guide and even encourages)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> Is it possible the error is actually something else?

We still don't know exactly why the linker crashes. I tried unsuccessfully to
produce a reduced test case based on the code structure in this file. However,
uncommenting any of the nested anonymous namespaces in this file causes the
linker to crash at least on Mac OS X 10.6 with Xcode 3.2.1.

http://codereview.chromium.org/558035/diff/1/18#newcode499
gpu/command_buffer/service/gles2_cmd_decoder.cc:499: #if defined(OS_MACOSX)
On 2010/01/29 04:06:45, greggman wrote:
> Same here
> #if defined(UNIT_TEST)
> #elif defined(OS_MACOSX)
> 
> or 
> 
> #if !defined(UNIT_TEST) && defined(OS_MACOSX)
> 
> either one

Done.

http://codereview.chromium.org/558035/diff/1/18#newcode679
gpu/command_buffer/service/gles2_cmd_decoder.cc:679: CFTypeRef io_surface_;
On 2010/01/29 21:02:18, stuartmorgan wrote:
> scoped_cftyperef? (The last one created doesn't appear to be released, but I
> assume that's a bug.)

Thanks, changed to use scoped_cftyperef<CFTypeRef>.

http://codereview.chromium.org/558035/diff/1/18#newcode1088
gpu/command_buffer/service/gles2_cmd_decoder.cc:1088: CGLPixelFormatAttribute
attribs[] = {
On 2010/01/29 04:06:45, greggman wrote:
> can this be static?
> 

I don't see a need for it to be.

http://codereview.chromium.org/558035/diff/1/18#newcode1094
gpu/command_buffer/service/gles2_cmd_decoder.cc:1094: if
(CGLChoosePixelFormat(attribs, &pixelFormat, &numPixelFormats) != kCGLNoError) {
On 2010/01/29 04:06:45, greggman wrote:
> > 80 chars
> 
> note: you can do
> 
> gcl lint <changename>
> 
> to get lint checks.

Fixed.

gcl lint reports all lint errors in the touched files, not just in the new code.
It spewed over 100 errors for this CL, the vast majority not in code I touched.

http://codereview.chromium.org/558035/diff/1/18#newcode1109
gpu/command_buffer/service/gles2_cmd_decoder.cc:1109: if (CGLCreatePBuffer(1, 1,
GL_TEXTURE_2D, GL_RGBA, 0, &pbuffer) != kCGLNoError) {
On 2010/01/29 04:06:45, greggman wrote:
> > 80 chars

Done.

http://codereview.chromium.org/558035/diff/1/18#newcode1185
gpu/command_buffer/service/gles2_cmd_decoder.cc:1185: #if defined(OS_MACOSX)
On 2010/01/29 04:06:45, greggman wrote:
> Needs the #if UNIT_TEST treatment

Done.

http://codereview.chromium.org/558035/diff/1/18#newcode1200
gpu/command_buffer/service/gles2_cmd_decoder.cc:1200: static IOSurfaceSupport*
io_surface_support = NULL;
On 2010/01/29 04:06:45, greggman wrote:
> A global variable will not work will it?
> 
> Multiple processes will share this variable?
> 
> Do we need to worry that IOSurfaceSuppport::Initalize won't be called?
> 
> Also, if this is the correct solution, how about moving the variable inside
the
> function. No need for it to be in the file level namespace?

You're right, this was sloppy and unnecessary. I've moved this variable into the
function.

http://codereview.chromium.org/558035/diff/1/18#newcode1246
gpu/command_buffer/service/gles2_cmd_decoder.cc:1246: CFMutableDictionaryRef
properties;
On 2010/01/29 21:02:18, stuartmorgan wrote:
> This appears to be leaked; use a scoped_cftyperef instead of a raw reference.

Thanks, done.

http://codereview.chromium.org/558035/diff/1/15
File gpu/command_buffer/service/gpu_processor_mac.cc (right):

http://codereview.chromium.org/558035/diff/1/15#newcode1
gpu/command_buffer/service/gpu_processor_mac.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/01/29 18:59:10, apatrick wrote:
> 2010

Fixed here and in the other new files.

http://codereview.chromium.org/558035/diff/1/6
File webkit/glue/plugins/mac_gpu_plugin_container.cc (right):

http://codereview.chromium.org/558035/diff/1/6#newcode11
webkit/glue/plugins/mac_gpu_plugin_container.cc:11: static IOSurfaceSupport*
io_surface_support = NULL;
On 2010/01/29 04:06:45, greggman wrote:
> Same question I had before. Is this global going to cause problems?

I've removed the global.

http://codereview.chromium.org/558035/diff/1/6#newcode15
webkit/glue/plugins/mac_gpu_plugin_container.cc:15: , y_(0)
On 2010/01/29 04:06:45, greggman wrote:
> comma on right (unless this is webkit code) I'm guessing from the copyright
it's
> not.

Done.

http://codereview.chromium.org/558035/diff/1/6#newcode68
webkit/glue/plugins/mac_gpu_plugin_container.cc:68:
io_surface_support->CGLTexImageIOSurface2D(context,
On 2010/01/29 04:06:45, greggman wrote:
> hmmm, It sounds like the GLES2 code needs to hide the fact that there is a
> texture bound.  Yea? I guess I should add that to my list. Otherwise the user
> can query the currently bound texture, get that ID and manipulation it.
> 
> WebGL needs that as a conformance test. That at init time it appears no
textures
> or render buffers are bound.

Yes, the command buffer service code needs to hide the fact that a hidden fbo is
bound. I did part of the work in this CL (adding fbo_ and bound_fbo_ to
GLES2DecoderImpl) but forgot to instrument glBindFramebuffer. The WebGL
implementation already tracks this in
src/third_party/WebKit/WebCore/html/canvas/WebGLRenderingContext.cpp, so we can
pull code from there.

This particular texture exists in the browser process, not the GPU process. From
that standpoint there is no issue.

http://codereview.chromium.org/558035/diff/1/6#newcode81
webkit/glue/plugins/mac_gpu_plugin_container.cc:81: glBindTexture(target,
texture_);
On 2010/01/29 04:06:45, greggman wrote:
> Does this state need to be saved? I'm guessing the idea is that this is a
> separate but GL context from the one the client code is using, they just share
> resources?

No state needs to be saved. This OpenGL context not only does not share
resources with the client code, it is in a completely different process.

http://codereview.chromium.org/558035/diff/1/9
File webkit/glue/plugins/mac_gpu_plugin_container_manager.cc (right):

http://codereview.chromium.org/558035/diff/1/9#newcode16
webkit/glue/plugins/mac_gpu_plugin_container_manager.cc:16:
MacGPUPluginContainerManager::AllocateFakePluginWindowHandle() {
On 2010/01/29 18:59:10, apatrick wrote:
> 4 char indent

Per earlier comment, I don't think it's necessary.

http://codereview.chromium.org/558035/diff/1/9#newcode51
webkit/glue/plugins/mac_gpu_plugin_container_manager.cc:51: glClearColor(0, 0,
0, 0);
On 2010/01/29 04:06:45, greggman wrote:
> do these states need to be saved and restored?

No. This GL context is in a different process than the client code.

http://codereview.chromium.org/558035/diff/1/10
File webkit/glue/plugins/mac_gpu_plugin_container_manager.h (right):

http://codereview.chromium.org/558035/diff/1/10#newcode59
webkit/glue/plugins/mac_gpu_plugin_container_manager.h:59: typedef
std::map<gfx::PluginWindowHandle, MacGPUPluginContainer*>
PluginWindowToContainerMap;
On 2010/01/29 04:06:45, greggman wrote:
> 80 chars? or is this webkit code? I'm guessing from the copyright it's not

Done.

http://codereview.chromium.org/558035/diff/1/11
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/558035/diff/1/11#newcode410
webkit/glue/webplugin_impl.cc:410: // if (window) DCHECK(!window_);
On 2010/01/29 21:02:18, stuartmorgan wrote:
> Remove?

I put that there because it's more clear than the DCHECK logic. I've clarified
the comment but left it in.

http://codereview.chromium.org/558035/diff/1/13
File webkit/tools/pepper_test_plugin/plugin_object.cc (right):

http://codereview.chromium.org/558035/diff/1/13#newcode311
webkit/tools/pepper_test_plugin/plugin_object.cc:311: // TODO(kbr): put back in
audio portion of test
On 2010/01/29 18:59:10, apatrick wrote:
> #if !defined(OS_MACOSX)

This presents problems. I don't think OS_MACOSX is defined in this code -- it is
defined for Chromium code, but not sample code which uses Pepper. I can add in
#ifndef __APPLE__.

http://codereview.chromium.org/558035/diff/1/13#newcode351
webkit/tools/pepper_test_plugin/plugin_object.cc:351: // TODO(kbr): put back in
audio portion of test
On 2010/01/29 18:59:10, apatrick wrote:
> #if !defined(OS_MACOSX)

Done as #ifndef __APPLE__ because I don't think OS_MACOSX is defined in this
code.

http://codereview.chromium.org/558035/diff/1/13#newcode394
webkit/tools/pepper_test_plugin/plugin_object.cc:394: // TODO(kbr): put back in
audio portion of test
On 2010/01/29 18:59:10, apatrick wrote:
> #if !defined(OS_MACOSX)

Done as #ifndef __APPLE__ because I don't think OS_MACOSX is defined in this
code.

http://codereview.chromium.org/558035/diff/1/14
File webkit/tools/pepper_test_plugin/test_page.html (right):

http://codereview.chromium.org/558035/diff/1/14#newcode32
webkit/tools/pepper_test_plugin/test_page.html:32: <object id="plugin"
type="pepper-application/x-pepper-test-plugin" width="400" height="400"
dimensions="3" />
On 2010/01/29 18:59:10, apatrick wrote:
> I don't think this change needs to be part of the CL.

I discussed it with the NaCl team and they didn't object. It's more important
that we exercise the 3D functionality at this point by default.

http://codereview.chromium.org/558035/diff/1/3
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/558035/diff/1/3#newcode185
webkit/tools/test_shell/mac/test_webview_delegate.mm:185: containing_view);
On 2010/01/29 18:59:10, apatrick wrote:
> isn't containing_view always going to be NULL here?

Yes. I incorporated the modified variable definition to match the previous code
structure, in particular in case it needs to be changed for the unit tests.

[apatrick_chromium, 2010-01-30 00:21:58]

http://codereview.chromium.org/558035/diff/1/23
File gpu/command_buffer/client/esTransform.h (right):

http://codereview.chromium.org/558035/diff/1/23#newcode2
gpu/command_buffer/client/esTransform.h:2: // Book:      OpenGL(R) ES 2.0
Programming Guide
Well so long as you move / delete it soon!

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode1200
gpu/command_buffer/service/gles2_cmd_decoder.cc:1200: static IOSurfaceSupport*
io_surface_support = NULL;
Is there any possibility of the function being called from multiple threads and
a race condition regarding calling Initialize?

http://codereview.chromium.org/558035/diff/1/13
File webkit/tools/pepper_test_plugin/plugin_object.cc (right):

http://codereview.chromium.org/558035/diff/1/13#newcode311
webkit/tools/pepper_test_plugin/plugin_object.cc:311: // TODO(kbr): put back in
audio portion of test
I think it will work. This file includes other files from base. If you #include
<base/build_config.h> it'll be defined.

[greggman, 2010-01-30 06:50:43]

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
On 2010/01/29 23:34:12, kbr wrote:
> When compiling the Pepper Test Plugin on Mac OS X the linker complained about
> the missing symbol gpu::CommandHeader::kMaxSize. Apparently this toolchain

I'm kind of lost. The code is full of static consts in structures. There are
hundreds of them in the command definitions.  See lines 181, 182, 209, 210,
etc..

Maybe the bug is something else?

How about moving it outside the structure and renaming it kCommandMaxEntries if
that solves the issue.

> doesn't like static constants in structs / classes.
>

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode1088
gpu/command_buffer/service/gles2_cmd_decoder.cc:1088: CGLPixelFormatAttribute
attribs[] = {
On 2010/01/29 23:34:12, kbr wrote:
> On 2010/01/29 04:06:45, greggman wrote:
> > can this be static?
> > 
> 
> I don't see a need for it to be.
> 

I guess I don't understand. As it is CPU code will be generated to create this
attribs array at runtime everytime this code is executed. If you put "static" in
front of it then some static data will be created at compile time and no code
will be generated.  So I guess, why not put static in front of it?

[Ken Russell (switch to Gerrit), 2010-02-02 02:13:17]

Addressed remaining review feedback.

Fixed build errors on Windows and Linux. Required refactoring of
pepper_test_plugin.gyp and splitting of INDEPENDENT_PLUGIN #define. Tested 3D
support in Pepper Test plugin on Windows; wasn't able to get this to build on
Linux so set use_3d=0 in pepper_test_plugin.gyp for now.

Appreciate prompt re-reviews and, if warranted, LGTMs.

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
On 2010/01/30 06:50:44, greggman wrote:
> On 2010/01/29 23:34:12, kbr wrote:
> > When compiling the Pepper Test Plugin on Mac OS X the linker complained
about
> > the missing symbol gpu::CommandHeader::kMaxSize. Apparently this toolchain
> 
> I'm kind of lost. The code is full of static consts in structures. There are
> hundreds of them in the command definitions.  See lines 181, 182, 209, 210,
> etc..
> 
> Maybe the bug is something else?
> 
> How about moving it outside the structure and renaming it kCommandMaxEntries
if
> that solves the issue.
> 
> > doesn't like static constants in structs / classes.

I don't understand it either but it has something to do with this being the sole
static const dependency between the pepper test plugin and this code. I've moved
this to a top-level const int32 kCommandHeaderMaxSize which works.

http://codereview.chromium.org/558035/diff/1/18
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/558035/diff/1/18#newcode1088
gpu/command_buffer/service/gles2_cmd_decoder.cc:1088: CGLPixelFormatAttribute
attribs[] = {
On 2010/01/30 06:50:44, greggman wrote:
> On 2010/01/29 23:34:12, kbr wrote:
> > On 2010/01/29 04:06:45, greggman wrote:
> > > can this be static?
> > > 
> > 
> > I don't see a need for it to be.
> > 
> 
> I guess I don't understand. As it is CPU code will be generated to create this
> attribs array at runtime everytime this code is executed. If you put "static"
in
> front of it then some static data will be created at compile time and no code
> will be generated.  So I guess, why not put static in front of it?

I thought you were talking about hoisting this out of the function. I've added
"static const" qualifiers for this array.

http://codereview.chromium.org/558035/diff/1/18#newcode1200
gpu/command_buffer/service/gles2_cmd_decoder.cc:1200: static IOSurfaceSupport*
io_surface_support = NULL;
On 2010/01/30 00:21:58, apatrick_chromium wrote:
> Is there any possibility of the function being called from multiple threads
and
> a race condition regarding calling Initialize?

This has been removed in the current version of the code.

http://codereview.chromium.org/558035/diff/1/13
File webkit/tools/pepper_test_plugin/plugin_object.cc (right):

http://codereview.chromium.org/558035/diff/1/13#newcode311
webkit/tools/pepper_test_plugin/plugin_object.cc:311: // TODO(kbr): put back in
audio portion of test
On 2010/01/30 00:21:58, apatrick_chromium wrote:
> I think it will work. This file includes other files from base. If you
#include
> <base/build_config.h> it'll be defined.

OK, I added #include "build/build_config.h", verified that OS_MACOSX is defined
in this code, and changed the #ifdefs.

[apatrick, 2010-02-02 02:37:46]

lgtm

[greggman, 2010-02-02 02:50:53]

LGTM

[piman, 2010-02-02 03:29:30]

http://codereview.chromium.org/558035/diff/1/12
File webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp (right):

http://codereview.chromium.org/558035/diff/1/12#newcode13
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:13:
'../../../gpu/gpu.gyp:pgl',
That will break linux x64 I think.

http://codereview.chromium.org/558035/diff/1/12#newcode68
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:68: ['OS!="mac"', {
How about simply removing this condition ?

[alokp, 2010-02-02 03:42:19]

http://codereview.chromium.org/558035/diff/13022/12051
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/13022/12051#newcode38
gpu/command_buffer/common/cmd_buffer_common.h:38: const int32
kCommandHeaderMaxSize = (1 << 21) - 1;
I have fixed many such issues including this one for linux. I have submitted one
CL today and will be submitting 3 more tomorrow. You should wait resolve before
you submit.

[piman, 2010-02-02 03:59:33]

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
On 2010/02/02 02:13:18, kbr wrote:
> On 2010/01/30 06:50:44, greggman wrote:
> > On 2010/01/29 23:34:12, kbr wrote:
> > > When compiling the Pepper Test Plugin on Mac OS X the linker complained
> about
> > > the missing symbol gpu::CommandHeader::kMaxSize. Apparently this toolchain
> > 
> > I'm kind of lost. The code is full of static consts in structures. There are
> > hundreds of them in the command definitions.  See lines 181, 182, 209, 210,
> > etc..
> > 
> > Maybe the bug is something else?
> > 
> > How about moving it outside the structure and renaming it kCommandMaxEntries
> if
> > that solves the issue.
> > 
> > > doesn't like static constants in structs / classes.
> 
> I don't understand it either but it has something to do with this being the
sole
> static const dependency between the pepper test plugin and this code. I've
moved
> this to a top-level const int32 kCommandHeaderMaxSize which works.
> 

They have to be declared in the .cc (with no initializer)
It's what the spec mandates (makes sense, you need to provide the location for
the storage, if for example you want to take the address of it).
Of course, MVSC doesn't like that, so it usually needs to be #ifdef-ed.

See e.g.
http://publib.boulder.ibm.com/infocenter/comphelp/v8v101/index.jsp?topic=/com...

[alokp, 2010-02-02 04:00:08]

http://codereview.chromium.org/558035/diff/13022/12053
File gpu/command_buffer/client/esTransform.c (right):

http://codereview.chromium.org/558035/diff/13022/12053#newcode19
gpu/command_buffer/client/esTransform.c:19: #include <math.h>
ditto

http://codereview.chromium.org/558035/diff/13022/12054
File gpu/command_buffer/client/esTransform.h (right):

http://codereview.chromium.org/558035/diff/13022/12054#newcode1
gpu/command_buffer/client/esTransform.h:1: //
why do yo need to move this file here. I was not allowed to submit it under gpu
by Legal. can't you just it from third_party?

[alokp, 2010-02-02 04:06:27]

http://codereview.chromium.org/558035/diff/1/21
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/1/21#newcode41
gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32 kMaxSize;
piman: Actually I tried what you suggested today - initialized in class
declaration and defined in .cc without initialization. msvc did not complain.
trybots are happy.

So I do not think there is any need to move it outside the class declaration. In
fact you could simply revert this change. I have already fixed it.

[piman, 2010-02-02 04:12:36]

On Mon, Feb 1, 2010 at 8:06 PM, <alokp@chromium.org> wrote:

>
> http://codereview.chromium.org/558035/diff/1/21
> File gpu/command_buffer/common/cmd_buffer_common.h (right):
>
> http://codereview.chromium.org/558035/diff/1/21#newcode41
> gpu/command_buffer/common/cmd_buffer_common.h:41: static const int32
> kMaxSize;
> piman: Actually I tried what you suggested today - initialized in class
> declaration and defined in .cc without initialization. msvc did not
> complain. trybots are happy.
>

That's great to know. I had only tried on older version of MSVC, so I assume
they fixed this.


>
> So I do not think there is any need to move it outside the class
> declaration. In fact you could simply revert this change. I have already
> fixed it.
>
>
> http://codereview.chromium.org/558035
>

[alokp, 2010-02-02 04:23:58]

I was going crazy today about why gcc was complaining about only a few static
variables and not others. I found out that it complains about only those
variables for whom you need to access the address (pointer or reference). The
STL algorithms such as std::min etc usually take const T& as argument which
causes gcc to complain about missing storage.

We may only need to define only a few variables to make gcc happy, but I think
it is a good practice to define all static variables anyway.

[stuartmorgan, 2010-02-02 15:45:13]

http://codereview.chromium.org/558035/diff/13022/12063
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/13022/12063#newcode65
chrome/browser/renderer_host/render_widget_host_view_mac.mm:65:
scoped_ptr<RenderWidgetHostViewMac> renderWidgetHostView_;
Why scoped_ptr? Having deallocation of this object delete the
rendereWidgetHostViewMac that owns it doesn't seem right.

http://codereview.chromium.org/558035/diff/13022/12063#newcode524
chrome/browser/renderer_host/render_widget_host_view_mac.mm:524:
gpu_plugin_layer_ = gpu_layer;
This is leaked. scoped_nsobject?

http://codereview.chromium.org/558035/diff/13022/12070
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/558035/diff/13022/12070#newcode295
chrome/common/plugin_messages_internal.h:295: gfx::PluginWindowHandle /* window
*/)
Does this need to be SYNC, or is set_noblock sufficient?

http://codereview.chromium.org/558035/diff/13022/12073
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/558035/diff/13022/12073#newcode1796
chrome/common/render_messages_internal.h:1796: gfx::PluginWindowHandle /* id */)
Here also, is the SYNC necessary?

[Ken Russell (switch to Gerrit), 2010-02-02 23:06:36]

Addressed Mac-specific and Pepper test plugin-related review feedback.

Undid most of the changes to the Pepper test plugin's gyp files and source code,
putting back in the INDEPENDENT_PLUGIN #define. Factored out the external
dependencies into a variable in pepper_test_plugin.gyp. Removed the
esTransform.[ch] files.

Built and tested the Pepper 3D test plugin on Mac, Windows and Linux. The
Windows trybot failure is due to an unrelated flaky test.

http://codereview.chromium.org/558035/diff/1/12
File webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp (right):

http://codereview.chromium.org/558035/diff/1/12#newcode13
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:13:
'../../../gpu/gpu.gyp:pgl',
On 2010/02/02 03:29:30, piman wrote:
> That will break linux x64 I think.

This has been fixed in the current code.

http://codereview.chromium.org/558035/diff/1/12#newcode68
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:68: ['OS!="mac"', {
On 2010/02/02 03:29:30, piman wrote:
> How about simply removing this condition ?

It's been removed in the current code.

http://codereview.chromium.org/558035/diff/13022/12063
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/13022/12063#newcode65
chrome/browser/renderer_host/render_widget_host_view_mac.mm:65:
scoped_ptr<RenderWidgetHostViewMac> renderWidgetHostView_;
On 2010/02/02 15:45:14, stuartmorgan wrote:
> Why scoped_ptr? Having deallocation of this object delete the
> rendereWidgetHostViewMac that owns it doesn't seem right.

Thanks, you're right, this should not have been a scoped_ptr. Fixed and
re-tested.

http://codereview.chromium.org/558035/diff/13022/12063#newcode524
chrome/browser/renderer_host/render_widget_host_view_mac.mm:524:
gpu_plugin_layer_ = gpu_layer;
On 2010/02/02 15:45:14, stuartmorgan wrote:
> This is leaked. scoped_nsobject?

AppKit manages the storage for this layer because we add it to the layer
hierarchy for the RenderWidgetHostViewCocoa. I've added a clarifying comment to
render_widget_host_view_mac.h.

http://codereview.chromium.org/558035/diff/13022/12070
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/558035/diff/13022/12070#newcode295
chrome/common/plugin_messages_internal.h:295: gfx::PluginWindowHandle /* window
*/)
On 2010/02/02 15:45:14, stuartmorgan wrote:
> Does this need to be SYNC, or is set_noblock sufficient?

You're right, it doesn't need to be SYNC. Fixed.

http://codereview.chromium.org/558035/diff/13022/12073
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/558035/diff/13022/12073#newcode1796
chrome/common/render_messages_internal.h:1796: gfx::PluginWindowHandle /* id */)
On 2010/02/02 15:45:14, stuartmorgan wrote:
> Here also, is the SYNC necessary?

Again, you're right. Fixed.

http://codereview.chromium.org/558035/diff/13022/12054
File gpu/command_buffer/client/esTransform.h (right):

http://codereview.chromium.org/558035/diff/13022/12054#newcode1
gpu/command_buffer/client/esTransform.h:1: //
On 2010/02/02 04:00:09, alokp wrote:
> why do yo need to move this file here. I was not allowed to submit it under
gpu
> by Legal. can't you just it from third_party?

It was only intended to be temporary so animation support in the GPU process
could be tested. It has been removed in the current version of the code and the
creation of the necessary rotation matrix done by hand in gles2_demo_cc.cc.

http://codereview.chromium.org/558035/diff/13022/12051
File gpu/command_buffer/common/cmd_buffer_common.h (right):

http://codereview.chromium.org/558035/diff/13022/12051#newcode38
gpu/command_buffer/common/cmd_buffer_common.h:38: const int32
kCommandHeaderMaxSize = (1 << 21) - 1;
On 2010/02/02 03:42:19, alokp wrote:
> I have fixed many such issues including this one for linux. I have submitted
one
> CL today and will be submitting 3 more tomorrow. You should wait resolve
before
> you submit.

Per the discussion I have put this back into the class and added a conditional
declaration in the .cc file because MSVC reported a duplicate symbol.

[piman, 2010-02-02 23:16:04]

I looked at the gyp changes and LGTM for that. Just one thing if you want to go
through another round of trybots, or clean up afterwards.

http://codereview.chromium.org/558035/diff/12092/13035
File webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp (right):

http://codereview.chromium.org/558035/diff/12092/13035#newcode73
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:73:
'<@(external_dependencies)',
just fyi, this is included in all cases except linux without -fPIC, so you don't
need to include it in the mac and win sections.

[stuartmorgan, 2010-02-02 23:24:56]

http://codereview.chromium.org/558035/diff/13022/12063
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/13022/12063#newcode524
chrome/browser/renderer_host/render_widget_host_view_mac.mm:524:
gpu_plugin_layer_ = gpu_layer;
On 2010/02/02 23:06:36, kbr wrote:
> On 2010/02/02 15:45:14, stuartmorgan wrote:
> > This is leaked. scoped_nsobject?
> 
> AppKit manages the storage for this layer because we add it to the layer
> hierarchy for the RenderWidgetHostViewCocoa.

I don't follow. It's a retain counted object; you alloc it, so you are
responsible for one retain, and you add it to root_layer, which will add one or
more retains that it manages. When the root layer is destroyed, the layer you
created still has the retain count you are responsible for, and it is leaked.

You can either release it immediately after adding it as a sublayer, or you can
use scoped_nsobject. Since you make calls directly to the reference, the latter
would be preferable since it eliminates the possibility that you will
accidentally use a dangling pointer if the root_layer goes away without you
expecting it somehow.

[alokp, 2010-02-02 23:26:52]

http://codereview.chromium.org/558035/diff/12092/13044
File gpu/command_buffer/common/cmd_buffer_common.cc (right):

http://codereview.chromium.org/558035/diff/12092/13044#newcode11
gpu/command_buffer/common/cmd_buffer_common.cc:11: #if !defined(OS_WIN)
This is weird. I submitted two CLs today with something similar without any
problem. See http://codereview.chromium.org/560002/show

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

Addressed review feedback on CALayer leak and pepper_test_plugin.gyp changes.

http://codereview.chromium.org/558035/diff/13022/12063
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/13022/12063#newcode524
chrome/browser/renderer_host/render_widget_host_view_mac.mm:524:
gpu_plugin_layer_ = gpu_layer;
On 2010/02/02 23:24:56, stuartmorgan wrote:
> On 2010/02/02 23:06:36, kbr wrote:
> > On 2010/02/02 15:45:14, stuartmorgan wrote:
> > > This is leaked. scoped_nsobject?
> > 
> > AppKit manages the storage for this layer because we add it to the layer
> > hierarchy for the RenderWidgetHostViewCocoa.
> 
> I don't follow. It's a retain counted object; you alloc it, so you are
> responsible for one retain, and you add it to root_layer, which will add one
or
> more retains that it manages. When the root layer is destroyed, the layer you
> created still has the retain count you are responsible for, and it is leaked.
> 
> You can either release it immediately after adding it as a sublayer, or you
can
> use scoped_nsobject. Since you make calls directly to the reference, the
latter
> would be preferable since it eliminates the possibility that you will
> accidentally use a dangling pointer if the root_layer goes away without you
> expecting it somehow.

I see now. One of the samples I was looking at had a memory leak which was
confusing. I've changed gpu_plugin_layer_ to a scoped_nsobject and retested.

http://codereview.chromium.org/558035/diff/12092/13035
File webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp (right):

http://codereview.chromium.org/558035/diff/12092/13035#newcode73
webkit/tools/pepper_test_plugin/pepper_test_plugin.gyp:73:
'<@(external_dependencies)',
On 2010/02/02 23:16:04, piman wrote:
> just fyi, this is included in all cases except linux without -fPIC, so you
don't
> need to include it in the mac and win sections.

I see now. I've cleaned this up in the current code.

[stuartmorgan, 2010-02-03 01:01:24]

LGTM; just a tiny comment fix.

http://codereview.chromium.org/558035/diff/18032/15106
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/18032/15106#newcode65
chrome/browser/renderer_host/render_widget_host_view_mac.mm:65:
RenderWidgetHostViewMac* renderWidgetHostView_;
I believe the style is to mark this with a "  // weak" at the end of the line.

[piman, 2010-02-03 01:07:30]

LGTM

[Ken Russell (switch to Gerrit), 2010-02-03 01:27:42]

http://codereview.chromium.org/558035/diff/18032/15106
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558035/diff/18032/15106#newcode65
chrome/browser/renderer_host/render_widget_host_view_mac.mm:65:
RenderWidgetHostViewMac* renderWidgetHostView_;
On 2010/02/03 01:01:25, stuartmorgan wrote:
> I believe the style is to mark this with a "  // weak" at the end of the line.

Thanks for review. Fixed. Submitting.