Closed Bug 1393270 Opened 7 years ago Closed 6 years ago

Consider merging JS::CompileForNonSyntacticScope and JS::Compile

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mccr8, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:tech-debt])

Attachments

(1 file)

For some reason, the public API to compile for non syntactic scopes uses a totally different function. This adds a lot of boilerplate in jsapi.cpp, and also means we have a fair amount of code like this in the browser:

if (mScopeChain.length() == 0) {
  compiled = JS::Compile(...stuff...);
} else {
  compiled = JS::CompileForNonSyntacticScope(...stuff...);
}

Jason suggested it could instead become a compile option.
Priority: -- → P3
Whiteboard: [js:tech-debt]
This will be necessary for bug 1401372, since it's not currently possible to OMT compile for non-syntactic scopes.
Assignee: nobody → kmaglione+bmo
Blocks: 1401372
Attachment #8911001 - Flags: review?(jorendorff) → review?(shu)
Naveed, is shu still doing reviews?  If not, who can pick up this review?
Flags: needinfo?(nihsanullah)
I should note that before https://hg.mozilla.org/mozilla-central/rev/924f41548f1a94b1b8b692ee989128ff87e6c2fd (bug 1165486) we did in fact have a CompileOption for this....

Luke, is it possible you're the right reviewer for this?  ;)
Flags: needinfo?(luke)
Flags: needinfo?(nihsanullah)
Flags: needinfo?(luke)
Attachment #8911001 - Flags: review?(shu) → review?(tcampbell)
I'll look at this in the new year. I want to finish Bug 1406153 first so that we have better fuzzy over these combinations of options.
Depends on: 1406153
Comment on attachment 8911001 [details]
Bug 1393270: Make nonSyntacticScope a compile option rather than a separate API.

https://reviewboard.mozilla.org/r/182472/#review220260

Hopefully we can end this game of hot-potato. I'm okay with us moving this to an option. The off-thread use case is a strong reason, and helps us make the Gecko embeddings be less weird. I'd like to iterate on how we handle non-syntactic environments in the future, but that is outside of the scope of this bug.

r+ with nits.

::: js/src/jsapi-tests/testCompileNonSyntactic.cpp:77
(Diff revision 1)
> +    options.setNonSyntacticScope(nonSyntactic);
> +
> +    JS::RootedScript script(cx);
> +
> +
> +    // Check explicit non-syntactic compillation first to make sure it doesn't

*compilation

::: js/src/jsapi.h:4092
(Diff revision 1)
>      bool throwOnAsmJSValidationFailureOption;
>      bool forceAsync;
>      bool sourceIsLazy;
>      bool allowHTMLComments;
>      bool isProbablySystemOrAddonCode;
> +    bool nonSyntacticScope;

Let's put this on ReadOnlyCompileOptions instead (like isRunOnce flag).

::: js/src/jsapi.cpp:4174
(Diff revision 1)
>  bool
> -JS::CompileForNonSyntacticScope(JSContext* cx, const ReadOnlyCompileOptions& options,
> +JS::CompileForNonSyntacticScope(JSContext* cx, const ReadOnlyCompileOptions& optionsArg,
>                                  SourceBufferHolder& srcBuf, JS::MutableHandleScript script)
>  {
> -    return ::Compile(cx, options, ScopeKind::NonSyntactic, srcBuf, script);
> +    CompileOptions options(cx, optionsArg);
> +    return ::Compile(cx, options.setNonSyntacticScope(true), srcBuf, script);

I'd prefer the options.setNonSyntacticScope to be on it's own line.
Attachment #8911001 - Flags: review?(tcampbell) → review+
Comment on attachment 8911001 [details]
Bug 1393270: Make nonSyntacticScope a compile option rather than a separate API.

https://reviewboard.mozilla.org/r/182472/#review220260

> I'd prefer the options.setNonSyntacticScope to be on it's own line.

Agreed. I'm not sure why I did it this way.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb77622a6887b1c9873578c801425cb52149ea2
Bug 1393270: Make nonSyntacticScope a compile option rather than a separate API. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c8fbc03e5a45c30516d881835fbfae65442bf2
Bug 1393270: Follow-up: Fix style linter error again. r=bustage CLOSED TREE
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12c47f485e3
Follow-up: Fix debug test bustage. r=bustage CLOSED TREE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: