Added Pepper 3D render context that instantiates the GPU plugin....

Added Pepper 3D device that instantiates the GPU plugin and sends GLES2 commands to it via a command buffer.

Added API for managing buffers to Pepper 3D device.

Removed DCHECK from WebPluginImpl::SetWindow that checks against a windowless plugin being given a window handle. Please check this! Now an initially windowless plugin instance gets a handle when it requests a Pepper 3D context. Perhaps the window handle should be concealed from the underlying plugin isntance.

Removed enable_gpu gyp variable and C macro. GPU code is always built on windows but not mac or linux. It is enabled at runtime with the --enable-gpu-plugin switch.

Redesigned CommandBuffer interface so it exposes shared memory through a Buffer. This was necessary because Pepper has no notion of shared memory handles. The Buffer exposes the shared memory as both a handle (through base::SharedMemory) and the mapped address and size.

Refactored CommandBufferEngine so mapped shared memory addresses and sizes are returned with a single call rather than two separate calls.
	
Added 3D demo to pepper test plugin.

TEST=try servers
BUG=none


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

[brettw, 2009-11-10 00:20:22]

The meat of this looks OK so far. A few nits:

http://codereview.chromium.org/367002/diff/1/17
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/1/17#newcode24
Line 24: #define AS_WIDE_STRING_LITERAL2(x) L ## x
Why do you need this? Why not just prefix a string with L? For use in production
(non-test) code, I usually recommend ASCIIToWide("foo") instead.

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

http://codereview.chromium.org/367002/diff/1/19#newcode32
Line 32: using std::string;
We don't normally do this, and write std::string, std::vector everywhere.

[apatrick, 2009-11-10 00:24:41]

http://codereview.chromium.org/367002/diff/1/17
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/1/17#newcode24
Line 24: #define AS_WIDE_STRING_LITERAL2(x) L ## x
It's because I need the literal as a regular string in one place and as a wide
string in another. I'll look at ASCIIToWide. That's actually doing a runtime
conversion though right? I don't need that.

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

http://codereview.chromium.org/367002/diff/1/19#newcode32
Line 32: using std::string;
Will fix.

[brettw, 2009-11-10 01:16:37]

On 2009/11/10 00:24:41, apatrick wrote:
> http://codereview.chromium.org/367002/diff/1/17
> File base/string_util.h (right):
> 
> http://codereview.chromium.org/367002/diff/1/17#newcode24
> Line 24: #define AS_WIDE_STRING_LITERAL2(x) L ## x
> It's because I need the literal as a regular string in one place and as a wide
> string in another. I'll look at ASCIIToWide. That's actually doing a runtime
> conversion though right? I don't need that.

I don't get how the current macro works. I don't see where the declaration of
TO_WIDE_STRING2 is. It seems like you would either have a macro that defines the
string and L MY_STRING would work but you'd get duplication, or you have a
constant and L kFooBar wouldn't work at all.

[apatrick, 2009-11-10 01:20:41]

http://codereview.chromium.org/367002/diff/1/17
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/1/17#newcode24
Line 24: #define AS_WIDE_STRING_LITERAL2(x) L ## x
Oh that's just a mistake. I must have uploaded before fixing that. It should be:

#define AS_WIDE_STRING_LITERAL(x) AS_WIDE_STRING_LITERAL2(x)

And the reason for that is so that macros can be used in place of x. It's
similar to the problem of writing a macro to concatenate two strings.

I'll just change it. No point in spending time arguing about it =).

[greggman, 2009-12-17 20:38:08]

LGTM from my part. I had one comment but it's not really related to this CL.

http://codereview.chromium.org/367002/diff/18209/18242
File gpu/command_buffer/client/id_allocator.h (right):

http://codereview.chromium.org/367002/diff/18209/18242#newcode47
gpu/command_buffer/client/id_allocator.h:47: class IdAllocator {
Currently I need Id = 0 to mean no-id. Should this class handle that or is it
best the way it is? I my case I allocate the first id when I init
GLES2CmdDecoder so that I get id 0 where as theoretically there is nothing in
this class that guarantees that the first Id I get will be 0 and therefore my
work around works.  Thoughts?

[brettw, 2009-12-17 20:49:25]

Random string suggestion first, looking at the rest now

http://codereview.chromium.org/367002/diff/18209/18210
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/367002/diff/18209/18210#newcode31
webkit/glue/plugins/plugin_list.h:31: #define kGPUPluginMimeType
"application/vnd.google.chrome.gpu-plugin"
I think this should be a "extern const char[]" instead, which will give you
typechecking and no macro evilness. If you need it to be a wstring, just call
ASCIIToWide. The places you use this take a wstring and ASCIIToWide is basically
as fast as calling wstring's constructor with a wchar_t initializer.

http://codereview.chromium.org/367002/diff/18209/18218
File webkit/tools/pepper_test_plugin/command_buffer_pepper.cc (right):

http://codereview.chromium.org/367002/diff/18209/18218#newcode1
webkit/tools/pepper_test_plugin/command_buffer_pepper.cc:1: // Copyright (c)
2006-2008 The Chromium Authors. All rights reserved.
All of the copyright notices should just say "2009" instead of a range.

[jam, 2009-12-18 21:29:21]

http://codereview.chromium.org/367002/diff/18209/18213
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/18209/18213#newcode25
base/string_util.h:25: #define AS_WIDE_STRING_LITERAL2(x) L ## x
I'm not sure this should go into string_util (it's already a huge file).

Why not just in the one place where you need this, do
_T(kGPUPluginMimeType)
?

http://codereview.chromium.org/367002/diff/18209/18253
File chrome/plugin/command_buffer_stub.h (left):

http://codereview.chromium.org/367002/diff/18209/18253#oldcode8
chrome/plugin/command_buffer_stub.h:8: #if defined(ENABLE_GPU)
nit: what's wrong with ENABLE_GPU?  it's nice so that when we want to enable
this on another platform, we don't have to hunt down manually for the OS_WIN and
change it to OS_WIN or ...  It also allows us to disable the gpu code easily
(i.e. for stable channel release builds if we want to).

http://codereview.chromium.org/367002/diff/18209/18215
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/367002/diff/18209/18215#newcode150
chrome/renderer/webplugin_delegate_pepper.cc:150: // Forward the new geometry to
nit comment

http://codereview.chromium.org/367002/diff/18209/18263
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/367002/diff/18209/18263#newcode1158
chrome/renderer/webplugin_delegate_proxy.cc:1158: 
nit: extra line

http://codereview.chromium.org/367002/diff/18209/18212
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/367002/diff/18209/18212#newcode191
webkit/glue/plugins/plugin_list.cc:191: LoadPlugin(FilePath(kGPUPluginFileName),
&new_plugins);
it breaks the layering when the webkit glue code looks at the command line
parameters for the current process.  This should be done in chrome code, and it
can register this plugin using RegisterInternalPlugin.  Or, you can look at what
we do for the default_plugin, where we call
webkit_glue::IsDefaultPluginEnabled() before adding it to the list.  that
function is implemented in the chrome code.

http://codereview.chromium.org/367002/diff/18209/18259
File webkit/glue/webplugin_impl.cc (left):

http://codereview.chromium.org/367002/diff/18209/18259#oldcode401
webkit/glue/webplugin_impl.cc:401: DCHECK(!windowless_);  // Make sure not
called twice.
yeah I don't think we want to do this, since we sometimes  assume that if
window_ is set, this is a windowed plugin (see the comment about this function
in webplugin.h).  I don't see where SetWindow is called now, or what's
triggering it indirectly.

[apatrick, 2009-12-18 23:38:56]

http://codereview.chromium.org/367002/diff/18209/18213
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/18209/18213#newcode25
base/string_util.h:25: #define AS_WIDE_STRING_LITERAL2(x) L ## x
The string was only being used in two places and with two different types. It
wasn't clear where to put the string where it could be shared between browser
and renderer. So I just got rid of the constant.

http://codereview.chromium.org/367002/diff/18209/18253
File chrome/plugin/command_buffer_stub.h (left):

http://codereview.chromium.org/367002/diff/18209/18253#oldcode8
chrome/plugin/command_buffer_stub.h:8: #if defined(ENABLE_GPU)
On 2009/12/18 21:29:21, John Abd-El-Malek wrote:
> nit: what's wrong with ENABLE_GPU?  it's nice so that when we want to enable
> this on another platform, we don't have to hunt down manually for the OS_WIN
and
> change it to OS_WIN or ...  It also allows us to disable the gpu code easily
> (i.e. for stable channel release builds if we want to).

Done.

