[gn] Use nacl_test_data template for PPAPI tests

[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684

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

[Petr Hosek, 2016-05-06 23:02:22]

phosek@chromium.org changed reviewers:
+ bbudge@chromium.org, dpranke@chromium.org, mcgrathr@chromium.org

[Petr Hosek, 2016-05-06 23:03:03]

Description was changed from

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684
==========

to

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684
==========

[Roland McGrath, 2016-05-06 23:07:24]

lgtm

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
File ppapi/native_client/nacl_test_data.gni (right):

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:32: #   destination_dir: the output
directory relative to the $root_build_dir
Say that it's optional and what omitting it means.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:45: if (defined(invoker.destination_dir))
{
This needs a comment about why the cases differ.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:82: if (defined(invoker.destination_dir))
{
It would be cleaner if this can be moved up to join with the
if(defiend(invoker.destination_dir)) that sets target_dir.  Then the comment
explaining the target_dir and name suffix differences can be a single comment.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:104: "defines",
alphasort the list

[Petr Hosek, 2016-05-06 23:21:06]

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
File ppapi/native_client/nacl_test_data.gni (right):

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:32: #   destination_dir: the output
directory relative to the $root_build_dir
On 2016/05/06 23:07:23, Roland McGrath wrote:
> Say that it's optional and what omitting it means.

Done.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:45: if (defined(invoker.destination_dir))
{
On 2016/05/06 23:07:23, Roland McGrath wrote:
> This needs a comment about why the cases differ.

Done.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:82: if (defined(invoker.destination_dir))
{
On 2016/05/06 23:07:23, Roland McGrath wrote:
> It would be cleaner if this can be moved up to join with the
> if(defiend(invoker.destination_dir)) that sets target_dir.  Then the comment
> explaining the target_dir and name suffix differences can be a single comment.

Done.

https://codereview.chromium.org/1955173002/diff/20001/ppapi/native_client/nac...
ppapi/native_client/nacl_test_data.gni:104: "defines",
On 2016/05/06 23:07:23, Roland McGrath wrote:
> alphasort the list

Done.

[Dirk Pranke, 2016-05-06 23:28:36]

I'm holding off on reviewing this because I don't yet understand how these
things are supposed to work; see my comments on the bug.

[bbudge, 2016-05-06 23:36:29]

Rubber stamp LGTM so I can get out of the way on this.

[Petr Hosek, 2016-05-06 23:39:39]

Patchset #4 (id:60001) has been deleted

[Dirk Pranke, 2016-05-07 01:06:52]

lgtm.

[Petr Hosek, 2016-05-09 06:36:55]

Just an update, in the original version of the patch, I omitted the pexe
pre-translation which was being used for PPAPI tests as we thought with Roland
that this just an optimization and isn't necessarily needed. Well, turned out
it's actually needed because the HTML test files used by PPAPI tests use
application/x-nacl MIME type for all variants. The latest version of the patch
fixes the failure which was observed when running the patch on trybots by
optionally supporting the pre-translation feature (which was already support by
nacl_test_data for Non-SFI, I just slightly extended it).

[Petr Hosek, 2016-05-09 06:37:15]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2016-05-09 06:37:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mcgrathr@chromium.org, bbudge@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1955173002/#ps100001
(title: "Allow pre-translating pexe to nexe")

[commit-bot: I haz the power, 2016-05-09 06:37:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955173002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1955173002/100001

[commit-bot: I haz the power, 2016-05-09 07:03:58]

Description was changed from

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684
==========

to

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684
==========

[commit-bot: I haz the power, 2016-05-09 07:03:59]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-05-09 07:05:34]

Description was changed from

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684
==========

to

==========
[gn] Use nacl_test_data template for PPAPI tests

The nacl_test_data template functionality largely overlaps with the
build logic for PPAPI tests. By replacing that logic with nacl_test_data
template, any future changes, like correctly handling Windows build,
can be isolated to that template.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=609684

Committed: https://crrev.com/e19dffc8f0896799ea782c0acba651ef39fc6d73
Cr-Commit-Position: refs/heads/master@{#392288}
==========

[commit-bot: I haz the power, 2016-05-09 07:05:35]

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