Comment on attachment 8524253 [details] [diff] [review] part 1. Remove the overloads of JS::Evaluate that don't take an rval mutable handle, and control the behavior via the JS::CompileOptions instead Review of attachment 8524253 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the Gecko bits.

Comment on attachment 8524253 [details] [diff] [review] part 1. Remove the overloads of JS::Evaluate that don't take an rval mutable handle, and control the behavior via the JS::CompileOptions instead Review of attachment 8524253 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testSourcePolicy.cpp @@ +23,5 @@ > JS::RootedFunction fun(cx); > JS::AutoObjectVector emptyScopeChain(cx); > + // But when compiling a function we don't want to use no-rval > + // mode, since it's not supported for functions. > + opts.setNoScriptRval(false); This derives from MOZ_ASSERT(!bce->script->noScriptRval()); in JSScript::fullyInitFromEmitter, correct? ::: js/src/jsapi.cpp @@ +4825,5 @@ > SourceBufferHolder srcBuf(chars, length, SourceBufferHolder::NoOwnership); > return ::Evaluate(cx, obj, optionsArg, srcBuf, rval); > } > > +extern JS_PUBLIC_API(bool) Mild preference for no extern on this, and seeing it only on the declaration.

Comment on attachment 8524255 [details] [diff] [review] part 2. Kill off JS_Evaluate(UC)Script Review of attachment 8524255 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testJSEvaluateScript.cpp @@ +14,5 @@ > static const char src[] = "var x = 5;"; > > JS::RootedValue retval(cx); > + JS::CompileOptions opts(cx); > + CHECK(JS::Evaluate(cx, obj, opts.setFileAndLine(__FILE__, __LINE__), Bleh, I wish we could do this without the setFAL call in the method-call. ::: js/src/jsapi.h @@ +4030,1 @@ > * for each context in the application, if you pass parented objects as the obj Just above this line is a claim that embedders should ContextOptionsRef(cx).setVarObjFix(true). Please fix this to RuntimeOptionsRef(rt).setVarObjFix(true). @@ +4030,5 @@ > * for each context in the application, if you pass parented objects as the obj > * parameter, or may ever pass such objects in the future. > * > * Why a runtime option? The alternative is to add six or so new API entry > * points with signatures matching the following six, and that doesn't seem "six" is out of date. Maybe "...to add APIs duplicating those below, for the other varobjfix sense, and that doesn't seem..." or so. @@ +4039,1 @@ > * etc., entry points. "The RuntimeOptionsRef adjustment, OTOH, ..." Should rewrap the etc. bit while you're changing here. ::: js/src/shell/js.cpp @@ +2625,5 @@ > JS_ReportError(cx, "Invalid scope argument to evalcx"); > return false; > } > + JS::CompileOptions opts(cx); > + opts.setFileAndLine(filename.get(), lineno); Hmm. It'd be nice if we could make this take a Move(filename) or something, but I guess the signatures are all off. Never mind for now.

> This derives from MOZ_ASSERT(!bce->script->noScriptRval()); in > JSScript::fullyInitFromEmitter, correct? Actually, from the same assert in EmitStatement in the bce->sc->isFunctionBox() case. But yes, same idea. > Mild preference for no extern on this, and seeing it only on the declaration. Will do. > Bleh, I wish we could do this without the setFAL call in the method-call. We can if we're willing to have our line be off-by-one... I wasn't sure which would be better and went with preserving behavior. > Please fix this to RuntimeOptionsRef(rt).setVarObjFix(true). Will do. > Maybe "...to add APIs duplicating those below, for the other varobjfix sense, and that > doesn't seem..." or so. Will do. > Should rewrap the etc. bit while you're changing here. OK.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ccb58311755 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f143345374

https://hg.mozilla.org/mozilla-central/rev/2ccb58311755 https://hg.mozilla.org/mozilla-central/rev/c8f143345374