Remove redundant use_nonsfi_mode from SelLdrStartParams.

Remove redundant use_nonsfi_mode from SelLdrStartParams.

The params is used for ServiceRuntime::StartSelLdr(). ServiceRuntime
already knows if it is for nonsfi or not. So the use_nonsfi_mode in
SelLdrStartParams is just redundant. Remove it.

BUG=333950
TEST=Ran browser_tests --gtest_filter=*NonSfi*:*NonSFI* locally, and trybots.
CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32

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

[hidehiko, 2014-06-17 05:02:28]

Hi Mark, PTAL.

This is extracted from crrev.com/337463002 as you suggested.
Unfortunately, now, linux_rel_precise32 bot has a disk-full issue. I'll run
trybot when it is fixed.

Regards,
- hidehiko

[Mark Seaborn, 2014-06-17 15:16:55]

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

https://codereview.chromium.org/332333003/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:77: false,  // No non-SFI mode
(i.e. in SFI-mode).
It's not obvious why it's OK to change this to 'false'.

Is it because LoadNaClModuleFromBackgroundThread() is only called from
LoadHelperNaClModule(), for the PNaCl translator?  It's not obvious, though,
that "from background thread" implies "for the PNaCl translator" and so "SFI
mode only".

Would it be worth adding a comment?

I suppose Non-SFI loading is covered by tests, so LGTM.  I'd like for Justin to
sign off on this, because he's more of an owner of this code than me.

[chromium-reviews, 2014-06-17 15:20:03]

On Tue, Jun 17, 2014 at 9:16 AM,  <mseaborn@chromium.org> wrote:
>
>
https://codereview.chromium.org/332333003/diff/1/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/plugin.cc (right):
>
>
https://codereview.chromium.org/332333003/diff/1/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/plugin.cc:77: false,  // No
>
> non-SFI mode (i.e. in SFI-mode).
> It's not obvious why it's OK to change this to 'false'.
>
> Is it because LoadNaClModuleFromBackgroundThread() is only called from
> LoadHelperNaClModule(), for the PNaCl translator?

Yes.

> It's not obvious,
> though, that "from background thread" implies "for the PNaCl translator"
> and so "SFI mode only".
>
> Would it be worth adding a comment?

Perhaps LoadNaClModuleFromBackgroundThread should be renamed to
LoadHelperNaClModule. We often use "helper" to denote modules used for
PNaCl translation. A comment wouldn't hurt, either.

>
> I suppose Non-SFI loading is covered by tests, so LGTM.  I'd like for
> Justin to sign off on this, because he's more of an owner of this code
> than me.
>
> https://codereview.chromium.org/332333003/

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

[hidehiko, 2014-06-17 16:47:03]

Thank you for review. PTAL?

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

https://codereview.chromium.org/332333003/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:77: false,  // No non-SFI mode
(i.e. in SFI-mode).
On 2014/06/17 15:16:55, Mark Seaborn wrote:
> It's not obvious why it's OK to change this to 'false'.
> 
> Is it because LoadNaClModuleFromBackgroundThread() is only called from
> LoadHelperNaClModule(), for the PNaCl translator?  It's not obvious, though,
> that "from background thread" implies "for the PNaCl translator" and so "SFI
> mode only".
> 
> Would it be worth adding a comment?
> 
> I suppose Non-SFI loading is covered by tests, so LGTM.  I'd like for Justin
to
> sign off on this, because he's more of an owner of this code than me.

Renamed the method name as Justin suggested. Also added short note that this is
for pnacl in the .h file.

[teravest, 2014-06-17 17:07:37]

lgtm

Thanks! This is definitely an improvement.

[hidehiko, 2014-06-17 17:56:51]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:58:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/332333003/20001

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

Change committed as 277864