Parametrize names of llc and ld nexes by reading them from the resource info JSON file.

Parametrize names of llc and ld nexes by reading them from the resource info JSON file.

At Chrome build-time, the pnacl_component_crx_gen.py script generates a "resource info" JSON file that's placed alongside the translator nexes. We were not actually reading this file so far, but with this CL, we are. The first use is not hard-coding the ld.nexe and llc.nexe tool names in the plugin code. Instead, the tool names are read from the info file. This will allow us, in the future, to move the tool names into the native client directory so we can change them without touching Chrome at all.


BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202002

[jvoung (off chromium), 2013-05-23 20:33:18]

mostly good (once it compiles on windows ;-))

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.cc:21: namespace plugin {
namespace file_utils {
same, separate lines

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.h (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:14: namespace plugin {
namespace file_utils {
c++ readability: "When declaring nested namespaces, put each namespace on its
own line."

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:33: } // namespace
file_utils
2 spaces between } and // namespace

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:12: #include <fcntl.h>
does fcntl.h still need to be included?

There's still an fstat in this file, but fstat uses unistd.h.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:93: // Append what
follows files to the pnacl URL prefix.
also comment that this now adds the expected architecture prefix

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:241: // Load llc and
ld.
remove comment about llc and ld, or somehow incorporate it into the resource
info file comment below

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:165: return false;
indentation for "return false"

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:38: static const
nacl::string GetResourceInfoUrl() {
Document the resource info also.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:109: // Parses resource
info json data in buf.  Returns true if successful.
Could put pipes around argument names like |buf| and |errmsg|

[eliben, 2013-05-23 20:54:25]

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.cc:21: namespace plugin {
namespace file_utils {
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> same, separate lines

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.h (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:14: namespace plugin {
namespace file_utils {
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> c++ readability: "When declaring nested namespaces, put each namespace on its
> own line."

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:33: } // namespace
file_utils
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> 2 spaces between } and // namespace

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:12: #include <fcntl.h>
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> does fcntl.h still need to be included?
> 
> There's still an fstat in this file, but fstat uses unistd.h.

Removed

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:93: // Append what
follows files to the pnacl URL prefix.
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> also comment that this now adds the expected architecture prefix

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:241: // Load llc and
ld.
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> remove comment about llc and ld, or somehow incorporate it into the resource
> info file comment below

Changed comment, and also added below.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:165: return false;
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> indentation for "return false"

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:38: static const
nacl::string GetResourceInfoUrl() {
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> Document the resource info also.

Done.

https://codereview.chromium.org/15697019/diff/13001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:109: // Parses resource
info json data in buf.  Returns true if successful.
On 2013/05/23 20:33:19, jvoung (cr) wrote:
> Could put pipes around argument names like |buf| and |errmsg|

Done.

[jvoung (off chromium), 2013-05-23 21:15:22]

LGTM, can remove the "This is a preliminary commit, not ready for review yet"
part of your commit message =)

https://codereview.chromium.org/15697019/diff/16002/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.h (right):

https://codereview.chromium.org/15697019/diff/16002/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:26: // Slurp the whole
contents of the given file (fd - open file descriptor) into
|fd|, |out_buf|, |max_size_to_read|

[eliben, 2013-05-23 21:20:59]

https://codereview.chromium.org/15697019/diff/16002/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_utils.h (right):

https://codereview.chromium.org/15697019/diff/16002/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_utils.h:26: // Slurp the whole
contents of the given file (fd - open file descriptor) into
On 2013/05/23 21:15:22, jvoung (cr) wrote:
> |fd|, |out_buf|, |max_size_to_read|

Done.

[commit-bot: I haz the power, 2013-05-23 23:37:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eliben@chromium.org/15697019/38001

[commit-bot: I haz the power, 2013-05-24 07:15:03]

Change committed as 202002