Run Pepper unit tests in Native Client, part 1 (pepper repo)....

Run Pepper unit tests in Native Client, part 1 (pepper repo).
Changes in native_client SConstruct is part 2.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=902
TEST=load scons-out/.../ppapi_tests.html in a Pepper2-enabled Chrome browser.

[garianov, 2010-10-20 21:36:50]

new: nacl.scons + ppapi_tests.html
also, some small fixes in ppapi code, so that it biulds

[David Springer, 2010-10-20 21:57:50]

LGTM, pending Neb's comments.  There are a couple of lint errors, it would be
nice (but not necessary, imho) to clean these up.

http://codereview.chromium.org/3993001/diff/1/5
File tests/test_file_ref.cc (right):

http://codereview.chromium.org/3993001/diff/1/5#newcode68
tests/test_file_ref.cc:68: sprintf(error_as_string, "%ld", error);
Lint says this should be "snprintf()".  Since this is in a test, I don't think
it really matters.

http://codereview.chromium.org/3993001/diff/1/6
File tests/test_url_loader.cc (right):

http://codereview.chromium.org/3993001/diff/1/6#newcode58
tests/test_url_loader.cc:58: sprintf(error_as_string, "%ld", error);
Lint says this should be "snprintf()".  I don't think it really matters in a
test, though.

[neb, 2010-10-20 22:02:53]

http://codereview.chromium.org/3993001/diff/1/3
File tests/nacl.scons (right):

