Set up NaClChromeMainArgs number_of_cores member so apps can size threadpools appropriately

Set up NaClChromeMainArgs number_of_cores member so apps can size threadpools appropriately

The outer sandbox on Linux and OSX was preventing
sysconf(_SC_NPROCESSORS_ONLN) from succeeding, so that NaCl
applications that need the number of threads were getting a
bogus value (-1 when using newlib, and 1 when using glibc).

TEST= run_sysconf_nprocessors_onln_test
BUG=176522
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195190

[bsy, 2013-04-16 17:42:35]

Please take a look

[jvoung (off chromium), 2013-04-16 18:37:38]

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_listener.h
File chrome/nacl/nacl_listener.h (right):

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_listener.h#n...
chrome/nacl/nacl_listener.h:37: #if defined(OS_LINUX) || defined(OS_MACOSX)
Perhaps leave a comment that this is isn't needed for windows because of the
difference in sandbox?

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_main.cc#newc...
chrome/nacl/nacl_main.cc:48: listener.set_number_of_cores(number_of_cores);
Still looking through the CL, but why do you have
listener.set_number_of_cores(x) here, and also call sysconf in the NaClListener
ctor?  Is the one in the ctor unnecessary?

https://codereview.chromium.org/14238013/diff/1/ppapi/native_client/tests/nac...
File ppapi/native_client/tests/nacl_browser/sysconf_nprocessors/nacl.scons
(right):

