Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(7 attachments)

We can do this now probably.
I'm not completely sure about the browser.js changes. Do you think there's more we can remove there?
Attachment #8928962 - Flags: review?(andrebargull)
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
Attachment #8928962 - Flags: review?(andrebargull) → review+
There's more to clean up, both in SM and on the DOM side, but I think this is a reasonable start.
Attachment #8928972 - Flags: review?(evilpies)
Can we just do this?
Attachment #8928980 - Flags: review?(kmaglione+bmo)
Attachment #8928982 - Flags: review?(evilpies)
Depends on: 1417895
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/
Attachment #8928972 - Flags: review?(evilpies) → review+
Attachment #8928982 - Flags: review?(evilpies) → review+
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.
Attachment #8928980 - Flags: review?(kmaglione+bmo) → review+
Oops, removing the -v argument from xpcshell broke mochitests.
Attachment #8929368 - Flags: review?(jmaher)
Attachment #8929368 - Flags: review?(jmaher) → review+
Pushed by jandemooij@gmail.com:
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
More to do here, including the follow-ups kmag requested.
Keywords: leave-open
Depends on: 1418294
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.
Attachment #8929412 - Flags: review?(kmaglione+bmo)
Attachment #8929415 - Flags: review?(evilpies)
(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)?
Flags: needinfo?(kmaglione+bmo)
Attachment #8929415 - Flags: review?(evilpies) → review+
(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.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P3
Duplicate of this bug: 1412533
Duplicate of this bug: 1412532
(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.
I filed bug 1418860 to remove the content policy along with the version parameter.
See Also: → 1418860
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.
Attachment #8929412 - Flags: review?(kmaglione+bmo) → review+
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
Blocks: 1423153
https://hg.mozilla.org/mozilla-central/rev/9c1c52d5259f
https://hg.mozilla.org/mozilla-central/rev/e56181d42ce2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 867617
No longer blocks: 867617
You need to log in before you can comment on or make changes to this bug.