Last Comment Bug 781035 - convert more places to use the C++ compile/evaluate api
: convert more places to use the C++ compile/evaluate api
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Benjamin Peterson
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 781070
Blocks: 781104
  Show dependency treegraph
 
Reported: 2012-08-07 16:22 PDT by :Benjamin Peterson
Modified: 2012-08-27 12:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
convert (13.22 KB, patch)
2012-08-07 16:22 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
convert (14.69 KB, patch)
2012-08-07 18:51 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
use c++ api (14.76 KB, patch)
2012-08-08 13:20 PDT, :Benjamin Peterson
jorendorff: review+
jst: superreview+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-08-07 16:22:40 PDT
Created attachment 649861 [details] [diff] [review]
convert
Comment 1 Nicholas Nethercote [:njn] 2012-08-07 18:02:02 PDT
Please provide a description, even if short, in all filed bugs.  It makes things much easier for people who follow all the bugs in a particular component.  Thanks!
Comment 2 :Benjamin Peterson 2012-08-07 18:51:06 PDT
Created attachment 649925 [details] [diff] [review]
convert

I forgot one.
Comment 3 :Benjamin Peterson 2012-08-08 13:20:51 PDT
Created attachment 650274 [details] [diff] [review]
use c++ api

Remove usage of JS::NullPtr.
Comment 4 Jason Orendorff [:jorendorff] 2012-08-22 11:12:48 PDT
Comment on attachment 650274 [details] [diff] [review]
use c++ api

Review of attachment 650274 [details] [diff] [review]:
-----------------------------------------------------------------

jst, this is just an API change. The new API isn't markedly better in these cases, but it's more explicit. I've reviewed it. Please request any additional reviews that are needed.

This eliminates two casts. Do we know why those were necessary in the first place? With the patch, does this still build on Windows?

::: dom/workers/WorkerPrivate.cpp
@@ +3795,5 @@
>                                                          &stringLength);
> +      if (!string) {
> +        retval = false;
> +        break;
> +      }

The original code called JS_ReportPendingException in the case where string is null.

@@ +3805,1 @@
>            !JS_ReportPendingException(aCx)) {

And here: JS_ReportPendingException is supposed to be called when Evaluate returns false.
Comment 5 Jason Orendorff [:jorendorff] 2012-08-22 11:19:52 PDT
I should comment briefly on why this is desirable.

There are a bunch of compile-time options that affect script compilation and nothing else. Historically some of these have been stored in the JSContext, but then some JSAPI-using code just grabs a JSContext somewhat arbitrarily and starts using it, so I think historically the choice of language version, compile options, and other odds and ends has often been pretty arbitrary. This is bad. To counteract that we've been adding more and more JS_Compile* APIs, like JS_CompileUCScriptForPrincipalsVersionOrigin.

With CompileOptions, there are still a bunch of knobs (too many, I guess), but you can see all of them. They're all in one place. If you want to know exactly which options your code is going to be compiled with, just look at the CompileOptions object you've made. That's what the JS engine will use.

Compilation depends only on the CompileOptions; AFAIK it no longer depends on JSContext state in any subtle ways.

Ideally I would like to make CompileOptions itself independent of any JSContext state. This is a step in that direction without changing the semantics at all yet.
Comment 6 Nicholas Nethercote [:njn] 2012-08-22 15:57:16 PDT
[A side note about CompileOptions:  it's now passed to JSScript::Create(), which I don't particularly like, because that function only reads some of its fields -- 5 out of 11, to be exact.]
Comment 7 Jason Orendorff [:jorendorff] 2012-08-22 16:27:25 PDT
Get your own bug!  :)

Note You need to log in before you can comment on or make changes to this bug.