1417844 - Remove JSVersion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

We can do this now probably.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix shell tests

I'm not completely sure about the browser.js changes. Do you think there's more we can remove there?

Comment 2

[André Bargull [:anba]]

Comment on attachment 8928962 [details] [diff] [review]
Part 1 - Fix shell tests

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

(In reply to Jan de Mooij [:jandem] from comment #1)
> I'm not completely sure about the browser.js changes. Do you think there's
> more we can remove there?

I guess we can also remove [1,2,3] and simplify [4] to |document.write(`<script src="${src}" charset="utf-8"><\/script>`);|, because I don't think we still use any parts of the mime/language-type mechanism for jstests.

[1] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/js/src/tests/browser.js#448-451
[2] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/js/src/tests/browser.js#455-460
[3] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/js/src/tests/browser.js#541-554
[4] https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/js/src/tests/browser.js#558

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove version from CompileOptions, CompartmentBehaviors, scripts

There's more to clean up, both in SM and on the DOM side, but I think this is a reasonable start.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove version from subscript cache path

Can we just do this?

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove more code

Comment 6

[Tom S. (please needinfo tschuster)]

Comment on attachment 8928972 [details] [diff] [review]
Part 2 - Remove version from CompileOptions, CompartmentBehaviors, scripts

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

Amazing!

::: js/src/jsapi.cpp
@@ -4107,4 @@
>      : ReadOnlyCompileOptions(), elementRoot(cx), elementAttributeNameRoot(cx),
>        introductionScriptRoot(cx)
>  {
> -    this->version = (version != JSVERSION_UNKNOWN) ? version : cx->findVersion();

\o/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8928980 [details] [diff] [review]
Part 3 - Remove version from subscript cache path

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

\o/

You can also remove:

https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/mozapps/extensions/AddonContentPolicy.cpp#30,35-156
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_jsversion.html

If you don't do it here, please file a follow-up bug in Toolkit :: WebExtensions and CC me.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Don't pass -v 170 to xpcshell

Oops, removing the -v argument from xpcshell broke mochitests.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/589914e65db7
part 1 - Remove JSVersion related code from jit-tests and jsreftests. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bcd8b8a36b
part 2 - Remove JSVersion from CompileOptions, CompartmentBehaviors, scripts. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/be79373884dc
part 3 - Remove JSVersion from subscript cache path. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/017923947e58
part 4 - Remove more JSVersion code. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d567300c8f2
part 5 - Stop passing -v 170 to xpcshell. r=jmaher

Comment 10

[Jan de Mooij [:jandem]]

More to do here, including the follow-ups kmag requested.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove more JSVersion code from XPConnect

Cu.evalInSandbox has an optional 3rd version argument and unfortunately it also has some other optional arguments. This means we would have to audit all callers (there are hundreds) if we want to remove it, so this patch just keeps this argument but we no longer use it.

I can file a follow-up bug to remove it if you want.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Remove JSVersion

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #7)
> You can also remove:

Note that we will still accept |type="application/javascript;version=1.8"| in the script loader (we accept versions 1.0-1.8). That might change in the future; I think we're supposed to ignore such scripts. Considering that, shouldn't we keep this message for now to make sure people don't start using versions (even though they're no-ops right now)?

Comment 14

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #7)
> > You can also remove:
> 
> Note that we will still accept |type="application/javascript;version=1.8"|
> in the script loader (we accept versions 1.0-1.8). That might change in the
> future; I think we're supposed to ignore such scripts. Considering that,
> shouldn't we keep this message for now to make sure people don't start using
> versions (even though they're no-ops right now)?

The original reason for this content policy was to prevent extensions from using proprietary JS features and lock us into supporting them. If the version parameter is a no-op now, I'm not worried about it, since it won't mean code that works with it now will break down the road.

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/589914e65db7
https://hg.mozilla.org/mozilla-central/rev/68bcd8b8a36b
https://hg.mozilla.org/mozilla-central/rev/be79373884dc
https://hg.mozilla.org/mozilla-central/rev/017923947e58
https://hg.mozilla.org/mozilla-central/rev/5d567300c8f2

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #14)
> The original reason for this content policy was to prevent extensions from
> using proprietary JS features and lock us into supporting them. If the
> version parameter is a no-op now, I'm not worried about it, since it won't
> mean code that works with it now will break down the road.

If people write a version parameter, the script element will be ignored in the future (to align with HTML spec and other browsers), so the code will break. So the content policy should be retained until we drop support for version parameters completely.

Comment 19

[Masatoshi Kimura [:emk]]

I filed bug 1418860 to remove the content policy along with the version parameter.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8929412 [details] [diff] [review]
Part 6 - Remove more JSVersion code from XPConnect

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

I'll accept this for now, but please file a follow-up bug to remove the optional argument.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1c52d5259f
part 6 - Remove more JSVersion code from XPConnect. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56181d42ce2
part 7 - Remove JSVersion. r=evilpie

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9c1c52d5259f
https://hg.mozilla.org/mozilla-central/rev/e56181d42ce2