http://codereview.chromium.org/367002/diff/18209/18215
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/367002/diff/18209/18215#newcode150
chrome/renderer/webplugin_delegate_pepper.cc:150: // Forward the new geometry to
On 2009/12/18 21:29:21, John Abd-El-Malek wrote:
> nit comment

Done.

http://codereview.chromium.org/367002/diff/18209/18263
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/367002/diff/18209/18263#newcode1158
chrome/renderer/webplugin_delegate_proxy.cc:1158: 
On 2009/12/18 21:29:21, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/367002/diff/18209/18212
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/367002/diff/18209/18212#newcode191
webkit/glue/plugins/plugin_list.cc:191: LoadPlugin(FilePath(kGPUPluginFileName),
&new_plugins);
On 2009/12/18 21:29:21, John Abd-El-Malek wrote:
> it breaks the layering when the webkit glue code looks at the command line
> parameters for the current process.  This should be done in chrome code, and
it
> can register this plugin using RegisterInternalPlugin.  Or, you can look at
what
> we do for the default_plugin, where we call
> webkit_glue::IsDefaultPluginEnabled() before adding it to the list.  that
> function is implemented in the chrome code.

Done.

http://codereview.chromium.org/367002/diff/18209/18210
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/367002/diff/18209/18210#newcode31
webkit/glue/plugins/plugin_list.h:31: #define kGPUPluginMimeType
"application/vnd.google.chrome.gpu-plugin"
On 2009/12/17 20:49:25, brettw wrote:
> I think this should be a "extern const char[]" instead, which will give you
> typechecking and no macro evilness. If you need it to be a wstring, just call
> ASCIIToWide. The places you use this take a wstring and ASCIIToWide is
basically
> as fast as calling wstring's constructor with a wchar_t initializer.

Done.

http://codereview.chromium.org/367002/diff/18209/18259
File webkit/glue/webplugin_impl.cc (left):

http://codereview.chromium.org/367002/diff/18209/18259#oldcode401
webkit/glue/webplugin_impl.cc:401: DCHECK(!windowless_);  // Make sure not
called twice.
It's because when the GPU plugin is active, there are two WebPluginDelegates:
the WebPluginDelegatePepper and the WebPluginDelegateProxy for the GPU plugin.
They both point at the same WebPluginImpl. So when they do their UpdateGeometry,
they both call SetWindow on the same WebPluginImpl. I'm not sure whether I
should be doing this. It seems it might help for things like getting events from
the GPU plugin to the underlying plugin but I haven't really looked into that
yet. I also wasn't sure whether WebPluginDelegatePepper would be able to create
a second WebPluginImpl for the GPU plugin. I hope that makes sense.

http://codereview.chromium.org/367002/diff/18209/18218
File webkit/tools/pepper_test_plugin/command_buffer_pepper.cc (right):

http://codereview.chromium.org/367002/diff/18209/18218#newcode1
webkit/tools/pepper_test_plugin/command_buffer_pepper.cc:1: // Copyright (c)
2006-2008 The Chromium Authors. All rights reserved.
On 2009/12/17 20:49:25, brettw wrote:
> All of the copyright notices should just say "2009" instead of a range.

Done.

[jam, 2009-12-21 18:55:11]

http://codereview.chromium.org/367002/diff/18209/18213
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/18209/18213#newcode25
base/string_util.h:25: #define AS_WIDE_STRING_LITERAL2(x) L ## x
On 2009/12/18 23:38:56, apatrick wrote:
> The string was only being used in two places and with two different types. It
> wasn't clear where to put the string where it could be shared between browser
> and renderer. So I just got rid of the constant.

If the string is used in more than one place, it should be in a constant.  For
plugin stuff, you can put it in plugin_constants_win.h for now.

http://codereview.chromium.org/367002/diff/18209/18259
File webkit/glue/webplugin_impl.cc (left):

http://codereview.chromium.org/367002/diff/18209/18259#oldcode401
webkit/glue/webplugin_impl.cc:401: DCHECK(!windowless_);  // Make sure not
called twice.
The reason I objected ot this dcheck is that for non-pepper plugins, it's useful
and it does help catch bugs.  So perhaps it should fire only if it's a non
pepper plugin, or the caller doesn't call it for pepper plugins etc

