Enable PPAPINaCl GLIBC browser_tests.

Enable PPAPINaCl GLIBC browser_tests.

NOTE: NaCl CL to update untrusted.gypi must go in first.

BUG=127699
TEST=this is a test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140786

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140917

[Brad Chen, 2012-05-18 17:13:18]

PTAL

[Mark Seaborn, 2012-05-18 17:30:43]

Drive-by review...

http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/cr...
File native_client_sdk/src/tools/create_nmf.py (right):

http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/cr...
native_client_sdk/src/tools/create_nmf.py:266: # Insure that the nexe name is
relative, not absolute.
'Ensure'?

http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/cr...
native_client_sdk/src/tools/create_nmf.py:267: # We assume that the nexe and the
correspodning nmf file are
'corresponding'

[Brad Chen, 2012-05-18 22:08:36]

Noel I'd be happy to make these changes if you'd like. Let me know. If you
prefer I minimize my changes to create_nmf that's fine too.

Brad

On Fri, May 18, 2012 at 10:30 AM, <mseaborn@chromium.org> wrote:

> Drive-by review...
>
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py>
> File native_client_sdk/src/tools/**create_nmf.py (right):
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py#newcode266<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py#newcode266>
> native_client_sdk/src/tools/**create_nmf.py:266: # Insure that the nexe
> name is relative, not absolute.
> 'Ensure'?
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py#newcode267<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py#newcode267>
> native_client_sdk/src/tools/**create_nmf.py:267: # We assume that the nexe
> and the correspodning nmf file are
> 'corresponding'
>
>
http://codereview.chromium.**org/10409032/<http://codereview.chromium.org/104...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Brad Chen, 2012-05-18 22:08:38]

Noel I'd be happy to make these changes if you'd like. Let me know. If you
prefer I minimize my changes to create_nmf that's fine too.

Brad

On Fri, May 18, 2012 at 10:30 AM, <mseaborn@chromium.org> wrote:

> Drive-by review...
>
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py>
> File native_client_sdk/src/tools/**create_nmf.py (right):
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py#newcode266<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py#newcode266>
> native_client_sdk/src/tools/**create_nmf.py:266: # Insure that the nexe
> name is relative, not absolute.
> 'Ensure'?
>
> http://codereview.chromium.**org/10409032/diff/1/native_**
>
client_sdk/src/tools/create_**nmf.py#newcode267<http://codereview.chromium.org/10409032/diff/1/native_client_sdk/src/tools/create_nmf.py#newcode267>
> native_client_sdk/src/tools/**create_nmf.py:267: # We assume that the nexe
> and the correspodning nmf file are
> 'corresponding'
>
>
http://codereview.chromium.**org/10409032/<http://codereview.chromium.org/104...
>

[Brad Chen, 2012-05-18 22:12:01]

Updated as per discussion with Noel and Brad Nelson.

PTAL

[noelallen1, 2012-05-18 23:08:23]

1 naming issue otherwise LGTM.

http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html
File ppapi/tests/test_case.html (right):

http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html#...
ppapi/tests/test_case.html:202: obj.setAttribute("src", "test_case.nmf");
Move this file and make the name consistent here?
   ppapi_nacl_tests_newlib.nmf

[Brad Chen, 2012-05-19 00:10:01]

Thanks. Try jobs and commit must wait until the nacl-side review is committed +
DEPS roll.

On 2012/05/18 23:08:23, noelallen1 wrote:
> 1 naming issue otherwise LGTM.
> 
> http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html
> File ppapi/tests/test_case.html (right):
> 
>
http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html#...
> ppapi/tests/test_case.html:202: obj.setAttribute("src", "test_case.nmf");
> Move this file and make the name consistent here?
>    ppapi_nacl_tests_newlib.nmf

[Brad Chen, 2012-05-19 00:10:24]

http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html
File ppapi/tests/test_case.html (right):

http://codereview.chromium.org/10409032/diff/8001/ppapi/tests/test_case.html#...
ppapi/tests/test_case.html:202: obj.setAttribute("src", "test_case.nmf");
On 2012/05/18 23:08:25, noelallen1 wrote:
> Move this file and make the name consistent here?
>    ppapi_nacl_tests_newlib.nmf

Done.

[Brad Chen, 2012-05-21 23:31:25]

Noel has already reviewed this but I see I also need a PPAPI reviewer. This CL
can't land until the other CL I sent you, then a DEPS roll to pick up a
NaCl-side update to native_client/build/untrusted.gypi.

Once I can do the DEPS roll I will do some try jobs.

[dmichael (off chromium), 2012-05-22 00:04:43]

http://codereview.chromium.org/10409032/diff/7002/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/10409032/diff/7002/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:370: }
Would it make sense to keep PPAPINaClTest with SetUpCommandLine, since the code
is the same? Then the Newlib and GLibc specializations need only override
BuildQuery, and we won't have so much copy/paste.

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
File native_client_sdk/src/tools/create_nmf.py (right):

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
native_client_sdk/src/tools/create_nmf.py:266: # Insure that the nexe name is
relative, not absolute.
Insure->Ensure?

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
native_client_sdk/src/tools/create_nmf.py:267: # We assume that the nexe and the
correspodning nmf file are
corresponding?

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp
File ppapi/ppapi_untrusted.gyp (right):

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp#n...
ppapi/ppapi_untrusted.gyp:44: 'nmf%': '<(PRODUCT_DIR)/>(nexe_target)_glibc.nmf',
maybe this should be named nmf_glibc% or something, since it's only used for the
glibc tests

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp#n...
ppapi/ppapi_untrusted.gyp:82: 'action_name': 'Generate NMF file and copy libs',
nit: It looks like you have 4-space indent instead of 2-space for 81->97

[Brad Chen, 2012-05-22 18:35:00]

I'm still need to roll NaCl DEPS before I can invoke the trybots.

http://codereview.chromium.org/10409032/diff/7002/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/10409032/diff/7002/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:370: }
On 2012/05/22 00:04:43, dmichael wrote:
> Would it make sense to keep PPAPINaClTest with SetUpCommandLine, since the
code
> is the same? Then the Newlib and GLibc specializations need only override
> BuildQuery, and we won't have so much copy/paste.

Done.

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
File native_client_sdk/src/tools/create_nmf.py (right):

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
native_client_sdk/src/tools/create_nmf.py:266: # Insure that the nexe name is
relative, not absolute.
On 2012/05/22 00:04:43, dmichael wrote:
> Insure->Ensure?

Done.

http://codereview.chromium.org/10409032/diff/7002/native_client_sdk/src/tools...
native_client_sdk/src/tools/create_nmf.py:267: # We assume that the nexe and the
correspodning nmf file are
On 2012/05/22 00:04:43, dmichael wrote:
> corresponding?

Done.

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp
File ppapi/ppapi_untrusted.gyp (right):

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp#n...
ppapi/ppapi_untrusted.gyp:44: 'nmf%': '<(PRODUCT_DIR)/>(nexe_target)_glibc.nmf',
On 2012/05/22 00:04:43, dmichael wrote:
> maybe this should be named nmf_glibc% or something, since it's only used for
the
> glibc tests

Done.

http://codereview.chromium.org/10409032/diff/7002/ppapi/ppapi_untrusted.gyp#n...
ppapi/ppapi_untrusted.gyp:82: 'action_name': 'Generate NMF file and copy libs',
On 2012/05/22 00:04:43, dmichael wrote:
> nit: It looks like you have 4-space indent instead of 2-space for 81->97

Done.

[dmichael (off chromium), 2012-05-22 19:31:00]

lgtm

http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
chrome/test/ui/ppapi_uitest.cc:335: PPAPINaClTest::PPAPINaClTest() {
optional: You can omit these empty default constructors if you wish.

[Brad Chen, 2012-05-22 20:56:55]

Thanks for mentioning that. It's fun to make code smaller.

On 2012/05/22 19:31:00, dmichael wrote:
> lgtm
> 
>
http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
> File chrome/test/ui/ppapi_uitest.cc (right):
> 
>
http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
> chrome/test/ui/ppapi_uitest.cc:335: PPAPINaClTest::PPAPINaClTest() {
> optional: You can omit these empty default constructors if you wish.

[Brad Chen, 2012-05-22 20:58:15]

http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/10409032/diff/12001/chrome/test/ui/ppapi_uites...
chrome/test/ui/ppapi_uitest.cc:335: PPAPINaClTest::PPAPINaClTest() {
On 2012/05/22 19:31:01, dmichael wrote:
> optional: You can omit these empty default constructors if you wish.

Done.

[Brad Chen, 2012-06-05 23:34:53]

Brad would you like to take one last look at ppapi_untrusted.gypi? Final changes
are to adapt to native_client/build/untrusted.gypi changes, and fix an NMF
generation bug on Windows (don't use lib64 symlink).

[commit-bot: I haz the power, 2012-06-06 15:20:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bradchen@google.com/10409032/43003

[commit-bot: I haz the power, 2012-06-06 17:41:24]

Change committed as 140786

[Brad Chen, 2012-06-06 18:46:06]

On 2012/06/06 17:41:24, I haz the power (commit-bot) wrote:
> Change committed as 140786

Reverted at r140790. Broke build on ARM.

[commit-bot: I haz the power, 2012-06-06 23:37:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bradchen@google.com/10409032/45003

[commit-bot: I haz the power, 2012-06-07 01:10:08]

Change committed as 140917