Closed
Bug 1393270
Opened 7 years ago
Closed 6 years ago
Consider merging JS::CompileForNonSyntacticScope and JS::Compile
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
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.
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8911001 -
Flags: review?(jorendorff) → review?(shu)
Comment 3•6 years ago
|
||
Naveed, is shu still doing reviews? If not, who can pick up this review?
Flags: needinfo?(nihsanullah)
Comment 4•6 years ago
|
||
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)
Updated•6 years ago
|
Flags: needinfo?(nihsanullah)
Flags: needinfo?(luke)
Attachment #8911001 -
Flags: review?(shu) → review?(tcampbell)
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb77622a6887b1c9873578c801425cb52149ea2 Bug 1393270: Make nonSyntacticScope a compile option rather than a separate API. r=tcampbell
Assignee | ||
Comment 9•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/39aaf089793437c9db28f5f1344c5aad8fbf0e8e Bug 1393270: Follow-up: Fix style linter error. r=bustage
Assignee | ||
Comment 10•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15aab71cf2e6baaaf9bc12110f8e17b65c9b7077 Bug 1393270: Follow-up: Fix build bustage from rebase. r=bustage
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c8fbc03e5a45c30516d881835fbfae65442bf2 Bug 1393270: Follow-up: Fix style linter error again. r=bustage CLOSED TREE
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12c47f485e3d661fc693be3113ad79ea186be64 Bug 1393270: Follow-up: Fix debug test bustage. r=bustage CLOSED TREE
Comment 13•6 years ago
|
||
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
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0bb77622a688 https://hg.mozilla.org/mozilla-central/rev/39aaf0897934 https://hg.mozilla.org/mozilla-central/rev/15aab71cf2e6 https://hg.mozilla.org/mozilla-central/rev/45c8fbc03e5a https://hg.mozilla.org/mozilla-central/rev/a12c47f485e3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
status-firefox57:
fix-optional → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•