Closed Bug 1342144 Opened 7 years ago Closed 7 years ago

Remove version parameter from the type attribute of script elements

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(7 files)

Bug 1342144 - Remove version parameter from the type attribute of script elements.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1342144 - Revert some version parameters in devtools/.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1342144 - Revert version parameters in test_ext_jsversion.html.
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1342144 - Restore test_getFileId.html and stop using legacy generators in the test.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1342144 - Whitelist some devtools scripts that are formerly versioned.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1342144 - Fix ESLint errors in browser/.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Bug 1342144 - Fix ESLint errors in toolkit/.
59 bytes, text/x-review-board-request
Yoric
: review+
Details 
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?
Attachment #8840860 - Flags: review?(paolo.mozmail) → review+
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?
Flags: needinfo?(allstars.chh)
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?
Attachment #8840861 - Flags: review?(dteller)
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 :(
Attachment #8840855 - Flags: review?(jmaher) → review-
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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
Attachment #8840858 - Flags: review?(jaws) → review+
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.
Attachment #8840856 - Flags: review?(jryans)
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!)
Attachment #8840859 - Flags: review?(jryans) → review+
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.
Attachment #8840857 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8840855 - Flags: review- → review?(jmaher)
Comment on attachment 8840856 [details]
Bug 1342144 - Revert some version parameters in devtools/.

See comment #21.
Attachment #8840856 - Flags: review?(jryans)
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!
Attachment #8840856 - Flags: review?(jryans) → review+
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
Attachment #8840855 - Flags: review?(jmaher) → review+
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
Attachment #8840861 - Flags: review?(dteller) → review+
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
See Also: → 1342760
Blocks: 1342764
(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.
Flags: needinfo?(allstars.chh)
Depends on: 1344286
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: