NaCl: Split out Non-SFI logic when starting NaCl.

NaCl: Split out Non-SFI logic when starting NaCl.

This splits out the Non-SFI case more explicitly when starting a NaCl module to
make the code easier to read. There isn't a whole lot of code in common between
the Non-SFI and SFI cases.

BUG=333950

[Mark Seaborn, 2014-05-30 15:36:29]

Please make sure the linux_rel_precise32 trybot passes before committing (see
https://groups.google.com/forum/#!topic/native-client-dev/eLjRyDbf1WA).  LGTM.

https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:408: browser_handle =
IPC::Channel::GenerateVerifiedChannelID("nacl");
Optional: I think that using GenerateVerifiedChannelID() isn't necessary when
using socketpairs, so you should be able to drop these three lines.

https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:451: IPC::ChannelHandle
trusted_renderer_handle = CreateTrustedListener(
I'm not sure what you have planned for NaClTrustedListener at this point.  But
since Non-SFI mode has no distinction between trusted and untrusted code, it
would be a bit confusing to be using NaClTrustedListener there.  Since
NaClTrustedListener does nothing at the moment, it might be better to leave this
out and pass IPC::ChannelHandle() for trusted_renderer_handle.

https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:472: return;
return statement is not needed.

[teravest, 2014-05-30 20:10:56]

On Fri, May 30, 2014 at 9:36 AM,  <mseaborn@chromium.org> wrote:
> Please make sure the linux_rel_precise32 trybot passes before committing
> (see
> https://groups.google.com/forum/#!topic/native-client-dev/eLjRyDbf1WA).
> LGTM.

Thanks for reminding me. I'll make sure that trybot passes before
trying to submit.

>
>
>
https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
> File components/nacl/loader/nacl_listener.cc (right):
>
>
https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
> components/nacl/loader/nacl_listener.cc:408: browser_handle =
> IPC::Channel::GenerateVerifiedChannelID("nacl");
> Optional: I think that using GenerateVerifiedChannelID() isn't necessary
> when using socketpairs, so you should be able to drop these three lines.

I removed these, and saw some test failures on precise32, so I added
the calls to GenerateVerifiedChannelID() to see if that gets the
trybot passing.

>
>
https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
> components/nacl/loader/nacl_listener.cc:451: IPC::ChannelHandle
> trusted_renderer_handle = CreateTrustedListener(
> I'm not sure what you have planned for NaClTrustedListener at this
> point.  But since Non-SFI mode has no distinction between trusted and
> untrusted code, it would be a bit confusing to be using
> NaClTrustedListener there.  Since NaClTrustedListener does nothing at
> the moment, it might be better to leave this out and pass
> IPC::ChannelHandle() for trusted_renderer_handle.

Done.

>
>
https://codereview.chromium.org/301993003/diff/20001/components/nacl/loader/n...
> components/nacl/loader/nacl_listener.cc:472: return;
> return statement is not needed.
>
> https://codereview.chromium.org/301993003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-06-05 14:08:10]

I had to add back the trusted_plugin_channel in the non-SFI case, though I'm not
sure yet why initialization seems to hang if it's not present. I added a TODO to
remove creation of that channel.

[teravest, 2014-06-05 14:08:13]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-06-05 14:11:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/301993003/80001

[teravest, 2014-06-06 14:25:22]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-06-06 14:26:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/301993003/100001

[teravest, 2014-06-06 15:34:47]

The CQ bit was unchecked by teravest@chromium.org

[teravest, 2014-06-06 15:35:13]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-06-06 15:36:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/301993003/100001

[commit-bot: I haz the power, 2014-06-06 17:21:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:21:51]

Could not make sense out of svn commit message.