Closed Bug 1428745 Opened 7 years ago Closed 7 years ago

Remove support for version parameter from script loader

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
58 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files)

version.patch
18.58 KB, patch
jonco
: review+
Details | Diff | Splinter Review
tests
16.72 KB, patch
jonco
: review+
Details | Diff | Splinter Review
part 3 - trimming
2.46 KB, patch
jonco
: review+
Details | Diff | Splinter Review
In bug 1418860 I added a telemetry ID to check how often version param is used. The result is that 0.02% of the scripts loaded have a version parameter. Let's remove this feature for real in this bug.
Attached patch version.patch — — Splinter Review
Assignee: nobody → amarchesini
Attachment #8940772 - Flags: review?(jcoppeard)
Comment on attachment 8940772 [details] [diff] [review] version.patch Review of attachment 8940772 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! It's going to collide with my renaming of enum values in line with the coding style, so sorry about that.
Attachment #8940772 - Flags: review?(jcoppeard) → review+
Attached patch tests — — Splinter Review
Attachment #8941027 - Flags: review?(jcoppeard)
Comment on attachment 8941027 [details] [diff] [review] tests Review of attachment 8941027 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/components/test/mochitest/test_HSplitBox_01.html @@ +9,5 @@ > <head> > <meta charset="utf-8"> > <title>Tree component test</title> > <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> Chrome and Edge trims the type attribute.
And the spec requires the trim: https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model:the-script-element-26 > Otherwise, if the script element has a type attribute, let the script block's type string for this script element be the value of that attribute with leading and trailing ASCII whitespace stripped.
(In reply to Masatoshi Kimura [:emk] from comment #5) > And the spec requires the trim: > https://html.spec.whatwg.org/multipage/scripting.html#script-processing- > model:the-script-element-26 > > Otherwise, if the script element has a type attribute, let the script block's type string for this script element be the value of that attribute with leading and trailing ASCII whitespace stripped. Good. Let's introduce this trimming here in this bug.
Attachment #8941027 - Flags: review?(jcoppeard) → review+
Attachment #8941053 - Flags: review?(jcoppeard)
Comment on attachment 8941053 [details] [diff] [review] part 3 - trimming Review of attachment 8941053 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/script/ScriptLoader.cpp @@ +1265,5 @@ > + > + // ASCII whitespace https://infra.spec.whatwg.org/#ascii-whitespace: > + // U+0009 TAB, U+000A LF, U+000C FF, U+000D CR, or U+0020 SPACE. > + static const char kASCIIWhitespace[] = "\t\n\f\r "; > + type.Trim(kASCIIWhitespace); Can we do this in HTMLScriptElement::GetScriptType() instead? I made HTMLScriptElement::FreezeExecutionAttrs() check for type="module" and this needs the trimming too.
Attachment #8941053 - Flags: review?(jcoppeard) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e53f251c5b8 Remove support for version parameter from script loader, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5dc7dfd429 Remove support for version parameter from script loader - tests, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/1d975770bd9a Remove support for version parameter from script loader - trimming script type, r=jonco
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb9af93ac53 Remove support for version parameter from script loader - fixing a broken depending test - CLOSED TREE, r=me
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e199f1ccf64c Remove support for version parameter from script loader - fixing WPTs - CLOSED TREE, r=me
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e84285278fe Remove support for version parameter from script loader - fixing tests - CLOSED TREE, r=me
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb9af93ac53 > Remove support for version parameter from script loader - fixing a broken depending test - CLOSED TREE, r=me Is this "fix" really correct? Why this patch broke this test?
Flags: needinfo?(amarchesini)
> Is this "fix" really correct? Why this patch broke this test? This test does a logging on onload but without calling waitForExplicitFinish(). Currently it works because it's the only test in that folder. Running 2 tests, triggers the failure.
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/30db4c961cc4 Remove support for version parameter from script loader, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/0908d28cbd85 Remove support for version parameter from script loader - tests, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/ce14f861f1a8 Remove support for version parameter from script loader - trimming script type, r=jonco
Shouldn't this have had an intent to unship email?
Keywords: dev-doc-needed
Blocks: 867617
No longer blocks: 867617
I've documented this, by: * Updating the note on the <script> element page: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script (see the type attribute) * Adding a note to the Fx59 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/59#HTML_2 * Adding a note to the browser compat data for script: https://github.com/mdn/browser-compat-data/pull/889
Keywords: dev-doc-needed → dev-doc-complete
Ooops, not finished. Let me know if you think it is OK. Thanks!
Flags: needinfo?(amarchesini)
It looks good to me. Thanks!
Flags: needinfo?(amarchesini)
Depends on: 1436590
Depends on: 1440338
Keywords: site-compat
Depends on: 1452996
Blocks: 1JS
Component: DOM → DOM: Core & HTML
Blocks: 1397822
Blocks: 1872837
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: