Add the necessary plumbing mechanisms to ensure proper WebGL support inside the <browser> tag, whic…

Add the necessary plumbing mechanisms to ensure proper WebGL support inside the <browser> tag, which is a separate patch.

Known bugs: Not all aspects of context sharing work properly; in no models would render although the background animated properly.

Requires a separate WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=86504

R=fsamuel@chromium.org,piman@chromium.org,brettw@chromium.org
BUG=None
TEST=compiles

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

[piman, 2012-05-15 20:59:15]

You'll probably want to take out the webkitplatformsupport_impl.h change from
this CL.
For the pepper side, a couple of things need to be moved a bit, but otherwise it
seems fine at a high level

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
File content/common/webkitplatformsupport_impl.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
content/common/webkitplatformsupport_impl.h:43: WebKit::WebView* webView);
NAK, the offscreen contexts are shared between webviews, so we can't have this
(in theory it's possible to create offscreen contexts without a view, and we
want to keep that behavior).

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
ppapi/proxy/ppb_graphics_3d_proxy.cc:175: ppapi::HostResource(),
I don't understand, don't you want to pass the share_context resource here?

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/plugin_delegate.h:175: CommandBufferProxy* share_buffer) =
0;
CommandBufferProxy here is a layering violation. It'd be better to pass the
PlatformContext3D to share with here, and grab the CommandBufferProxy inside of
the implementation

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:225:
EnterResourceNoLock<PPB_Graphics3D_API> enter(share_context, false);
It'd be nicer to do the EnterResource thing in the thunk layer

[(scshunt), 2012-05-16 16:39:43]

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
File content/common/webkitplatformsupport_impl.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
content/common/webkitplatformsupport_impl.h:43: WebKit::WebView* webView);
On 2012/05/15 20:59:17, piman wrote:
> NAK, the offscreen contexts are shared between webviews, so we can't have this
> (in theory it's possible to create offscreen contexts without a view, and we
> want to keep that behavior).

See the WebKit bug thread for some discussion of this. The idea here is that the
WebView (or, specifically, the RenderViewImpl) contains the necessary state to
determine whether or not to request the context, not that the context should be
associated with the WebView. The code will work fine without a WebView.

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
ppapi/proxy/ppb_graphics_3d_proxy.cc:175: ppapi::HostResource(),
On 2012/05/15 20:59:17, piman wrote:
> I don't understand, don't you want to pass the share_context resource here?

I haven't touched this method. Possibly yes; this is actually just a hunk from
fsamuel's patch that I had to copy over since the interface to this message
changed.

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:225:
EnterResourceNoLock<PPB_Graphics3D_API> enter(share_context, false);
On 2012/05/15 20:59:17, piman wrote:
> It'd be nicer to do the EnterResource thing in the thunk layer

Which is the thunk layer?

[Fady Samuel, 2012-05-16 16:43:49]

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
ppapi/proxy/ppb_graphics_3d_proxy.cc:175: ppapi::HostResource(),
On 2012/05/16 16:39:43, scshunt wrote:
> On 2012/05/15 20:59:17, piman wrote:
> > I don't understand, don't you want to pass the share_context resource here?
> 
> I haven't touched this method. Possibly yes; this is actually just a hunk from
> fsamuel's patch that I had to copy over since the interface to this message
> changed.

Yup, this was some stuff I changed when I started working on this plumbing.
There's no reason to do this really, just create a HostResource from PP_Instance
and PP_Resource in CreateProxyResource.

[piman, 2012-05-16 17:12:00]

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
File content/common/webkitplatformsupport_impl.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
content/common/webkitplatformsupport_impl.h:43: WebKit::WebView* webView);
On 2012/05/16 16:39:43, scshunt wrote:
> On 2012/05/15 20:59:17, piman wrote:
> > NAK, the offscreen contexts are shared between webviews, so we can't have
this
> > (in theory it's possible to create offscreen contexts without a view, and we
> > want to keep that behavior).
> 
> See the WebKit bug thread for some discussion of this. The idea here is that
the
> WebView (or, specifically, the RenderViewImpl) contains the necessary state to
> determine whether or not to request the context, not that the context should
be
> associated with the WebView. The code will work fine without a WebView.

