Pepper: Move PnaclOptions outside trusted plugin.

Pepper: Move PnaclOptions outside trusted plugin.

I ran into this while moving the Manifest code out of the trusted plugin and
figured this was big enough to stand on its own.

We have to create a PP_PNaClOptions struct instead of PnaclOptions so that the
objects of that type can be passed to functions in PPB_NaCl_Private.

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

[dmichael (off chromium), 2014-04-15 23:15:03]

lgtm

https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:384: void
GrabUrlAndPP_PNaClOptions(const Json::Value& url_spec,
I think you can keep the old name...  this looks awkward, and PP_ is kind of
like a namespace

https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:395: opt_level = 2;
Maybe duplicate the comment from pnacl_options.cc, since this looks wrong
without that context?

https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:34:
result.push_back('\x00');
This all looks a little more complicated than it needs to be. At the call-site,
this is all turned in to a string. I know you just copied it, so don't do this
if you don't want to. But we could just use a string return type, and it would
look more like...

nacl::string str;  // (not sure why nacl and not std, but OK)
nacl::stringstream ss;
ss << "-O" << opt_level;
str = ss.str() + '\x00';
if (is_debug)
  str += "-bitcode-format=llvm" + '\x00';

return str;

[teravest, 2014-04-16 15:50:56]

On Tue, Apr 15, 2014 at 5:15 PM,  <dmichael@chromium.org> wrote:
> lgtm
>
>
>
>
>
https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):
>
>
https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/json_manifest.cc:384: void
> GrabUrlAndPP_PNaClOptions(const Json::Value& url_spec,
> I think you can keep the old name...  this looks awkward, and PP_ is
> kind of like a namespace

Thanks, I didn't see that happen in the search-and-replace.

>
>
https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/json_manifest.cc:395: opt_level =
> 2;
> Maybe duplicate the comment from pnacl_options.cc, since this looks
> wrong without that context?

Done.

>
>
https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> (right):
>
>
https://codereview.chromium.org/235983020/diff/40001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:34:
> result.push_back('\x00');
> This all looks a little more complicated than it needs to be. At the
> call-site, this is all turned in to a string. I know you just copied it,
> so don't do this if you don't want to. But we could just use a string
> return type, and it would look more like...
>
> nacl::string str;  // (not sure why nacl and not std, but OK)
> nacl::stringstream ss;
> ss << "-O" << opt_level;
> str = ss.str() + '\x00';
> if (is_debug)
>   str += "-bitcode-format=llvm" + '\x00';
>
> return str;

Done. That's much better.
Presumably someone was concerned about using null characters in strings?
I've kept using nacl::string here instead of std::string for consistency.

>
> https://codereview.chromium.org/235983020/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-04-16 16:05:06]

Thanks, still lgtm!

https://codereview.chromium.org/235983020/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/235983020/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:28: str +=
"-bitcode-format=llvm" + '\x00';
I realized you might want to wrap one of these with std::string, or you're
likely to get pointer arithmetic instead of string concatenation. (Yay C++!)

Sorry about that.

[teravest, 2014-04-16 16:11:12]

On Wed, Apr 16, 2014 at 10:05 AM,  <dmichael@chromium.org> wrote:
> Thanks, still lgtm!
>
>
>
https://codereview.chromium.org/235983020/diff/60001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> (right):
>
>
https://codereview.chromium.org/235983020/diff/60001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:28: str
> += "-bitcode-format=llvm" + '\x00';
> I realized you might want to wrap one of these with std::string, or
> you're likely to get pointer arithmetic instead of string concatenation.
> (Yay C++!)

Good catch.

>
> Sorry about that.
>
> https://codereview.chromium.org/235983020/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-04-16 18:16:26]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-04-16 18:17:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/235983020/100001

[commit-bot: I haz the power, 2014-04-16 19:47:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-16 19:47:34]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel
  tryserver.chromium on win_chromium_rel

[teravest, 2014-04-16 20:10:29]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-04-16 20:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/235983020/120001

[commit-bot: I haz the power, 2014-04-17 00:25:51]

Change committed as 264365