[WIP] Yet another demo for BMM NaCl ppapi connection.

[WIP] Yet another demo for BMM NaCl ppapi connection.

BUG=

[Mark Seaborn, 2014-02-03 22:59:07]

Can you reduce this change down to just the refactoring of the existing NaCl
startup code path, please?

i.e. Get this ready for commit by removing the parts that add new functionality
for Non-SFI Mode.

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:850: void
NaClProcessHost::OnPpapiRendererChannelCreated(
This method should go away, right?

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:861: bool
NaClProcessHost::OnUntrustedMessageForwarded(const IPC::Message& msg) {
I think this should go away too, since it only handles
PpapiHostMsg_NaClChannelCreated.

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.h:180: void
OnPpapiBrowserChannelCreated(
Rename to OnPpapiChannelsCreated()?

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.h:184: void
OnPpapiRendererChannelCreated(const IPC::ChannelHandle& channel_handle);
This method should go away, right?

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_ipc_adapter.cc (left):

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_ipc_adapter.cc:524: new
PpapiHostMsg_NaClChannelCreated(channel_handle));
Also remove the definition of PpapiHostMsg_NaClChannelCreated from
ppapi/proxy/ppapi_messages.h, since this is the only place where it is sent.

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:252:
IPC::Channel::SocketPair(&browser_ppapi_fds[0], &browser_ppapi_fds[1]);
You need to check the return value here.  However, this part isn't needed for
the first (refactoring-only) commit.

https://codereview.chromium.org/139993009/diff/1/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/139993009/diff/1/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:1110: bool Channel::SocketPair(int* fd1, int* fd2) {
Why have a layer of wrapping?

You could just declare SocketPair() as a function inside the IPC namespace.  It
doesn't need to be a static member of a class.

However, this part isn't needed for the first (refactoring-only) commit...

[Mark Seaborn, 2014-02-04 01:18:30]

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (left):

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:840: new
PpapiMsg_CreateNaClChannel(
I think you will want to keep the PpapiMsg_CreateNaClChannel message, in order
to send the PpapiNaClChannelArgs to the NaCl process.

So you'll still want to send PpapiMsg_CreateNaClChannel here (though the handle
argument seems to be unnecessary -- I don't know why it's there), but then still
call ReplyToRenderer() afterwards.

But then plugin_main_nacl.cc will still need to wait for
PpapiMsg_CreateNaClChannel() and create a PluginDispatcher in response.  It just
won't need to send a handle back (because it will use the pre-created renderer
socket FD), so you can still remove PpapiHostMsg_NaClChannelCreated.

[hidehiko, 2014-02-05 08:43:32]

As you know, I've sent a CL about refactoring (crrev.com/150713003), and I think
we have few changes on it.
# Again, thank you for your review. I addressed your comments to this CL in the
under-review CL.

So, I think it's time to move forward.
I made the third demo on the patch (crrev.com/140573003).
The main focus of the change is the BMM actually
(ipc_channel.h, nacl_listener.cc, nonsfi_main.cc, irt_ppapi.cc,
ppapi/proxy/plugin_main.cc and ppruntime.h).

Could you take a look at that CL?

The summary of what I did:
- Exposed SocketPair and use it in nacl_lister on BMM.
- Set raw FD #s to global vars in plugin_main.cc.
- Created a new thread for the plugin main thread, to return from the
load_module SRPC.
- Introduced components/nacl/loader/nonsfi/DEPS file to resolve presubmit error.
- Linked ppapi_proxy to nacl_helper.

If the direction is OK, I'll create new CLs by extracting main parts from the
demo CL for the code review.
Any feedback is welcome.

Thanks!
- hidehiko



On 2014/02/04 01:18:30, Mark Seaborn wrote:
>
https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
> File components/nacl/browser/nacl_process_host.cc (left):
> 
>
https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
> components/nacl/browser/nacl_process_host.cc:840: new
> PpapiMsg_CreateNaClChannel(
> I think you will want to keep the PpapiMsg_CreateNaClChannel message, in order
> to send the PpapiNaClChannelArgs to the NaCl process.
> 
> So you'll still want to send PpapiMsg_CreateNaClChannel here (though the
handle
> argument seems to be unnecessary -- I don't know why it's there), but then
still
> call ReplyToRenderer() afterwards.
> 
> But then plugin_main_nacl.cc will still need to wait for
> PpapiMsg_CreateNaClChannel() and create a PluginDispatcher in response.  It
just
> won't need to send a handle back (because it will use the pre-created renderer
> socket FD), so you can still remove PpapiHostMsg_NaClChannelCreated.