This is flawed. See the discussion there.
I don't see why you need this. If you're a guest renderer, all your contexts
need to go through pepper. If you're not, all your contexts need to go directly
to the GPU process. Regardless of the webview.

I really suggest you move the webkit changes out of this CL, otherwise it's
going to hold up the part where we're in agreement.
Besides, you'll need to handle the fact that webkit rolls at a different time
than this CL, so even if we settle on changing the interface, you'll need to
have some temporary code for managing the transition without breaking the build.

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/ppapi/proxy/ppb_graphi...
ppapi/proxy/ppb_graphics_3d_proxy.cc:175: ppapi::HostResource(),
On 2012/05/16 16:43:50, Fady Samuel wrote:
> On 2012/05/16 16:39:43, scshunt wrote:
> > On 2012/05/15 20:59:17, piman wrote:
> > > I don't understand, don't you want to pass the share_context resource
here?
> > 
> > I haven't touched this method. Possibly yes; this is actually just a hunk
from
> > fsamuel's patch that I had to copy over since the interface to this message
> > changed.
> 
> Yup, this was some stuff I changed when I started working on this plumbing.
> There's no reason to do this really, just create a HostResource from
PP_Instance
> and PP_Resource in CreateProxyResource.

Note: it's not trivially creating a HostResource(instance, share_context), the
resource IDs are different on both sides. What you want to do is lookup the
resource object and get its host_resource().
If you follow my advice for the ppb_graphics_3d_impl and move the lookup (the
EnterResource) into the thunk layer, then both sides can share that lookup code,
and you can just pass through the resource object.

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:225:
EnterResourceNoLock<PPB_Graphics3D_API> enter(share_context, false);
On 2012/05/16 16:39:43, scshunt wrote:
> On 2012/05/15 20:59:17, piman wrote:
> > It'd be nicer to do the EnterResource thing in the thunk layer
> 
> Which is the thunk layer?

ppapi/thunk/ppb_graphics_3d_thunk.cc

[Fady Samuel, 2012-05-16 17:40:54]

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
File content/common/webkitplatformsupport_impl.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/1/content/common/webkitp...
content/common/webkitplatformsupport_impl.h:43: WebKit::WebView* webView);
On 2012/05/16 17:12:00, piman wrote:
> On 2012/05/16 16:39:43, scshunt wrote:
> > On 2012/05/15 20:59:17, piman wrote:
> > > NAK, the offscreen contexts are shared between webviews, so we can't have
> this
> > > (in theory it's possible to create offscreen contexts without a view, and
we
> > > want to keep that behavior).
> > 
> > See the WebKit bug thread for some discussion of this. The idea here is that
> the
> > WebView (or, specifically, the RenderViewImpl) contains the necessary state
to
> > determine whether or not to request the context, not that the context should
> be
> > associated with the WebView. The code will work fine without a WebView.
> 
> This is flawed. See the discussion there.
> I don't see why you need this. If you're a guest renderer, all your contexts
> need to go through pepper. If you're not, all your contexts need to go
directly
> to the GPU process. Regardless of the webview.
> 
> I really suggest you move the webkit changes out of this CL, otherwise it's
> going to hold up the part where we're in agreement.
> Besides, you'll need to handle the fact that webkit rolls at a different time
> than this CL, so even if we settle on changing the interface, you'll need to
> have some temporary code for managing the transition without breaking the
build.


I agree, let's split this up into two cls.

As for the issue, I'm still trying to figure out why that is? Our current
requirement is a single render process may have many RenderViews, each with a
different embedder process, and hence, multiple GuestToEmbedderChannels, one per
embedder process. If the two embedders are in different SiteInstances, then the
guests should not be able to share texture resources. If the two embedders share
a SiteInstance (share a process), then the guests will automagically share
resources, anyway because they'll be using the one GuestToEmbedderChannel. That
same renderer process may have RenderViews that do not have an embedder and so
their contexts go through the GPU process. I'm waiting on creis@ for some
further clarification but it seems that cross-process navigation is allowed to
reuse old RenderViews, and so a RenderView that had a context directly with the
GPU process may need to drop that context and pick up a pepper context. This is
a fairly complicated problem that I think needs further discussion offline.

