PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC

ppapi/proxy/ppapi_messages.h

Index: ppapi/proxy/ppapi_messages.h
diff --git a/ppapi/proxy/ppapi_messages.h b/ppapi/proxy/ppapi_messages.h
index 3bc0d5fe694fe275c3f1911f8070010850c7b8fd..dcde9d9dca9aaea37838d62b955cf5dc18c3621f 100644
--- a/ppapi/proxy/ppapi_messages.h
+++ b/ppapi/proxy/ppapi_messages.h
@@ -868,6 +868,18 @@ IPC_SYNC_MESSAGE_ROUTED1_1(PpapiMsg_PPPInstancePrivate_GetInstanceObject,
 
 #endif  // !defined(OS_NACL) && !defined(NACL_WIN64)
 
+// Calls the PNaCl linker process to link the given object files together
+// to produce a nexe file.  Note that this PNaCl translation plumbing is
+// not part of PPAPI proper, but this message is defined here because it
+// needs to use the ppapi::proxy::SerializedHandle type.

[bbudge, 2015/12/16 20:30:06]

Should this message be moved up close to the other "implementation detail"
messages, e.g. PpapiMsg_InitializeNaClDispatcher? I see the distinction you're
trying to make - that the recipient is not a pepper plugin. Maybe saying that
would be clearer than "not part of PPAPI proper" since that also could apply to
the other implementation detail messages.

[Mark Seaborn, 2015/12/17 06:09:56]

That's not quite the distinction I was making...

By "PPAPI proper" I meant "the set of interfaces defined by ppapi/c/".

NaCl has a couple of things that are outside that set of PPAPI interfaces:

 * Public IRT interfaces, e.g. open_resource(), defined in irt.h.  The
implementation of this has a slightly odd split between ppapi/nacl_irt/
(untrusted side) and components/nacl/renderer/ (trusted side).

 * Plumbing for running the PNaCl translator (not a public interface at all). 
This is also split between ppapi/nacl_irt/ (untrusted side) and
components/nacl/renderer/ (trusted side).

With this comment in the code, I was trying to answer the question "why isn't
this message declared in one of
components/nacl/common/nacl_{host_,renderer_,}messages.h instead?".

Maybe that question doesn't need answering? :-)  I can leave out this comment if
you think it's misleading.  The distinction I was making doesn't matter too much
given that NaCl is not usable separately from PPAPI in Chrome.

[bbudge, 2015/12/17 19:10:24]

Whatever seems reasonable to you. See my other comments.

[Mark Seaborn, 2015/12/21 22:58:50]

OK, I have removed the part mentioning "PPAPI proper" for now, but made the
comment more descriptive in other ways.

+IPC_SYNC_MESSAGE_CONTROL2_1(PpapiMsg_PnaclTranslatorLink,
+                            /* object file FDs for inputs */
+                            std::vector<ppapi::proxy::SerializedHandle>,
+                            /* nexe file FD for output */
+                            ppapi::proxy::SerializedHandle,
+                            /* success status result */
+                            bool)
+
 // Reports to the browser that a plugin has been active.
 IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive)

[bbudge, 2015/12/16 20:30:06]

This also seems like an implementation detail message. As long as you're here,
could you move it near PpapiMsg_Crash and PpapiMsg_Hang?

[Mark Seaborn, 2015/12/17 06:09:56]

Hmm, I would regard all the IPC messages as implementation details.

By "implementation detail message", do you mean "doesn't have a 1-1
correspondence with an interface defined in ppapi/c/"?
Not sure that is appropriate, because the file seems to be grouped by:

// These are from the browser to the plugin.
[PpapiMsg_* messages...]

// These are from the plugin to the renderer.
[PpapiHostMsg_* messages...]

(Though maybe there are parts that don't follow that grouping -- it's hard to
tell.)

Hmm, should PnaclTranslatorLink have a PpapiPluginMsg_ prefix?  The naming
scheme is not entirely clear to me.

[bbudge, 2015/12/17 19:10:22]

Right, by "implementation detail" I meant not used for any Pepper API. It's not
a very clear way to say that :)

I agree, PpapiHostMsg_Keepalive should actually be moved below, after
PpapiHostMsg_StartupInitializationComplete.

You raise a good point about naming. I think PpapiPNaClTranslatorMsg_Link or
PpapiPNaClMsg_Link would help distinguish these messages from the others.

[Mark Seaborn, 2015/12/21 22:58:50]

I'm a bit reluctant to start using new prefixes because I'm not sure I
understand the naming conventions fully.

This doc talks about the "ViewMsg_" and "ViewHostMsg_" prefixes but doesn't
explain how it's supposed to be generalised:
https://www.chromium.org/developers/design-documents/inter-process-communication
(which I think I first read ~6 years ago!).  Is it supposed to be based on the
receiving C++ class or the receiving process?

So I'd be inclined to leave it as PpapiMsg_PnaclTranslatorLink.  But some
alternatives would be:
 * PnaclTranslatorMsg_Link
 * PpapiPnaclTranslatorMsg_Link

Please pick one -- I'll defer to your judgement here. :-)

(The Chrome code uses the spelling "Pnacl" rather than "PNaCl", BTW.)

[bbudge, 2015/12/21 23:28:51]

OK, maybe I'm overthinking this. Your first idea seems OK to me, so
PpapiMsg_PnaclTranslatorLink is fine. This comment thread seems to have diverged
from my original intent, which was that the existing PpapiHostMsg_KeepAlive
seems misplaced. But you can leave it if it's too much trouble.

 
@@ -885,6 +897,9 @@ IPC_MESSAGE_CONTROL1(PpapiHostMsg_ChannelCreated,
 IPC_MESSAGE_CONTROL0(PpapiHostMsg_StartupInitializationComplete)
 
 // Calls renderer to open a resource file for nacl_irt_open_resource().
+// Note that open_resource() is not part of PPAPI proper, but this message
+// is defined here because it needs to use the
+// ppapi::proxy::SerializedHandle type.

[bbudge, 2015/12/16 20:30:06]

Related to my above comments, would it be clearer to say that it's a message
from the translator, which is not a pepper plugin?

[Mark Seaborn, 2015/12/17 06:09:56]

PpapiHostMsg_OpenResource is actually usable by normal nexes (those in the Web
Store), in addition to the PNaCl translator.

Again, the distinction I was trying to explain here maybe doesn't matter very
much since NaCl is not usable separately from PPAPI in Chrome.  So I could drop
this extra comment if you prefer.

[bbudge, 2015/12/17 19:10:24]

Hmm, the message name is kind of misleading given the comments above. It really
should be something like PpapiNaClMsg_OpenResource. I guess the comment is
helpful given the naming.

[Mark Seaborn, 2015/12/21 22:58:50]

OK, I'm dropping this comment change from this CL, and moving it to a separate
CL (https://codereview.chromium.org/1540953002/).

 IPC_SYNC_MESSAGE_CONTROL1_3(PpapiHostMsg_OpenResource,
                             std::string /* key */,
                             ppapi::proxy::SerializedHandle /* fd */,