Closed
Bug 1417895
Opened 6 years ago
Closed 6 years ago
Remove JSVersion from script loader
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
4.12 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.45 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•6 years ago
|
||
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 | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
I went with an enum class instead of bool, to make screw ups less likely.
Attachment #8929396 -
Flags: review?(bzbarsky)
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b6ff051a2bf https://hg.mozilla.org/mozilla-central/rev/44f3d16c0927
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•