[(scshunt), 2012-05-16 23:10:44]

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
ppapi/proxy/ppb_graphics_3d_proxy.cc:173: API_ID_PPB_GRAPHICS_3D, instance,
HostResource(), attribs, &result));
I'm going to leave this as-is for now. Adding support for this might be a later
CL.

[piman, 2012-05-16 23:38:44]

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:118:
CommandBufferProxy* share_buffer = 0;
nit: share_buffer = NULL

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:121:
static_cast<PlatformContext3DImpl*>(share_context);
nit: indent for continuation should be +4

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
ppapi/proxy/ppb_graphics_3d_proxy.cc:173: API_ID_PPB_GRAPHICS_3D, instance,
HostResource(), attribs, &result));
On 2012/05/16 23:10:44, scshunt wrote:
> I'm going to leave this as-is for now. Adding support for this might be a
later
> CL.

Why? It should really be as simple as
static_cast<Graphics3D*>(share_context)->host_resource() and remove the
DCHECK/early exit above.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/resourc...
File ppapi/proxy/resource_creation_proxy.h (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/resourc...
ppapi/proxy/resource_creation_proxy.h:28: } // namespace thunk
You shouldn't need that since it has to be implicitly forward-declared by
resource_creation_api.h, since it's part of the API.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_3d_thunk.cc:30: EnterGraphics3D
enter_share(share_context, true);
This should be EnterResourceNoLock, since the lock was already taken by the
EnterResourceCreation.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
File ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc:27: EnterGraphics3D
enter_share(share_context, true);
same here, NoLock.

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:221:
PluginDelegate::PlatformContext3D* share_platform_context = 0;
nit: s/0/NULL/

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:224:
static_cast<PPB_Graphics3D_Impl*>(share_context);
nit: indent +4

[piman, 2012-05-17 00:05:58]

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:212: return
CreateGLES2Impl(kCommandBufferSize, kTransferBufferSize);
Actually, I'm just realizing, this will also need to pass the share context's
gles2_impl() so that you can share resources at the client level (all you have
right now is the sharing on the other side).

You'll need to modify CreateGLES2Impl to pass the other GLES2Implementation's
share_group() as a parameter to the constructor of the new GLES2Implementation.

[(scshunt), 2012-05-17 16:55:45]

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:118:
CommandBufferProxy* share_buffer = 0;
On 2012/05/16 23:38:45, piman wrote:
> nit: share_buffer = NULL
picked.

https://chromiumcodereview.appspot.com/10386145/diff/9002/content/renderer/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:121:
static_cast<PlatformContext3DImpl*>(share_context);
On 2012/05/16 23:38:45, piman wrote:
> nit: indent for continuation should be +4
picked.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/proxy/ppb_gra...
ppapi/proxy/ppb_graphics_3d_proxy.cc:173: API_ID_PPB_GRAPHICS_3D, instance,
HostResource(), attribs, &result));
On 2012/05/16 23:38:45, piman wrote:
> On 2012/05/16 23:10:44, scshunt wrote:
> > I'm going to leave this as-is for now. Adding support for this might be a
> later
> > CL.
> 
> Why? It should really be as simple as
> static_cast<Graphics3D*>(share_context)->host_resource() and remove the
> DCHECK/early exit above.

Oh. Ok.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_3d_thunk.cc:30: EnterGraphics3D
enter_share(share_context, true);
On 2012/05/16 23:38:45, piman wrote:
> This should be EnterResourceNoLock, since the lock was already taken by the
> EnterResourceCreation.
Ah ok.

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
File ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc:27: EnterGraphics3D
enter_share(share_context, true);
On 2012/05/16 23:38:45, piman wrote:
> same here, NoLock.

Ok.

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:212: return
CreateGLES2Impl(kCommandBufferSize, kTransferBufferSize);
On 2012/05/17 00:05:58, piman wrote:
> Actually, I'm just realizing, this will also need to pass the share context's
> gles2_impl() so that you can share resources at the client level (all you have
> right now is the sharing on the other side).
> 
> You'll need to modify CreateGLES2Impl to pass the other GLES2Implementation's
> share_group() as a parameter to the constructor of the new
GLES2Implementation.