[hidehiko, 2014-02-07 01:30:25]

On 2014/02/05 08:43:32, hidehiko wrote:
> As you know, I've sent a CL about refactoring (crrev.com/150713003), and I
think
> we have few changes on it.
> # Again, thank you for your review. I addressed your comments to this CL in
the
> under-review CL.
> 
> So, I think it's time to move forward.
> I made the third demo on the patch (crrev.com/140573003).
> The main focus of the change is the BMM actually
> (ipc_channel.h, nacl_listener.cc, nonsfi_main.cc, irt_ppapi.cc,
> ppapi/proxy/plugin_main.cc and ppruntime.h).
> 
> Could you take a look at that CL?
> 
> The summary of what I did:
> - Exposed SocketPair and use it in nacl_lister on BMM.
> - Set raw FD #s to global vars in plugin_main.cc.
> - Created a new thread for the plugin main thread, to return from the
> load_module SRPC.
> - Introduced components/nacl/loader/nonsfi/DEPS file to resolve presubmit
error.
> - Linked ppapi_proxy to nacl_helper.
> 
> If the direction is OK, I'll create new CLs by extracting main parts from the
> demo CL for the code review.
> Any feedback is welcome.
> 
> Thanks!
> - hidehiko
> 

Hi Mark,
friendly ping?

- hidehiko


> 
> 
> On 2014/02/04 01:18:30, Mark Seaborn wrote:
> >
>
https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
> > File components/nacl/browser/nacl_process_host.cc (left):
> > 
> >
>
https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
> > components/nacl/browser/nacl_process_host.cc:840: new
> > PpapiMsg_CreateNaClChannel(
> > I think you will want to keep the PpapiMsg_CreateNaClChannel message, in
order
> > to send the PpapiNaClChannelArgs to the NaCl process.
> > 
> > So you'll still want to send PpapiMsg_CreateNaClChannel here (though the
> handle
> > argument seems to be unnecessary -- I don't know why it's there), but then
> still
> > call ReplyToRenderer() afterwards.
> > 
> > But then plugin_main_nacl.cc will still need to wait for
> > PpapiMsg_CreateNaClChannel() and create a PluginDispatcher in response.  It
> just
> > won't need to send a handle back (because it will use the pre-created
renderer
> > socket FD), so you can still remove PpapiHostMsg_NaClChannelCreated.

[Mark Seaborn, 2014-02-07 22:53:43]

I started writing comments on this change, but then I realised you actually want
me to comment on https://codereview.chromium.org/140573003.  That's really
confusing.  Please don't send me pings on changes you don't want me to review.
:-)  Please close changes if they're obsolete, and if a change is ready for
review, fill out its commit message and press "Publish+Mail Comments".

I will start looking at https://codereview.chromium.org/140573003 now...

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.h:180: void
OnPpapiBrowserChannelCreated(
Can you rebase this onto the refactoring that you committed
(https://src.chromium.org/viewvc/chrome?revision=249531&view=revision), please?

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_filename.cc (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_filename.cc:4: 
Can you leave out the implementation of irt_filename.cc, please, as we discussed
in https://codereview.chromium.org/130753003/?

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/139993009/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_main.cc:85: event.Wait();
This stuff with SetPpapiStartEvent() shouldn't be necessary.  load_module can
return immediately.  It doesn't have to wait until user code calls ppapi_start()
before returning.

https://codereview.chromium.org/139993009/diff/1/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/139993009/diff/1/ipc/ipc_channel.h#newcode206
ipc/ipc_channel.h:206: // On POSIX an IPC::Channel wraps a socketpair() with set
some attributes
"with some attributes set"

However, since this is documenting the interface it probably shouldn't specify
implementation details.  So I'd just say "SocketPair() returns a pair of socket
FDs suitable for using with IPC::Channel".