On 2009/12/18 23:38:56, apatrick wrote:
> It's because when the GPU plugin is active, there are two WebPluginDelegates:
> the WebPluginDelegatePepper and the WebPluginDelegateProxy for the GPU plugin.
> They both point at the same WebPluginImpl. So when they do their
UpdateGeometry,
> they both call SetWindow on the same WebPluginImpl. I'm not sure whether I
> should be doing this. It seems it might help for things like getting events
from
> the GPU plugin to the underlying plugin but I haven't really looked into that
> yet. I also wasn't sure whether WebPluginDelegatePepper would be able to
create
> a second WebPluginImpl for the GPU plugin.

It depends, will it want to intercept any of the calls from the GPU
WebPluginDelegate to WebPluginImpl?  Off the top of my head, I'm not sure why it
would, so it's probably better not to add this extra complexity now, and add it
only if it's needed.

> I hope that makes sense.

[brettw, 2009-12-22 18:49:49]

LGTM from me with these minor changes.

http://codereview.chromium.org/367002/diff/18209/18231
File gpu/command_buffer/service/command_buffer_service.cc (right):

http://codereview.chromium.org/367002/diff/18209/18231#newcode8
gpu/command_buffer/service/command_buffer_service.cc:8: #include
"gpu/command_buffer/service/command_buffer_service.h"
Technically, this include should be before any other since it matches the .cc

http://codereview.chromium.org/367002/diff/18209/18223
File webkit/glue/plugins/npapi_extension_thunk.cc (right):

http://codereview.chromium.org/367002/diff/18209/18223#newcode244
webkit/glue/plugins/npapi_extension_thunk.cc:244: Device2DDestroyContext,
It seems like you should initialize the 2D device's function pointers for the
new functions you added to something that just returns failure rather than
leaving them uninitialized.

http://codereview.chromium.org/367002/diff/18373/18437
File chrome/common/gpu_plugin.h (right):

http://codereview.chromium.org/367002/diff/18373/18437#newcode13
chrome/common/gpu_plugin.h:13: }
Don't forget the "// namespace chrome" comment here

[apatrick, 2009-12-22 21:50:47]

http://codereview.chromium.org/367002/diff/18209/18213
File base/string_util.h (right):

http://codereview.chromium.org/367002/diff/18209/18213#newcode25
base/string_util.h:25: #define AS_WIDE_STRING_LITERAL2(x) L ## x
On 2009/12/21 18:55:11, John Abd-El-Malek wrote:
> On 2009/12/18 23:38:56, apatrick wrote:
> > The string was only being used in two places and with two different types.
It
> > wasn't clear where to put the string where it could be shared between
browser
> > and renderer. So I just got rid of the constant.
> 
> If the string is used in more than one place, it should be in a constant.  For
> plugin stuff, you can put it in plugin_constants_win.h for now.

Done.

http://codereview.chromium.org/367002/diff/18209/18231
File gpu/command_buffer/service/command_buffer_service.cc (right):

http://codereview.chromium.org/367002/diff/18209/18231#newcode8
gpu/command_buffer/service/command_buffer_service.cc:8: #include
"gpu/command_buffer/service/command_buffer_service.h"
On 2009/12/22 18:49:50, brettw wrote:
> Technically, this include should be before any other since it matches the .cc

Done.

http://codereview.chromium.org/367002/diff/18209/18223
File webkit/glue/plugins/npapi_extension_thunk.cc (right):

http://codereview.chromium.org/367002/diff/18209/18223#newcode244
webkit/glue/plugins/npapi_extension_thunk.cc:244: Device2DDestroyContext,
On 2009/12/22 18:49:50, brettw wrote:
> It seems like you should initialize the 2D device's function pointers for the
> new functions you added to something that just returns failure rather than
> leaving them uninitialized.

Done.

http://codereview.chromium.org/367002/diff/18209/18259
File webkit/glue/webplugin_impl.cc (left):

http://codereview.chromium.org/367002/diff/18209/18259#oldcode401
webkit/glue/webplugin_impl.cc:401: DCHECK(!windowless_);  // Make sure not
called twice.
The DCHECK is back. I modified the way WebPluginDelegatePepper works so it does
not call WebPluginImpl::SetWindow twice when a 3D device is in use.

http://codereview.chromium.org/367002/diff/18373/18437
File chrome/common/gpu_plugin.h (right):

http://codereview.chromium.org/367002/diff/18373/18437#newcode13
chrome/common/gpu_plugin.h:13: }
On 2009/12/22 18:49:50, brettw wrote:
> Don't forget the "// namespace chrome" comment here

Done.

[jam, 2009-12-22 23:21:34]

lgtm