Currently I believe we only use the Raw codepath. Should this change, then, to
ensure that we get the gles2 stuff? Should the plugin API offer a Create and
CreateRaw itself?

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:221:
PluginDelegate::PlatformContext3D* share_platform_context = 0;
On 2012/05/16 23:38:45, piman wrote:
> nit: s/0/NULL/

picked.

https://chromiumcodereview.appspot.com/10386145/diff/9002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:224:
static_cast<PPB_Graphics3D_Impl*>(share_context);
On 2012/05/16 23:38:45, piman wrote:
> nit: indent +4

Also picked.

[(scshunt), 2012-05-24 18:38:39]

I've updated this with mostly everything that I think I can do reasonably right
now, except possibly put GLES2 code into Graphics3D. But I have no real way of
testing that, and I won't be able to test the equivalent code until after we
sort out the issues with the sharing model and I can actually connect all the
pieces together.

[piman, 2012-05-24 23:10:08]

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:168: :
ppapi::HostResource(),
No, you should pass here the WebGraphicsContext3DCommandBufferImpl you're
sharing with, and unwrap it to get to the resource.
you can have several onscreen contexts (say for different renderviews) that need
to share together. Also onscreen vs shared context can be created in any order,
but they need to share with one another.

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:182:
render_view->set_guest_graphics_resource(resource);
err, what if we have more than 1 "onscreen" context per render view? This
happens today during lost context recovery (you may have at the same time the
old context which is lost, and the new one).

[(scshunt), 2012-05-24 23:18:07]

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:168: :
ppapi::HostResource(),
On 2012/05/24 23:10:08, piman wrote:
> No, you should pass here the WebGraphicsContext3DCommandBufferImpl you're
> sharing with, and unwrap it to get to the resource.
> you can have several onscreen contexts (say for different renderviews) that
need
> to share together. Also onscreen vs shared context can be created in any
order,
> but they need to share with one another.

I'm going to be changing this up a lot here now that I have a better idea of the
sharing model I think we want to use, so anything here will be temporary anyway.

If I change this into a ppapi::HostResource() in the meanwhile (getting rid of
the conditional), and drop the rest of the file, is this good to go? It would be
good to upstream the rest of this code.

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:182:
render_view->set_guest_graphics_resource(resource);
On 2012/05/24 23:10:08, piman wrote:
> err, what if we have more than 1 "onscreen" context per render view? This
> happens today during lost context recovery (you may have at the same time the
> old context which is lost, and the new one).

This is actually an unrelated fix, and is an improvement over the old code, but
only slightly. This will probably change now that I think I have a good model of
the sharing model.

[Fady Samuel, 2012-05-24 23:19:00]

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
File content/renderer/browser_plugin/guest_to_embedder_channel.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/17001/content/renderer/b...
content/renderer/browser_plugin/guest_to_embedder_channel.cc:182:
render_view->set_guest_graphics_resource(resource);
On 2012/05/24 23:10:08, piman wrote:
> err, what if we have more than 1 "onscreen" context per render view? This
> happens today during lost context recovery (you may have at the same time the
> old context which is lost, and the new one).

PpapiCommandBufferProxy knows about its HostResource. So the old, lost context
will still be able to talk to the embedder.

guest_graphics_resource is only used in RenderViewImpl::DidInitiatePaint to
inform the embedder it should invalidate its browser plugin container.

[(scshunt), 2012-05-25 21:18:36]

I've fixed the indicated and brought up to a recent HEAD; it should be ready to
go now.

[piman, 2012-05-25 21:21:09]

On 2012/05/25 21:18:36, scshunt wrote:
> I've fixed the indicated and brought up to a recent HEAD; it should be ready
to
> go now.

I still want the GLES2Implementation construction fix (to pass the share group
there). The rest is fine.

[(scshunt), 2012-05-25 22:34:55]

Sorry about that. I think I got this correct?

[piman, 2012-05-25 22:41:00]

LGTM+nits. Thanks!

https://chromiumcodereview.appspot.com/10386145/diff/22002/ppapi/shared_impl/...
File ppapi/shared_impl/ppb_graphics_3d_shared.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/22002/ppapi/shared_impl/...
ppapi/shared_impl/ppb_graphics_3d_shared.cc:122: share_gles2->share_group() :
NULL,
nit: looks like this should fit on the same line

