Non-SFI mode: Build ppapi libraries for nacl_helper_nonsfi.

Non-SFI mode: Build ppapi libraries for nacl_helper_nonsfi.

This CL builds the ppapi_proxy, ppapi_shared and ppapi_ipc libraries
for nacl_helper_nonsfi.
It is now for just making sure it is built properly. It will be actually used
from nacl_helper_nonsfi in a following CL.

BUG=358465
TEST=Ran trybot. Built nacl_helper_nonsfi on top of this CL, and made sure some browser tests actually work well locally.

Committed: https://crrev.com/b0c30620cec01a6d508347537860c8e97d7a861e
Cr-Commit-Position: refs/heads/master@{#302015}

[hidehiko, 2014-10-27 08:51:56]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org, teravest@chromium.org

[hidehiko, 2014-10-27 08:51:56]

Thank you for your review in advance,
- hidehiko

[Mark Seaborn, 2014-10-27 18:59:07]

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/manifest_serv...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/manifest_serv...
ppapi/nacl_irt/manifest_service.cc:107: #if defined(OS_NACL) &&
!defined(__native_client_nonsfi__)
Can use OS_NACL_SFI once your other change is committed.

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/plugin_main.cc
File ppapi/nacl_irt/plugin_main.cc (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/plugin_main.c...
ppapi/nacl_irt/plugin_main.cc:36: #if defined(__native_client__) &&
!defined(__native_client_nonsfi__)
Similarly, OS_NACL_SFI.  You might as well commit your other change first to
reduce churn here.

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp
File ppapi/ppapi_proxy_nacl.gyp (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp#n...
ppapi/ppapi_proxy_nacl.gyp:67: 'nacl_irt/irt_ppapi.cc',
Surely you are going to want some of irt_ppapi.cc eventually, such as the
definition of irt_ppapihook?

In the interests of reducing the number of targets duplicated as "FOO" and
"FOO_nonsfi" variants, can you do conditionalisation in these source files
instead of in the Gyp logic, please?

(For now, I think that irt_ppapi.cc will compile fine as-is.  So maybe we can
leave adding #ifs to it until the point where we can test them?)

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp#n...
ppapi/ppapi_proxy_nacl.gyp:69: 'nacl_irt/irt_start.cc',
Same applies to irt_start.cc.  At the point where you need this to be
conditionalised, you could wrap the code in this file with "#if OS_NACL_SFI".

[hidehiko, 2014-10-28 19:28:09]

Patchset #2 (id:10001) has been deleted

[hidehiko, 2014-10-28 19:35:55]

Patchset #2 (id:30001) has been deleted

[hidehiko, 2014-10-28 20:14:00]

Thank you for review. PTAL.

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/manifest_serv...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/manifest_serv...
ppapi/nacl_irt/manifest_service.cc:107: #if defined(OS_NACL) &&
!defined(__native_client_nonsfi__)
On 2014/10/27 18:59:07, Mark Seaborn wrote:
> Can use OS_NACL_SFI once your other change is committed.

Done.

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/plugin_main.cc
File ppapi/nacl_irt/plugin_main.cc (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/nacl_irt/plugin_main.c...
ppapi/nacl_irt/plugin_main.cc:36: #if defined(__native_client__) &&
!defined(__native_client_nonsfi__)
On 2014/10/27 18:59:07, Mark Seaborn wrote:
> Similarly, OS_NACL_SFI.  You might as well commit your other change first to
> reduce churn here.

Done.

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp
File ppapi/ppapi_proxy_nacl.gyp (right):

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp#n...
ppapi/ppapi_proxy_nacl.gyp:67: 'nacl_irt/irt_ppapi.cc',
On 2014/10/27 18:59:07, Mark Seaborn wrote:
> Surely you are going to want some of irt_ppapi.cc eventually, such as the
> definition of irt_ppapihook?
> 
> In the interests of reducing the number of targets duplicated as "FOO" and
> "FOO_nonsfi" variants, can you do conditionalisation in these source files
> instead of in the Gyp logic, please?
> 
> (For now, I think that irt_ppapi.cc will compile fine as-is.  So maybe we can
> leave adding #ifs to it until the point where we can test them?)

Ah, I didn't think about sharing the irt_ppapi.cc, but admittedly we can.
Now, irt_ppapi is properly built for nacl_helper_nonsfi (rather than exclude its
contents by #ifdef).

https://codereview.chromium.org/652393003/diff/1/ppapi/ppapi_proxy_nacl.gyp#n...
ppapi/ppapi_proxy_nacl.gyp:69: 'nacl_irt/irt_start.cc',
On 2014/10/27 18:59:07, Mark Seaborn wrote:
> Same applies to irt_start.cc.  At the point where you need this to be
> conditionalised, you could wrap the code in this file with "#if OS_NACL_SFI".

Unlike irt_ppapi.cc, we won't use this at all. Exclude its contents by #ifdef at
all.

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:23: g_is_main_thread = 1;
FYI: Is this flag still needed? IIUC, this is no longer used.
If unnecessary, I'm happy to work on its removal in a separate CL.

[Mark Seaborn, 2014-10-28 21:57:23]

LGTM

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:21: #if !defined(OS_NACL_NONSFI)
If you're going to add a conditional, please add a comment explaining why it's
here, plus maybe a TODO if you plan to remove g_is_main_thread.

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:23: g_is_main_thread = 1;
On 2014/10/28 20:14:00, hidehiko wrote:
> FYI: Is this flag still needed? IIUC, this is no longer used.
> If unnecessary, I'm happy to work on its removal in a separate CL.

Yeah, it is no longer checked by open_resource() since we switched to the
Chrome-IPC-based implementation.

It would be good to remove it.  It would also be good to add a test to check
that open_resource() works on the main thread after PPAPI has been initialised. 
(In the original SRPC implementation, it didn't work, which was the reason for
checking g_is_main_thread to give a graceful error instead of deadlocking.)

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:61: static int not_pnacl_filter(void) {
How come the compiler doesn't warn about this being unused when compiling for
Non-SFI mode?  Is this actually getting compiled?  Are warnings enabled
properly?

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:81: NULL,
Maybe add a comment:  "If we change PNaCl to use Non-SFI Mode on the open web,
we should add a filter here."

[hidehiko, 2014-10-29 05:59:29]

Patchset #3 (id:70001) has been deleted

[hidehiko, 2014-10-29 06:00:14]

Thank you for review, Mark.

Justin, would you mind to review?

Regards,
- hidehiko

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:23: g_is_main_thread = 1;
On 2014/10/28 21:57:23, Mark Seaborn wrote:
> On 2014/10/28 20:14:00, hidehiko wrote:
> > FYI: Is this flag still needed? IIUC, this is no longer used.
> > If unnecessary, I'm happy to work on its removal in a separate CL.
> 
> Yeah, it is no longer checked by open_resource() since we switched to the
> Chrome-IPC-based implementation.
> 
> It would be good to remove it.  It would also be good to add a test to check
> that open_resource() works on the main thread after PPAPI has been
initialised. 
> (In the original SRPC implementation, it didn't work, which was the reason for
> checking g_is_main_thread to give a graceful error instead of deadlocking.)

Ok. +1 for adding the test. Let me look into it later. Added TODO.

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:61: static int not_pnacl_filter(void) {
On 2014/10/28 21:57:23, Mark Seaborn wrote:
> How come the compiler doesn't warn about this being unused when compiling for
> Non-SFI mode?  Is this actually getting compiled?  Are warnings enabled
> properly?

Good catch. Guarded by #ifdef.
We have -Wno-unused-function so it is expected behavior.
(pls find it in pnacl_compiler_flags in native_client/build/untrusted.gypi and
build/common.gypi, i.e. it is defined in two places).

https://codereview.chromium.org/652393003/diff/50001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:81: NULL,
On 2014/10/28 21:57:23, Mark Seaborn wrote:
> Maybe add a comment:  "If we change PNaCl to use Non-SFI Mode on the open web,
> we should add a filter here."

Done.

[teravest, 2014-10-29 16:20:08]

lgtm

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
File ppapi/nacl_irt/plugin_main.cc (right):

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
ppapi/nacl_irt/plugin_main.cc:38: // TODO(hidehiko): Make sure this SRPC is
actually used on SFI-mode.
Do you want to remove this comment about SRPC?

[Mark Seaborn, 2014-10-29 16:37:31]

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
File ppapi/nacl_irt/plugin_main.cc (right):

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
ppapi/nacl_irt/plugin_main.cc:38: // TODO(hidehiko): Make sure this SRPC is
actually used on SFI-mode.
On 2014/10/29 16:20:07, teravest wrote:
> Do you want to remove this comment about SRPC?

Having *some* TODO here is appropriate, because we do want to remove this use of
SRPC here...

[hidehiko, 2014-10-30 00:34:41]

Thank you for review. Submitting...

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
File ppapi/nacl_irt/plugin_main.cc (right):

https://codereview.chromium.org/652393003/diff/90001/ppapi/nacl_irt/plugin_ma...
ppapi/nacl_irt/plugin_main.cc:38: // TODO(hidehiko): Make sure this SRPC is
actually used on SFI-mode.
On 2014/10/29 16:37:31, Mark Seaborn wrote:
> On 2014/10/29 16:20:07, teravest wrote:
> > Do you want to remove this comment about SRPC?
> 
> Having *some* TODO here is appropriate, because we do want to remove this use
of
> SRPC here...

Yes, this is intentional. I'll look into this later.

[hidehiko, 2014-10-30 00:35:00]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-30 00:37:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652393003/90001

[commit-bot: I haz the power, 2014-10-30 01:51:20]

Committed patchset #3 (id:90001)

[commit-bot: I haz the power, 2014-10-30 01:52:13]

Patchset 3 (id:??) landed as
https://crrev.com/b0c30620cec01a6d508347537860c8e97d7a861e
Cr-Commit-Position: refs/heads/master@{#302015}