https://codereview.chromium.org/14238013/diff/1/ppapi/native_client/tests/nac...
ppapi/native_client/tests/nacl_browser/sysconf_nprocessors/nacl.scons:37:
is_broken=(env.PPAPIBrowserTesterIsBroken() or
Is it possible to frame this test within the NaClBrowserTest framework instead? 
See chrome/test/nacl/ for the driver + chrome/test/data/nacl/ and for setting up
the html + nexe/pexe compiles.

For example, the exit status test has been moved there.

We are trying to move things out of "nacl_integration", since the normal
"browser_test" infrastructure has better sharding and such.  We also stopped
testing PNaCl under nacl_integration, and only test via browser_test nowadays.

[bsy, 2013-04-17 15:05:37]

PTAL

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_listener.h
File chrome/nacl/nacl_listener.h (right):

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_listener.h#n...
chrome/nacl/nacl_listener.h:37: #if defined(OS_LINUX) || defined(OS_MACOSX)
On 2013/04/16 18:37:38, jvoung (cr) wrote:
> Perhaps leave a comment that this is isn't needed for windows because of the
> difference in sandbox?

Done.

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/1/chrome/nacl/nacl_main.cc#newc...
chrome/nacl/nacl_main.cc:48: listener.set_number_of_cores(number_of_cores);
Nope.  One in ctor is superfluous and unnecessary, especially since nacl_main.cc
declares its NaClListener object after the sandbox has been enabled.  Removed.

https://codereview.chromium.org/14238013/diff/1/ppapi/native_client/tests/nac...
File ppapi/native_client/tests/nacl_browser/sysconf_nprocessors/nacl.scons
(right):

https://codereview.chromium.org/14238013/diff/1/ppapi/native_client/tests/nac...
ppapi/native_client/tests/nacl_browser/sysconf_nprocessors/nacl.scons:37:
is_broken=(env.PPAPIBrowserTesterIsBroken() or
On 2013/04/16 18:37:38, jvoung (cr) wrote:
> Is it possible to frame this test within the NaClBrowserTest framework
instead? 
> See chrome/test/nacl/ for the driver + chrome/test/data/nacl/ and for setting
up
> the html + nexe/pexe compiles.
> 
> For example, the exit status test has been moved there.
> 
> We are trying to move things out of "nacl_integration", since the normal
> "browser_test" infrastructure has better sharding and such.  We also stopped
> testing PNaCl under nacl_integration, and only test via browser_test nowadays.

Done.

[jvoung (off chromium), 2013-04-17 17:07:18]

Thanks for porting the test!  Looks pretty good.  A couple of nits.  You might
also need to find an OWNER review for chrome/test/nacl/.

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:10: #if defined(OS_LINUX) || defined(OS_MACOSX)
Can this just be #if defined(OS_POSIX) like the others?

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_main.cc#...
chrome/nacl/nacl_main.cc:36: #if defined(OS_LINUX) || defined(OS_MACOSX)
OS_POSIX in this file too?

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(left):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:10:
#include <string>
Why not #include <string> if you are still using std::string below?

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:60:
std::string reply;
Is this reply variable actually used, or is "sb" the only thing used?

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html
(right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html:23:
'THIS TEST CANNOT RUN STANDALONE -- run using scons instead'});
"run using scons" -> "run using browser_test" ?

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html:28:
this.removeEventListener('message', arguments.callee, false);
hmm wonder where "arguments" comes from?

https://codereview.chromium.org/14238013/diff/31001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest.cc:91: NACL_BROWSER_TEST_F(NaClBrowserTest,
SysconfNprocessorsOnln, {
You might not need to "MAYBE_${testname}", but I can't guarantee it ;-)  The
other tests had "MAYBE" to disable it under Linux ASAN, but it might actually
work under asan nowadays.

[bsy, 2013-04-17 20:55:21]

+ncbray

please take a look

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:10: #if defined(OS_LINUX) || defined(OS_MACOSX)
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> Can this just be #if defined(OS_POSIX) like the others?

Done.

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/nacl/nacl_main.cc#...
chrome/nacl/nacl_main.cc:36: #if defined(OS_LINUX) || defined(OS_MACOSX)
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> OS_POSIX in this file too?

Done.

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(left):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:10:
#include <string>
string is included on line 15

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:60:
std::string reply;
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> Is this reply variable actually used, or is "sb" the only thing used?

Done.

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html
(right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html:23:
'THIS TEST CANNOT RUN STANDALONE -- run using scons instead'});
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> "run using scons" -> "run using browser_test" ?

Done.

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.html:28:
this.removeEventListener('message', arguments.callee, false);
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> hmm wonder where "arguments" comes from?

cut-n-pasted code from other tests.  it appears to be deprecated, but still
works when not in strict mode.  since we don't name the function, this is the
"right" way to get the function.  since we can use ECMAScript 3 feature to name
function expressions, i'll change this to use named function expressions but
leave all other tests that use arguments.callee untouched.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Functions_and_f...

https://codereview.chromium.org/14238013/diff/31001/chrome/test/nacl/nacl_bro...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/nacl/nacl_bro...
chrome/test/nacl/nacl_browsertest.cc:91: NACL_BROWSER_TEST_F(NaClBrowserTest,
SysconfNprocessorsOnln, {
On 2013/04/17 17:07:18, jvoung (cr) wrote:
> You might not need to "MAYBE_${testname}", but I can't guarantee it ;-)  The
> other tests had "MAYBE" to disable it under Linux ASAN, but it might actually
> work under asan nowadays.

Done.

[Nick Bray (chromium), 2013-04-18 00:24:18]

LGTM w/ simplification.

https://codereview.chromium.org/14238013/diff/34003/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/34003/chrome/nacl/nacl_main.cc#...
chrome/nacl/nacl_main.cc:37: int number_of_cores =
sysconf(_SC_NPROCESSORS_ONLN);
Why not inline?  The skew between the preprocessor conditions is a little funky
and should be made uniform, if you don't inline.

[jvoung (off chromium), 2013-04-18 01:43:18]

LGTM

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(left):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:10:
#include <string>
On 2013/04/17 20:55:21, bsy wrote:
> string is included on line 15

Isn't line 15 <cstring> instead of <string> ?

[bsy, 2013-04-18 03:25:06]

thx

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
File
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc
(left):

https://codereview.chromium.org/14238013/diff/31001/chrome/test/data/nacl/sys...
chrome/test/data/nacl/sysconf_nprocessors_onln/sysconf_nprocessors_onln_test.cc:10:
#include <string>
you're right.  moved to be after C headers.

https://codereview.chromium.org/14238013/diff/34003/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (right):

https://codereview.chromium.org/14238013/diff/34003/chrome/nacl/nacl_main.cc#...
chrome/nacl/nacl_main.cc:37: int number_of_cores =
sysconf(_SC_NPROCESSORS_ONLN);
unskewed.  added comment explaining that this has to be before EnableSandbox().

[commit-bot: I haz the power, 2013-04-18 16:54:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsy@google.com/14238013/48001

[commit-bot: I haz the power, 2013-04-18 17:53:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 19:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bsy@google.com/14238013/48001

[commit-bot: I haz the power, 2013-04-19 14:46:29]

Change committed as 195190