Because we stopped using legacy generators and for-each, we can remove useless version parameters from the type attribute of script elements. We have a more aggressive reason to do this. ESLint ignores scripts in versioned JavaScripts. Currently many ESLint errors are missed due to version parameters: https://public-artifacts.taskcluster.net/OSbtfzNrQtqOpFYTY7tRHg/0/public/logs/live_backing.log So version parameters are not only useless but also harmful. Let's kill them.

Comment on attachment 8840860 [details] Bug 1342144 - Fix ESLint errors in browser/. https://reviewboard.mozilla.org/r/115294/#review116716 ::: browser/base/content/test/general/healthreport_testRemoteCommands.html:79 (Diff revision 1) > } else { > writeDiagnostic("Telemetry ping " + i + " matches."); > } > } > > - return true; > + return valid; Was this a bug in the test code? Maybe it should land in a separate changeset. ::: browser/base/content/test/general/healthreport_testRemoteCommands.html:181 (Diff revision 1) > - validateResponse: function(payload) { > + /* eslint no-shadow: 0 */ > + validateResponse(payload) { Rename the argument? ::: browser/components/originattributes/test/mochitest/test_permissions_api.html:178 (Diff revision 1) > - iframe.src = 'file_empty.html'; > + iframe.src = "file_empty.html"; > iframe.onload = () => resolve(iframe.contentWindow); > document.body.appendChild(iframe); > }); > } > - debugger; > + Can you explain why this was here in the first place?

Comment on attachment 8840860 [details] Bug 1342144 - Fix ESLint errors in browser/. https://reviewboard.mozilla.org/r/115294/#review116716 > Was this a bug in the test code? Maybe it should land in a separate changeset. Yes, I noticed because ESLint complained that |valid| is unused. > Rename the argument? OK, done. > Can you explain why this was here in the first place? I don't know. |debugger;| had existed when bug 1315927 added the test file.

Yoshi, you are the patch author of bug 1315927. Why did you include |debugger;| in test_permissions_api.html?

Comment on attachment 8840861 [details] Bug 1342144 - Fix ESLint errors in toolkit/. https://reviewboard.mozilla.org/r/115296/#review116738 ::: toolkit/components/contentprefs/tests/mochitest/test_remoteContentPrefs.html:64 (Diff revision 2) > > tester.ok(cps !== null, "got the content pref service"); > > let setPref = Defer(); > cps.setGlobal("testing", 42, null, { > - handleCompletion: function(reason) { > + handleCompletion(reason) { This will make methods harder to find :/ Is this the official new style?

Comment on attachment 8840855 [details] Bug 1342144 - Remove version parameter from the type attribute of script elements. https://reviewboard.mozilla.org/r/115284/#review116754 I had to review this in raw diff mode as a few files didn't show up properly in reviewboard. r- for the one issue with version=latest. ::: toolkit/components/extensions/test/mochitest/test_ext_jsversion.html:33 (Diff revision 1) > files: { > "background.html": ` > <meta charset="utf-8"> > ${script('src="background.js" type="application/javascript"')} > - ${script('src="background-1.js" type="application/javascript;version=1.8"')} > + ${script('src="background-1.js" type="application/javascript"')} > ${script('src="background-2.js" type="application/javascript;version=latest"')} there is a version=latest here :(

Comment on attachment 8840858 [details] Bug 1342144 - Restore test_getFileId.html and stop using legacy generators in the test. https://reviewboard.mozilla.org/r/115290/#review116806

Comment on attachment 8840856 [details] Bug 1342144 - Revert some version parameters in devtools/. https://reviewboard.mozilla.org/r/115286/#review116820 ::: devtools/client/debugger/test/mochitest/browser_dbg_parser-03.js:22 (Diff revision 1) > "let a = 42;", > "</script>", > "<script type='text/javascript'>", > "let b = 42;", > "</script>", > - "<script type='text/javascript'>", > + "<script type='text/javascript;version=1.8'>", It seems like this is testing a JS parser to see if it can handle the version parameter. Since we still have versions for now, it seems like we should keep these cases. ::: devtools/client/debugger/test/mochitest/browser_dbg_parser-05.js:22 (Diff revision 1) > "a.push('var a = 42;');", > "a.push('</script>');", > "a.push('<script type=\"text/javascript\">');", > "a.push('var b = 42;');", > "a.push('</script>');", > - "a.push('<script type=\"text/javascript\">');", > + "a.push('<script type=\"text/javascript;version=1.8\">');", It seems like this is testing a JS parser to see if it can handle the version parameter. Since we still have versions for now, it seems like we should keep these cases. ::: devtools/client/debugger/test/mochitest/browser_dbg_parser-11.js:15 (Diff revision 1) > function test() { > let { Parser } = Cu.import("resource://devtools/shared/Parser.jsm", {}); > > let source = [ > '<script type="text/javascript" src="chrome://foo.js"/>', > - '<script type="application/javascript" src="chrome://baz.js"/>', > + '<script type="application/javascript;version=1.8" src="chrome://baz.js"/>', It seems like this is testing a JS parser to see if it can handle the version parameter. Since we still have versions for now, it seems like we should keep these cases.

