Closed Bug 1417895 Opened 2 years ago Closed 2 years ago

Remove JSVersion from script loader

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

In bug 1417844 we want to remove JSVersion.

The script loader calls ParseJavascriptVersion here:

https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/script/ScriptLoader.cpp#1194

With versions gone from SM, I think the only effect this has is on this code:

https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#2149-2152

AFAICS, this means we execute:

  <script type="text/javascript;version=1.8">

But not scripts with an unknown version number like 1.9.

We could replace the JSVersion here with a |bool unknownVersion| to keep the current behavior.. What should we do, Boris?
Flags: needinfo?(bzbarsky)
Replacing with a bool unknownVersion for now is fine.

Longer term, we should consider implementing the HTML spec here, which would mean <script type="text/javascript;version=1.8"> would not execute either.  Neither would any other type string that has stuff after the actual MIME type...  Before we do that we'd need to eliminate all such uses from our own tree and check what other browsers do.
Flags: needinfo?(bzbarsky)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8929394 - Flags: review?(bzbarsky)
I went with an enum class instead of bool, to make screw ups less likely.
Attachment #8929396 - Flags: review?(bzbarsky)
Comment on attachment 8929394 [details] [diff] [review]
Part 1 - Move ParseJavascriptVersion from nsContentUtils to ScriptLoader.cpp

r=me
Attachment #8929394 - Flags: review?(bzbarsky) → review+
Comment on attachment 8929396 [details] [diff] [review]
Part 2 - Use a ValidJSVersion enum instead of JSVersion

>   // To avoid decoding issues, the JSVersion is explicitly guarded here, and the

So have we already moved to a place where script serialization/deserialization is version-independent?  That seems like a prereq here.

r=me if so.
Attachment #8929396 - Flags: review?(bzbarsky) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6ff051a2bf
part 1 - Move ParseJavascriptVersion from nsContentUtils to ScriptLoader.cpp. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f3d16c0927
part 2 - Use a ValidJSVersion enum instead of JSVersion in script loader. r=bz
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> So have we already moved to a place where script
> serialization/deserialization is version-independent?  That seems like a
> prereq here.

Yep, I already landed patches for bug 1417844 that remove JSVersion from CompileOptions, scripts, etc.
https://hg.mozilla.org/mozilla-central/rev/2b6ff051a2bf
https://hg.mozilla.org/mozilla-central/rev/44f3d16c0927
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.