When launching PNaCl helper nexes, explicitly disable IRT loading.

When launching PNaCl helper nexes, explicitly disable IRT loading.

This is done by setting the irt_fd to -1.  However NaCl
currently errors out when given -1, so this depends on a
NaCl-side sel_ldr change which will understand that -1 means 
skip loading: https://codereview.chromium.org/11743028/

BUG= http://code.google.com/p/nativeclient/issues/detail?id=3241
TEST= manual (patch in NaCl side CL and run PNaCl w/ TOOLS_REVISION at 10509).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176389

[Mark Seaborn, 2013-01-04 00:05:41]

LGTM

https://codereview.chromium.org/11761025/diff/1/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/11761025/diff/1/chrome/nacl/nacl_listener.cc#...
chrome/nacl/nacl_listener.cc:253: args->irt_fd = -1;
Maybe comment that the IRT FD/handle remains open in this process, but that
doesn't matter.

https://codereview.chromium.org/11761025/diff/1/chrome/renderer/pepper/ppb_na...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/11761025/diff/1/chrome/renderer/pepper/ppb_na...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:108: PP_ToBool(uses_irt),
You don't need PP_ToBool() here, do you?  It's already a PP_Bool.

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
ppapi/api/private/ppb_nacl_private.idl:47: * The |uses_irt| flag indicates if a
nexe depends on the IRT or not.
Maybe "...indicates whether the IRT should be loaded in this NaCl process"?

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
ppapi/api/private/ppb_nacl_private.idl:54: [in] int32_t socket_count,
Can you keep socket_count and imc_handles together?  You can put uses_irt next
to the other PP_Bools.

I'd put "uses_irt" earlier in the argument list, before "uses_ppapi" (since
that's only temporary) and before "enable_ppapi_dev" (since it's only a dev
option).

https://codereview.chromium.org/11761025/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/11761025/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/plugin.cc:656: false /* uses_irt */,
Maybe comment that the PNaCl translator isn't built to use the IRT, and why.

[jvoung (off chromium), 2013-01-04 00:29:42]

https://codereview.chromium.org/11761025/diff/1/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/11761025/diff/1/chrome/nacl/nacl_listener.cc#...
chrome/nacl/nacl_listener.cc:253: args->irt_fd = -1;
On 2013/01/04 00:05:41, Mark Seaborn wrote:
> Maybe comment that the IRT FD/handle remains open in this process, but that
> doesn't matter.

Or how about avoid sending the handle in the first place?

[jvoung (off chromium), 2013-01-04 17:09:23]

https://codereview.chromium.org/11761025/diff/1/chrome/renderer/pepper/ppb_na...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/11761025/diff/1/chrome/renderer/pepper/ppb_na...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:108: PP_ToBool(uses_irt),
On 2013/01/04 00:05:41, Mark Seaborn wrote:
> You don't need PP_ToBool() here, do you?  It's already a PP_Bool.

This is convert it from a PP_Bool to a "bool" (instead of the other way?)

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
ppapi/api/private/ppb_nacl_private.idl:47: * The |uses_irt| flag indicates if a
nexe depends on the IRT or not.
On 2013/01/04 00:05:41, Mark Seaborn wrote:
> Maybe "...indicates whether the IRT should be loaded in this NaCl process"?

Done.

https://codereview.chromium.org/11761025/diff/1/ppapi/api/private/ppb_nacl_pr...
ppapi/api/private/ppb_nacl_private.idl:54: [in] int32_t socket_count,
On 2013/01/04 00:05:41, Mark Seaborn wrote:
> Can you keep socket_count and imc_handles together?  You can put uses_irt next
> to the other PP_Bools.
> 
> I'd put "uses_irt" earlier in the argument list, before "uses_ppapi" (since
> that's only temporary) and before "enable_ppapi_dev" (since it's only a dev
> option).

Done.  Actually |uses_ppapi| is also toggled to false for PNaCl, so I'm not sure
how temporary that is.  It may hang around until the nacl trusted plugin logic
is rewritten?