http://codereview.chromium.org/3993001/diff/1/3#newcode9
tests/nacl.scons:9: ppapi_tests_nexe = env.ComponentProgram('ppapi_tests.nexe',
Is there a way to wildcard this so that people don't have to modify the scons
file every time they add tests?

http://codereview.chromium.org/3993001/diff/1/7
File tests/test_var.cc (right):

http://codereview.chromium.org/3993001/diff/1/7#newcode92
tests/test_var.cc:92: pp::Var int_var(static_cast<int32_t>(42));
What was it being cast into otherwise? We made Var have a bunch of implicit
constructors for the purpose of being able to do things like obj->Call("f", 1,
true, "value"), and it would suck if Var(1) is not an int var.

[garianov, 2010-10-20 22:18:20]

http://codereview.chromium.org/3993001/diff/1/3
File tests/nacl.scons (right):

http://codereview.chromium.org/3993001/diff/1/3#newcode9
tests/nacl.scons:9: ppapi_tests_nexe = env.ComponentProgram('ppapi_tests.nexe',
On 2010/10/20 22:02:53, neb wrote:
> Is there a way to wildcard this so that people don't have to modify the scons
> file every time they add tests?

done

http://codereview.chromium.org/3993001/diff/1/5
File tests/test_file_ref.cc (right):

http://codereview.chromium.org/3993001/diff/1/5#newcode68
tests/test_file_ref.cc:68: sprintf(error_as_string, "%ld", error);
On 2010/10/20 21:57:50, David Springer wrote:
> Lint says this should be "snprintf()".  Since this is in a test, I don't think
> it really matters.

Original code is using |printf|, decided to to change.

http://codereview.chromium.org/3993001/diff/1/6
File tests/test_url_loader.cc (right):

http://codereview.chromium.org/3993001/diff/1/6#newcode58
tests/test_url_loader.cc:58: sprintf(error_as_string, "%ld", error);
On 2010/10/20 21:57:50, David Springer wrote:
> Lint says this should be "snprintf()".  I don't think it really matters in a
> test, though.

Original code is using |printf|, decided not to change.

http://codereview.chromium.org/3993001/diff/1/7
File tests/test_var.cc (right):

http://codereview.chromium.org/3993001/diff/1/7#newcode92
tests/test_var.cc:92: pp::Var int_var(static_cast<int32_t>(42));
On 2010/10/20 22:02:53, neb wrote:
> What was it being cast into otherwise? We made Var have a bunch of implicit
> constructors for the purpose of being able to do things like obj->Call("f", 1,
> true, "value"), and it would suck if Var(1) is not an int var.

here's the compiler output (without cast):

scons: Building targets ...
________Compiling scons-out/nacl-x86-32/obj/tests/ppapi_tests/test_var.o
/usr/local/google/garianov/nacl/ppapi/tests/test_var.cc: In member function
'std::string TestVar::TestInvalidAndEmpty()':
/usr/local/google/garianov/nacl/ppapi/tests/test_var.cc:92: error: call of
overloaded 'Var(int)' is ambiguous
/usr/local/google/garianov/nacl/ppapi/cpp/var.h:54: note: candidates are:
pp::Var::Var(const pp::Var&)
/usr/local/google/garianov/nacl/ppapi/cpp/var.h:27: note:                
pp::Var::Var(double)
/usr/local/google/garianov/nacl/ppapi/cpp/var.h:26: note:                
pp::Var::Var(int32_t)
/usr/local/google/garianov/nacl/ppapi/cpp/var.h:25: note:                
pp::Var::Var(bool)
scons: *** [scons-out/nacl-x86-32/obj/tests/ppapi_tests/test_var.o] Error 1
scons: building terminated because of errors.

[darin (slow to review), 2010-10-20 23:13:51]

http://codereview.chromium.org/3993001/diff/11001/12002
File tests/nacl.scons (right):

http://codereview.chromium.org/3993001/diff/11001/12002#newcode1
tests/nacl.scons:1: # Copyright (c) 2010 The Native Client Authors. All rights
reserved.
The Native Client Authors -> The Chromium Authors

That is what the LICENSE file indicates.

http://codereview.chromium.org/3993001/diff/11001/12002#newcode7
tests/nacl.scons:7: env.Prepend(CPPDEFINES=['XP_UNIX'])
why do you need to define XP_UNIX?  this smells like something required when
using NPAPI, which should not be relevant here.

http://codereview.chromium.org/3993001/diff/11001/12002#newcode21
tests/nacl.scons:21: 
gratuitous new lines?

http://codereview.chromium.org/3993001/diff/11001/12003
File tests/ppapi_tests.html (right):

http://codereview.chromium.org/3993001/diff/11001/12003#newcode7
tests/ppapi_tests.html:7: frameWin.frameElement.style.height = height;
this looks like a fork of test_case.html, and there's some non-trivial bits in
here.  it would be nicer to avoid duplicating code.  perhaps you could just
augment test_case.html with a special query parameter?

http://codereview.chromium.org/3993001/diff/11001/12006
File tests/test_var.cc (right):

http://codereview.chromium.org/3993001/diff/11001/12006#newcode92
tests/test_var.cc:92: pp::Var int_var(static_cast<int32_t>(42));
can you just do 42L to get the same result in a less verbose manner?

[neb, 2010-10-20 23:57:28]

http://codereview.chromium.org/3993001/diff/11001/12006
File tests/test_var.cc (right):

http://codereview.chromium.org/3993001/diff/11001/12006#newcode92
tests/test_var.cc:92: pp::Var int_var(static_cast<int32_t>(42));
We should really add an int overload to Var() which promotes to long, this
should work without any markup.

[garianov, 2010-10-21 17:27:12]

please have another look

http://codereview.chromium.org/3993001/diff/11001/12002
File tests/nacl.scons (right):

http://codereview.chromium.org/3993001/diff/11001/12002#newcode1
tests/nacl.scons:1: # Copyright (c) 2010 The Native Client Authors. All rights
reserved.
On 2010/10/20 23:13:52, darin wrote:
> The Native Client Authors -> The Chromium Authors
> 
> That is what the LICENSE file indicates.

Done.

http://codereview.chromium.org/3993001/diff/11001/12002#newcode7
tests/nacl.scons:7: env.Prepend(CPPDEFINES=['XP_UNIX'])
On 2010/10/20 23:13:52, darin wrote:
> why do you need to define XP_UNIX?  this smells like something required when
> using NPAPI, which should not be relevant here.

removed

http://codereview.chromium.org/3993001/diff/11001/12002#newcode21
tests/nacl.scons:21: 
On 2010/10/20 23:13:52, darin wrote:
> gratuitous new lines?

removed

http://codereview.chromium.org/3993001/diff/11001/12003
File tests/ppapi_tests.html (right):

http://codereview.chromium.org/3993001/diff/11001/12003#newcode7
tests/ppapi_tests.html:7: frameWin.frameElement.style.height = height;
On 2010/10/20 23:13:52, darin wrote:
> this looks like a fork of test_case.html, and there's some non-trivial bits in
> here.  it would be nicer to avoid duplicating code.  perhaps you could just
> augment test_case.html with a special query parameter?
Done. Changed the way test case is specified:
?testcase=TestCaseName&mode=nacl - for tests running in NaCl.

http://codereview.chromium.org/3993001/diff/11001/12006
File tests/test_var.cc (right):

http://codereview.chromium.org/3993001/diff/11001/12006#newcode92
tests/test_var.cc:92: pp::Var int_var(static_cast<int32_t>(42));
On 2010/10/20 23:57:29, neb wrote:
> We should really add an int overload to Var() which promotes to long, this
> should work without any markup.

Done.

[neb, 2010-10-21 17:43:08]

LGTM. Darin?

[garianov, 2010-10-26 17:23:05]

I had to change some code to get green trybots...
Please have another look

[darin (slow to review), 2010-10-26 23:11:52]

sorry... forgot to send these comments earlier.

http://codereview.chromium.org/3993001/diff/2002/11006
File tests/test_case.html (right):

http://codereview.chromium.org/3993001/diff/2002/11006#newcode68
tests/test_case.html:68: var nexes = "ARM: ppapi_tests_arm.nexe \nx86-32:
ppapi_tests.nexe \nx86-64: ppapi_tests_x86_64.";
i thought the plan was to switch over to pointing the <embed> at a manifest
file, which itself would contain this architecture mapping.

http://codereview.chromium.org/3993001/diff/2002/11006#newcode69
tests/test_case.html:69: obj = document.createElement("embed");
why do we need to create an <embed> tag in this case and an <object> tag in the
other case.  we could just create the same element type in both cases right?

http://codereview.chromium.org/3993001/diff/2002/11006#newcode87
tests/test_case.html:87: obj.setAttribute("mode", mode);
it seems like you might as well just set this up top after setting the "nexes"
attribute.

http://codereview.chromium.org/3993001/diff/2002/11009
File tests/testing_instance.cc (right):

http://codereview.chromium.org/3993001/diff/2002/11009#newcode26
tests/testing_instance.cc:26: bool TestingInstance::Init(uint32_t argc, const
char* argn[], const char* argv[]) {
nit: might be nice to fix this line which appears to exceed the 80 char limit

[David Springer, 2010-10-26 23:43:11]

http://codereview.chromium.org/3993001/diff/2002/11006
File tests/test_case.html (right):

http://codereview.chromium.org/3993001/diff/2002/11006#newcode68
tests/test_case.html:68: var nexes = "ARM: ppapi_tests_arm.nexe \nx86-32:
ppapi_tests.nexe \nx86-64: ppapi_tests_x86_64.";
On 2010/10/26 23:11:53, darin wrote:
> i thought the plan was to switch over to pointing the <embed> at a manifest
> file, which itself would contain this architecture mapping.

I'm working on the manifest file support right now.  We plan to continue to
support the "nexes" attrib until the manifest file stuff is ready, then we'll
deprecate it.

[darin (slow to review), 2010-10-27 04:58:16]

OK

On Tue, Oct 26, 2010 at 4:43 PM, <dspringer@google.com> wrote:

>
> http://codereview.chromium.org/3993001/diff/2002/11006
> File tests/test_case.html (right):
>
> http://codereview.chromium.org/3993001/diff/2002/11006#newcode68
> tests/test_case.html:68: var nexes = "ARM: ppapi_tests_arm.nexe
> \nx86-32: ppapi_tests.nexe \nx86-64: ppapi_tests_x86_64.";
> On 2010/10/26 23:11:53, darin wrote:
>
>> i thought the plan was to switch over to pointing the <embed> at a
>>
> manifest
>
>> file, which itself would contain this architecture mapping.
>>
>
> I'm working on the manifest file support right now.  We plan to continue
> to support the "nexes" attrib until the manifest file stuff is ready,
> then we'll deprecate it.
>
>
> http://codereview.chromium.org/3993001/show
>

[garianov, 2010-10-28 15:56:33]

please have another look

http://codereview.chromium.org/3993001/diff/2002/11006
File tests/test_case.html (right):

http://codereview.chromium.org/3993001/diff/2002/11006#newcode69
tests/test_case.html:69: obj = document.createElement("embed");
On 2010/10/26 23:11:53, darin wrote:
> why do we need to create an <embed> tag in this case and an <object> tag in
the
> other case.  we could just create the same element type in both cases right?

Done.

http://codereview.chromium.org/3993001/diff/2002/11006#newcode87
tests/test_case.html:87: obj.setAttribute("mode", mode);
On 2010/10/26 23:11:53, darin wrote:
> it seems like you might as well just set this up top after setting the "nexes"
> attribute.

Done.

http://codereview.chromium.org/3993001/diff/2002/11009
File tests/testing_instance.cc (right):

http://codereview.chromium.org/3993001/diff/2002/11009#newcode26
tests/testing_instance.cc:26: bool TestingInstance::Init(uint32_t argc, const
char* argn[], const char* argv[]) {
On 2010/10/26 23:11:53, darin wrote:
> nit: might be nice to fix this line which appears to exceed the 80 char limit

Done.

[darin (slow to review), 2010-11-01 16:12:20]

LGTM