Comment on attachment 8840859 [details] Bug 1342144 - Whitelist some devtools scripts that are formerly versioned. https://reviewboard.mozilla.org/r/115292/#review116822 Ignore list changes look good. (I had no idea they were ignored because of the version!)

Comment on attachment 8840857 [details] Bug 1342144 - Revert version parameters in test_ext_jsversion.html. https://reviewboard.mozilla.org/r/115288/#review116868 Looks good to me, but I'm not sure why there's a separate patch to revert this change, rather than just removing it from the original patch.

Comment on attachment 8840861 [details] Bug 1342144 - Fix ESLint errors in toolkit/. https://reviewboard.mozilla.org/r/115296/#review116738 > This will make methods harder to find :/ > > Is this the official new style? ESLint complained with the following error: > /home/worker/checkouts/gecko/toolkit/components/contentprefs/tests/mochitest/test_remoteContentPrefs.html:63:11 | Expected method shorthand. (object-shorthand) So I have to either fix or whitelist the file.

Comment on attachment 8840855 [details] Bug 1342144 - Remove version parameter from the type attribute of script elements. https://reviewboard.mozilla.org/r/115284/#review116754 > there is a version=latest here :( It is the only instance of version=latest: https://dxr.mozilla.org/mozilla-central/search?q=/javascript;version=latest&redirect=false And it is required anyway (see the "Revert version parameters in test_ext_jsversion.html" patch). So I did not bother to regenerate the patch.

Comment on attachment 8840856 [details] Bug 1342144 - Revert some version parameters in devtools/. https://reviewboard.mozilla.org/r/115286/#review116820 > It seems like this is testing a JS parser to see if it can handle the version parameter. > > Since we still have versions for now, it seems like we should keep these cases. Yes, I restored version parameters exactly because of the reason you said. They are removed by the previous patch which is generated by a sed script. Sorry for the confusion, but I would not like to fold manual changes to the auto-generated gigantic patch because it will make it virtually impossible to review the previous patch.

Comment on attachment 8840857 [details] Bug 1342144 - Revert version parameters in test_ext_jsversion.html. https://reviewboard.mozilla.org/r/115288/#review116868 Because the original patch is generated by a sed script. I thought manual changes should have a separate review.

Comment on attachment 8840855 [details] Bug 1342144 - Remove version parameter from the type attribute of script elements. See comment #20.

Comment on attachment 8840856 [details] Bug 1342144 - Revert some version parameters in devtools/. See comment #21.

Comment on attachment 8840856 [details] Bug 1342144 - Revert some version parameters in devtools/. https://reviewboard.mozilla.org/r/115286/#review116910 Oh wow, I guess I got confused and thought this patch was removing them, but I see you're just reverting them back! Looks good then!

Comment on attachment 8840855 [details] Bug 1342144 - Remove version parameter from the type attribute of script elements. https://reviewboard.mozilla.org/r/115284/#review116948 thanks for the explanation

I think that removing the type attribute altogether would be a valid strategy too. Per the HTML spec, omitted attribute has the same semantics than type="application/javascript" https://html.spec.whatwg.org/multipage/scripting.html#attr-script-type

Comment on attachment 8840861 [details] Bug 1342144 - Fix ESLint errors in toolkit/. https://reviewboard.mozilla.org/r/115296/#review116968

Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/a072d7934ff7 Remove version parameter from the type attribute of script elements. r=jmaher https://hg.mozilla.org/integration/autoland/rev/5b22d7dc3adf Revert some version parameters in devtools/. r=jryans https://hg.mozilla.org/integration/autoland/rev/afe09cddd8d3 Revert version parameters in test_ext_jsversion.html. r=kmag https://hg.mozilla.org/integration/autoland/rev/bf792b35653d Restore test_getFileId.html and stop using legacy generators in the test. r=jaws https://hg.mozilla.org/integration/autoland/rev/180fe1126e50 Whitelist some devtools scripts that are formerly versioned. r=jryans https://hg.mozilla.org/integration/autoland/rev/1ce0cb7b45d1 Fix ESLint errors in browser/. r=Paolo https://hg.mozilla.org/integration/autoland/rev/cc224749e77b Fix ESLint errors in toolkit/. r=Yoric

https://hg.mozilla.org/mozilla-central/rev/a072d7934ff7 https://hg.mozilla.org/mozilla-central/rev/5b22d7dc3adf

https://hg.mozilla.org/mozilla-central/rev/afe09cddd8d3 https://hg.mozilla.org/mozilla-central/rev/bf792b35653d https://hg.mozilla.org/mozilla-central/rev/180fe1126e50 https://hg.mozilla.org/mozilla-central/rev/1ce0cb7b45d1 https://hg.mozilla.org/mozilla-central/rev/cc224749e77b

(In reply to Masatoshi Kimura [:emk] from comment #12) > Yoshi, you are the patch author of bug 1315927. Why did you include > |debugger;| in test_permissions_api.html? I copied the test from dom/permission/tests/test_permissions_api.htm, maybe you can ask Marcos (from bug 1295877) for more detail information if it still matters.