https://codereview.chromium.org/11761025/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/11761025/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/plugin.cc:656: false /* uses_irt */,
On 2013/01/04 00:05:41, Mark Seaborn wrote:
> Maybe comment that the PNaCl translator isn't built to use the IRT, and why.

Done.

[Mark Seaborn, 2013-01-04 17:40:53]

LGTM

https://codereview.chromium.org/11761025/diff/10004/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11761025/diff/10004/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:729: // Send over the IRT file
handle.  We don't close our own copy!
I think this comment should stay next to the ShareHandleToSelLdr() call.  It's
commenting on the 'false' arg.

[jvoung (off chromium), 2013-01-04 17:59:26]

+brettw for ppapi and chrome/renderer/pepper OWNERS
+jschuh for chrome/common/*_messages OWNERS

https://codereview.chromium.org/11761025/diff/10004/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11761025/diff/10004/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:729: // Send over the IRT file
handle.  We don't close our own copy!
On 2013/01/04 17:40:54, Mark Seaborn wrote:
> I think this comment should stay next to the ShareHandleToSelLdr() call.  It's
> commenting on the 'false' arg.

Done.

[jschuh, 2013-01-04 20:50:53]

ipc security lgtm

[jvoung (off chromium), 2013-01-07 19:30:56]

brettw: ping

Looks like the number of parameters exceeded the amount for the current ipc
macros, so I also added one for 5 in params and 4 out params...

[brettw, 2013-01-07 20:16:33]

Our rule for the IPC stuff is if you need more than what we have alread, you
have too many and you should write a struct (which will be more readable,
difficult to mess up, etc.).

[Mark Seaborn, 2013-01-07 20:23:13]

On 7 January 2013 12:16, <brettw@chromium.org> wrote:

> Our rule for the IPC stuff is if you need more than what we have alread,
> you
> have too many and you should write a struct (which will be more readable,
> difficult to mess up, etc.).
>

@Jan: You should be able to remove the socket_count argument from
ChromeViewHostMsg_LaunchNaCl (as a separate change), since it's always 1.
See kNumberOfChannelsToBeCreated in
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc.  This
parameter is left over from when it used to be used in the NaCl repo.

Then you won't have to create a struct here.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2013-01-07 20:23:13]

On 7 January 2013 12:16, <brettw@chromium.org> wrote:

> Our rule for the IPC stuff is if you need more than what we have alread,
> you
> have too many and you should write a struct (which will be more readable,
> difficult to mess up, etc.).
>

@Jan: You should be able to remove the socket_count argument from
ChromeViewHostMsg_LaunchNaCl (as a separate change), since it's always 1.
See kNumberOfChannelsToBeCreated in
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc.  This
parameter is left over from when it used to be used in the NaCl repo.

Then you won't have to create a struct here.

Mark

[brettw, 2013-01-07 20:47:29]

If it were me I would do the struct anyway. We'll almost surely be adding more
stuff later. Your choice, though.

[jvoung - send to chromium..., 2013-01-07 22:42:32]

On 2013/01/07 20:47:29, brettw wrote:
> If it were me I would do the struct anyway. We'll almost surely be adding more
> stuff later. Your choice, though.

I can do both bits of cleanup:

Get rid of the unused param in one CL and move the params to a struct in another
CL.  There's no real downside to moving it into a struct is there?

[brettw, 2013-01-07 22:53:59]

The only downside is figuring out the macros to serialize the struct.

[jvoung (off chromium), 2013-01-08 22:26:11]

On 2013/01/07 22:53:59, brettw wrote:
> The only downside is figuring out the macros to serialize the struct.


I think I will hold on turning it into a struct.  That will involve serializing
the GURL-typed parameter, which may involve modifying the googleurl repo?

[brettw, 2013-01-09 07:02:48]

I'm not really sure what your problem is, can you elaborate?

[jvoung (off chromium), 2013-01-11 01:20:10]

rebased on top of other CLs and reverted the IPC macro addition