Closed Bug 1428745 Opened 2 years ago Closed 2 years ago

Remove support for version parameter from script loader

Categories

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

58 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 2 open bugs)

Details

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

Attachments

(3 files)

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.patchSplinter 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 testsSplinter 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
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
You need to log in before you can comment on or make changes to this bug.