Convert srpc definitions from using int64 to using PP_Instance, PP_Module, an...

Convert srpc definitions from using int64 to using PP_Instance, PP_Module, and PP_Resource

TEST= fake_browser
BUG= none


Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=4041

[nfullagar, 2010-12-23 21:37:00]

couple line formats but otherwise LGTM, and thanks for cleaning this up!

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_graphics_2d.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_graphics_2d.cc:69: graphics_2d,
line above and two below might fit on single line now?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_image_data.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_image_data.cc:92: resource,
line above and line below could fit on one line?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_ppb_instance_rpc_server.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_ppb_instance_rpc_server.cc:113: instance,
line above and two below could fit on one line?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_ppp_instance.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_ppp_instance.cc:73:
DebugPrintf("Browser::PPP_Instance::DidCreate(%"NACL_PRId32")\n", instance);
Is PP_Instance 32 bit or is this left over from the other CL to convert 64->32 ?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_g...
File src/shared/ppapi_proxy/plugin_graphics_2d.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_g...
src/shared/ppapi_proxy/plugin_graphics_2d.cc:115: GetMainSrpcChannel(),
line above and two below could fit on one line?

[polina, 2010-12-23 21:58:30]

Would it be a lot of trouble to do the same for PP_Bool (vs int32_t)?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_ppp_instance.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_ppp_instance.cc:73:
DebugPrintf("Browser::PPP_Instance::DidCreate(%"NACL_PRId32")\n", instance);
On 2010/12/23 21:37:00, nfullagar wrote:
> Is PP_Instance 32 bit or is this left over from the other CL to convert 64->32
?