https://chromiumcodereview.appspot.com/10386145/diff/22002/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/22002/webkit/plugins/ppa...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:215: =
static_cast<PPB_Graphics3D_Shared*>(share_context)->gles2_impl();
nit: for wrapping, the = should go on the previous line.

[(scshunt), 2012-05-25 22:43:19]

Nits picked. Apparently I can commit if you press some magic button on the
review site?

[commit-bot: I haz the power, 2012-05-25 22:47:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scshunt@google.com/10386145/26008

[commit-bot: I haz the power, 2012-05-25 22:47:30]

Presubmit check for 10386145-26008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ppapi

Presubmit checks took 2.5s to calculate.

[(scshunt), 2012-05-25 22:48:39]

Hey Brett,

Just need your look over the ppapi/ changes.

Thanks,
Sean

[brettw, 2012-05-25 23:19:40]

https://chromiumcodereview.appspot.com/10386145/diff/26008/ppapi/proxy/ppb_gr...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/26008/ppapi/proxy/ppb_gr...
ppapi/proxy/ppb_graphics_3d_proxy.cc:72: bool
Graphics3D::Init(gpu::gles2::GLES2Implementation *share_gles2) {
Nit: * next to type rather than arg name

https://chromiumcodereview.appspot.com/10386145/diff/26008/ppapi/thunk/ppb_gr...
File ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc (right):

https://chromiumcodereview.appspot.com/10386145/diff/26008/ppapi/thunk/ppb_gr...
ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc:30: PPB_Graphics3D_API* share_api =
NULL;
This code should be in the impl/proxy files. I want to eventually autogenerate
the thunk layer, so I don't want the thunk functions doing anything other than
dispatching on the Instance/Resource param and forwarding everything else.

[(scshunt), 2012-05-26 00:14:20]

Ok, should be good to go now?

[Fady Samuel, 2012-05-28 14:11:45]

A few pre-review nits. Please keep changes as compact as possible to avoid
unnecessary code churn and to make cls more readable. Thanks.

http://codereview.chromium.org/10386145/diff/23004/ppapi/proxy/resource_creat...
File ppapi/proxy/resource_creation_proxy.cc (right):

http://codereview.chromium.org/10386145/diff/23004/ppapi/proxy/resource_creat...
ppapi/proxy/resource_creation_proxy.cc:47: using
ppapi::thunk::PPB_Graphics3D_API;
Is this necessary? If not, please remove.

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/ppb_graphics_3...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/ppb_graphics_3...
ppapi/thunk/ppb_graphics_3d_thunk.cc:32: 
Please remove.

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/ppb_graphics_3...
File ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc (right):

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/ppb_graphics_3...
ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc:29: 
Please get rid of this unnecessary change.

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/resource_creat...
File ppapi/thunk/resource_creation_api.h (right):

http://codereview.chromium.org/10386145/diff/23004/ppapi/thunk/resource_creat...
ppapi/thunk/resource_creation_api.h:36: class PPB_Graphics3D_API;
Do we need this?

http://codereview.chromium.org/10386145/diff/23004/webkit/plugins/ppapi/resou...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/10386145/diff/23004/webkit/plugins/ppapi/resou...
webkit/plugins/ppapi/resource_creation_impl.cc:48: using
ppapi::thunk::PPB_Graphics3D_API;
Is this necessary? If not, please get rid of this.

http://codereview.chromium.org/10386145/diff/23004/webkit/plugins/ppapi/resou...
File webkit/plugins/ppapi/resource_creation_impl.h (right):

http://codereview.chromium.org/10386145/diff/23004/webkit/plugins/ppapi/resou...
webkit/plugins/ppapi/resource_creation_impl.h:60: PP_Instance instance,
Hmm, it looks like the signature is the same. What's the purpose of this change?
It should still fit in 80 characters?

[brettw, 2012-05-29 18:20:07]

lgtm

[commit-bot: I haz the power, 2012-05-29 18:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scshunt@google.com/10386145/34001

[commit-bot: I haz the power, 2012-05-29 19:06:18]

Try job failure for 10386145-34001 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-29 19:28:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scshunt@google.com/10386145/19018

[commit-bot: I haz the power, 2012-05-29 21:54:58]

Change committed as 139385