NaCl Proxy for graphics3d.

NaCl Proxy for graphics3d.
Removal of old context3d and surface3d from nacl proxy will follow.
(Chrome rev will be updated as find_chrome_revisions.py finds a good one
above 97019)

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2060
TEST= initial simple test included, also re-enables gles book examples
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6484

[alokp1, 2011-08-17 04:24:42]

The 3D part looks fine. I would recommend also getting it reviewed by someone
more familiar with NaCl.

http://codereview.chromium.org/7631010/diff/3010/SConstruct
File SConstruct (right):

http://codereview.chromium.org/7631010/diff/3010/SConstruct#newcode2753
SConstruct:2753: 'tests/ppapi_example_gles2/nacl.scons',
I would not recommend using these examples as tests. I will shortly add a valid
test.

http://codereview.chromium.org/7631010/diff/3010/src/shared/ppapi_proxy/brows...
File src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc (right):

http://codereview.chromium.org/7631010/diff/3010/src/shared/ppapi_proxy/brows...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:76: void
PpbGraphics3DRpcServer::PPB_Graphics3D_Create(
Most of it looks straight-forward, but I do not feel qualified to review NaCl
SRPC stuff.

[nfullagar, 2011-08-17 17:39:53]

test removed, ncbray having a look at nacl side.

http://codereview.chromium.org/7631010/diff/3010/SConstruct
File SConstruct (right):

http://codereview.chromium.org/7631010/diff/3010/SConstruct#newcode2753
SConstruct:2753: 'tests/ppapi_example_gles2/nacl.scons',
On 2011/08/17 04:24:42, alokp1 wrote:
> I would not recommend using these examples as tests. I will shortly add a
valid
> test.

ok, re-disabled.

http://codereview.chromium.org/7631010/diff/3010/src/shared/ppapi_proxy/brows...
File src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc (right):

http://codereview.chromium.org/7631010/diff/3010/src/shared/ppapi_proxy/brows...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:76: void
PpbGraphics3DRpcServer::PPB_Graphics3D_Create(
On 2011/08/17 04:24:42, alokp1 wrote:
> Most of it looks straight-forward, but I do not feel qualified to review NaCl
> SRPC stuff.

I'll add another reviewer, thx Alok.

[Nick Bray, 2011-08-17 18:20:33]

I can say that everything but the "new" files looks OK, but there are a few
things that could be improved about the test.

For new new files, get sehr to look at the srpc / proxy glue, I don't know if I
am missing any subtleties.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:20: namespace gles2 {
Is this stylistically correct for nestled namespaces?

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
File tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html (right):

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:18:
plugin.addEventListener('message', messageListener, false);
Use expectEvent(...) isntead of addEventListener / removeEventListener.  See the
"bad" test for an example.

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:22: function
testAndAssertStatusAndCallbacks(test, name, status, callback, calls) {
Not used.

[sehr (please use chromium), 2011-08-17 20:44:36]

Only one significant issue -- virtual destructors.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
File src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:34: (int)
attrib_list_count, attrib_list == NULL ? 1 : 0);
(attrib_list == NULL) ? 1 : 0

here and below.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:202:
(int)resource_id, (int)size);
no c-style casts.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:229:
desc_wrapper.reset(factory.ImportShmHandle((NaClHandle)native_handle,
ditto.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:330: id,
alignment is off for this and the next two lines.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:33: int num = 0;
int32_t num.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:32: ~PluginGraphics3D();
Classes having virtual functions always have virtual destructors.  BTW, is the
Dtor in PluginResource virtual?

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:44: struct PP_CompletionCallback
callback);
alignment.

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
File tests/ppapi_browser/ppb_graphics3d/nacl.scons (right):

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/nacl.scons:29: [ 'ppapi_ppb_graphics3d.html',
indent is goofy.

[nfullagar, 2011-08-18 00:23:57]

ptal, thx!

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
File src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:34: (int)
attrib_list_count, attrib_list == NULL ? 1 : 0);
On 2011/08/17 20:44:36, sehr wrote:
> (attrib_list == NULL) ? 1 : 0
> 
> here and below.

Done.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:202:
(int)resource_id, (int)size);
On 2011/08/17 20:44:36, sehr wrote:
> no c-style casts.

Done.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:229:
desc_wrapper.reset(factory.ImportShmHandle((NaClHandle)native_handle,
On 2011/08/17 20:44:36, sehr wrote:
> ditto.

Done.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:330: id,
On 2011/08/17 20:44:36, sehr wrote:
> alignment is off for this and the next two lines.

Done.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:33: int num = 0;
On 2011/08/17 20:44:36, sehr wrote:
> int32_t num.

Done.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h (right):

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:20: namespace gles2 {
On 2011/08/17 18:20:33, Nick Bray wrote:
> Is this stylistically correct for nestled namespaces?

looks strange but I think that's what we do.

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:32: ~PluginGraphics3D();
On 2011/08/17 20:44:36, sehr wrote:
> Classes having virtual functions always have virtual destructors.  BTW, is the
> Dtor in PluginResource virtual?  

good catch - kargo-kulted over from plugin_context_3d.h (and plugin_surface_3d.h
and those are going away)

http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:44: struct PP_CompletionCallback
callback);
On 2011/08/17 20:44:36, sehr wrote:
> alignment.

Done.

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
File tests/ppapi_browser/ppb_graphics3d/nacl.scons (right):

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/nacl.scons:29: [ 'ppapi_ppb_graphics3d.html',
On 2011/08/17 20:44:36, sehr wrote:
> indent is goofy.

Done.

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
File tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html (right):

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:18:
plugin.addEventListener('message', messageListener, false);
On 2011/08/17 18:20:33, Nick Bray wrote:
> Use expectEvent(...) isntead of addEventListener / removeEventListener.  See
the
> "bad" test for an example.

Done.

http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:22: function
testAndAssertStatusAndCallbacks(test, name, status, callback, calls) {
On 2011/08/17 18:20:33, Nick Bray wrote:
> Not used.

Done.

[sehr (please use chromium), 2011-08-18 00:30:21]

LGTM.

On 2011/08/18 00:23:57, nfullagar wrote:
> ptal, thx!
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
> File src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc (right):
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
> src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:34: (int)
> attrib_list_count, attrib_list == NULL ? 1 : 0);
> On 2011/08/17 20:44:36, sehr wrote:
> > (attrib_list == NULL) ? 1 : 0
> > 
> > here and below.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
> src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:202:
> (int)resource_id, (int)size);
> On 2011/08/17 20:44:36, sehr wrote:
> > no c-style casts.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
> src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:229:
> desc_wrapper.reset(factory.ImportShmHandle((NaClHandle)native_handle,
> On 2011/08/17 20:44:36, sehr wrote:
> > ditto.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/browser...
> src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc:330: id,
> On 2011/08/17 20:44:36, sehr wrote:
> > alignment is off for this and the next two lines.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc (right):
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:33: int num = 0;
> On 2011/08/17 20:44:36, sehr wrote:
> > int32_t num.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> File src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h (right):
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:20: namespace gles2 {
> On 2011/08/17 18:20:33, Nick Bray wrote:
> > Is this stylistically correct for nestled namespaces?
> 
> looks strange but I think that's what we do.
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:32: ~PluginGraphics3D();
> On 2011/08/17 20:44:36, sehr wrote:
> > Classes having virtual functions always have virtual destructors.  BTW, is
the
> > Dtor in PluginResource virtual?  
> 
> good catch - kargo-kulted over from plugin_context_3d.h (and
plugin_surface_3d.h
> and those are going away)
> 
>
http://codereview.chromium.org/7631010/diff/52/src/shared/ppapi_proxy/plugin_...
> src/shared/ppapi_proxy/plugin_ppb_graphics_3d.h:44: struct
PP_CompletionCallback
> callback);
> On 2011/08/17 20:44:36, sehr wrote:
> > alignment.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
> File tests/ppapi_browser/ppb_graphics3d/nacl.scons (right):
> 
>
http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
> tests/ppapi_browser/ppb_graphics3d/nacl.scons:29: [
'ppapi_ppb_graphics3d.html',
> On 2011/08/17 20:44:36, sehr wrote:
> > indent is goofy.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
> File tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html (right):
> 
>
http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
> tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:18:
> plugin.addEventListener('message', messageListener, false);
> On 2011/08/17 18:20:33, Nick Bray wrote:
> > Use expectEvent(...) isntead of addEventListener / removeEventListener.  See
> the
> > "bad" test for an example.
> 
> Done.
> 
>
http://codereview.chromium.org/7631010/diff/52/tests/ppapi_browser/ppb_graphi...
> tests/ppapi_browser/ppb_graphics3d/ppapi_ppb_graphics3d.html:22: function
> testAndAssertStatusAndCallbacks(test, name, status, callback, calls) {
> On 2011/08/17 18:20:33, Nick Bray wrote:
> > Not used.
> 
> Done.

[nfullagar, 2011-08-18 20:22:23]

committed r6484