Same question. I think this is premature.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
File src/shared/ppapi_proxy/plugin_image_data.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
src/shared/ppapi_proxy/plugin_image_data.cc:66: if (retval ==
NACL_SRPC_RESULT_OK) {
we can get rid of this if-else now if we set resource to kInvalidResourceId at
initialization, can't we?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
File src/shared/ppapi_proxy/ppb_instance.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
src/shared/ppapi_proxy/ppb_instance.srpc:11: 'inputs': [['instance',
'PP_Instance'],  # PP_Instance
# PP_Instance/Resource comments can now be removed

[noelallen_use_chromium, 2010-12-23 22:23:23]

I'll take a quick look at bool and see if that makes sense for this change or a
separate CL.  If same, I'll re-review.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_graphics_2d.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_graphics_2d.cc:69: graphics_2d,
On 2010/12/23 21:37:00, nfullagar wrote:
> line above and two below might fit on single line now?

Done.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_image_data.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_image_data.cc:92: resource,
On 2010/12/23 21:37:00, nfullagar wrote:
> line above and line below could fit on one line?

Done.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_ppb_instance_rpc_server.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_ppb_instance_rpc_server.cc:113: instance,
On 2010/12/23 21:37:00, nfullagar wrote:
> line above and two below could fit on one line?

Done.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
File src/shared/ppapi_proxy/browser_ppp_instance.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/browser_...
src/shared/ppapi_proxy/browser_ppp_instance.cc:73:
DebugPrintf("Browser::PPP_Instance::DidCreate(%"NACL_PRId32")\n", instance);
Whoops, stale edit.  Done.
On 2010/12/23 21:37:00, nfullagar wrote:
> Is PP_Instance 32 bit or is this left over from the other CL to convert 64->32
?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_g...
File src/shared/ppapi_proxy/plugin_graphics_2d.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_g...
src/shared/ppapi_proxy/plugin_graphics_2d.cc:115: GetMainSrpcChannel(),
On 2010/12/23 21:37:00, nfullagar wrote:
> line above and two below could fit on one line?

Done.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
File src/shared/ppapi_proxy/plugin_image_data.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
src/shared/ppapi_proxy/plugin_image_data.cc:66: if (retval ==
NACL_SRPC_RESULT_OK) {
I'm not convinced we can count on the function not to change 'resource' on
error.  What if the resource is allocated, but failes to initialize?  What if
the function write is counting on the user to check for an error?

On 2010/12/23 21:58:30, polina wrote:
> we can get rid of this if-else now if we set resource to kInvalidResourceId at
> initialization, can't we?

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
File src/shared/ppapi_proxy/ppb_instance.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
src/shared/ppapi_proxy/ppb_instance.srpc:11: 'inputs': [['instance',
'PP_Instance'],  # PP_Instance
Not sure that makes sense.  Would be leave the PP_Var comments?  This way they
are all commented.

On 2010/12/23 21:58:30, polina wrote:
> # PP_Instance/Resource comments can now be removed

[polina, 2010-12-23 22:33:13]

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
File src/shared/ppapi_proxy/plugin_image_data.cc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/plugin_i...
src/shared/ppapi_proxy/plugin_image_data.cc:66: if (retval ==
NACL_SRPC_RESULT_OK) {
On 2010/12/23 22:23:23, noelallen wrote:
> I'm not convinced we can count on the function not to change 'resource' on
> error.  What if the resource is allocated, but failes to initialize?  What if
> the function write is counting on the user to check for an error?
> 
> On 2010/12/23 21:58:30, polina wrote:
> > we can get rid of this if-else now if we set resource to kInvalidResourceId
at
> > initialization, can't we?
> 

We implement the server code, so we know exactly how this works. When it comes
to returning a resource, it is an actual return value (not output arg) from a
Chrome function. The server functions that call them always return OK.

That said, I am ok with keeping this "better-safe-than-sorry" code. But I would
suggest dropping the "else" and {}.

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
File src/shared/ppapi_proxy/ppb_instance.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
src/shared/ppapi_proxy/ppb_instance.srpc:11: 'inputs': [['instance',
'PP_Instance'],  # PP_Instance
On 2010/12/23 22:23:23, noelallen wrote:
> Not sure that makes sense.  Would be leave the PP_Var comments?  This way they
> are all commented.
> 
> On 2010/12/23 21:58:30, polina wrote:
> > # PP_Instance/Resource comments can now be removed

It's up to us whether we want to always comment or only when the srpc type is
different from the PPB signature type. Either is fine.

[neb, 2010-12-23 23:11:39]

Would you mind PP_Bool to the bunch? It would make .srpc files more readable.
Otherwise, LGTM.

[polina, 2010-12-23 23:41:23]

Marked all PP_Bools in ppb_* files. 
(Suggested alternative name consistent across in a couple of cases, but you
don't have to take them if it's too much work).

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
File src/shared/ppapi_proxy/ppb_audio_config_dev.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
src/shared/ppapi_proxy/ppb_audio_config_dev.srpc:24: 'outputs': [['out_bool',
'int32_t']
'outputs': [['is_audio_config', 'PP_Bool']

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
File src/shared/ppapi_proxy/ppb_audio_dev.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
src/shared/ppapi_proxy/ppb_audio_dev.srpc:24: 'outputs': [['out_bool',
'int32_t'],
'outputs': [['is_audio', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
src/shared/ppapi_proxy/ppb_audio_dev.srpc:38: 'outputs': [['out_bool',
'int32_t'],
'outputs': [['success', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_audi...
src/shared/ppapi_proxy/ppb_audio_dev.srpc:45: 'outputs': [['out_bool',
'int32_t'],
'outputs': [['success', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_grap...
File src/shared/ppapi_proxy/ppb_graphics_2d.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_grap...
src/shared/ppapi_proxy/ppb_graphics_2d.srpc:13: ['is_always_opaque', 'int32_t'],
 ['is_always_opaque', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_grap...
src/shared/ppapi_proxy/ppb_graphics_2d.srpc:21: 'outputs': [['success',
'int32_t'],
'outputs': [['is_graphics2d', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_grap...
src/shared/ppapi_proxy/ppb_graphics_2d.srpc:28: ['is_always_opaque', 'int32_t'],
['is_always_opaque', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_grap...
src/shared/ppapi_proxy/ppb_graphics_2d.srpc:29: ['success', 'int32_t'],
'outputs': [['success', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_imag...
File src/shared/ppapi_proxy/ppb_image_data.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_imag...
src/shared/ppapi_proxy/ppb_image_data.srpc:18: 'outputs': [['success',
'int32_t'],
 'outputs': [['success', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_imag...
src/shared/ppapi_proxy/ppb_image_data.srpc:25: ['init_to_zero', 'int32_t'],
['init_to_zero', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_imag...
src/shared/ppapi_proxy/ppb_image_data.srpc:33: 'outputs': [['success',
'int32_t'],
'outputs': [['is_imagedata', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_imag...
src/shared/ppapi_proxy/ppb_image_data.srpc:40: ['success', 'int32_t'],
['success', 'PP_Bool'],

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
File src/shared/ppapi_proxy/ppb_instance.srpc (right):

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
src/shared/ppapi_proxy/ppb_instance.srpc:26: 'outputs': [['success', 'int32_t'],
 # PP_Bool
 'outputs': [['success', 'PP_Bool'],  # PP_Bool

http://codereview.chromium.org/5974006/diff/1/src/shared/ppapi_proxy/ppb_inst...
src/shared/ppapi_proxy/ppb_instance.srpc:32: 'outputs': [['is_full_frame',
'int32_t'],  # PP_Bool
'outputs': [['is_full_frame', 'PP_Bool'],  # PP_Bool

[noelallen_use_chromium, 2010-12-28 22:19:36]

I tried the PP_Bool changes, unfortunately since PP_Bool is an enum and NOT an
int, we can not cleanly convert from our variant to a PP_Bool type.

In addition adding a dependency on pp_bool.h for generic SRPC seems
questionable, so I went ahead with the original change without PP_Bool.

[polina, 2010-12-28 23:57:19]

Thanks for trying, Noel. Even without PP_Bool this is an improvement. Could you
please add url_request and url_response files to the CL?

On 2010/12/28 22:19:36, noelallen wrote:
> I tried the PP_Bool changes, unfortunately since PP_Bool is an enum and NOT an
> int, we can not cleanly convert from our variant to a PP_Bool type.
> 
> In addition adding a dependency on pp_bool.h for generic SRPC seems
> questionable, so I went ahead with the original change without PP_Bool.

[polina, 2010-12-29 00:29:44]

Aren't you missing browser_ppb_url* files?

On 2010/12/29 00:23:45, noelallen wrote:

[polina, 2010-12-29 00:47:49]

lgtm (if trybots are happy)