Closed Bug 1359614 Opened 7 years ago Closed 7 years ago

Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised.

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: hemantsingh1612, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

Currently the files in security/sandbox/test don't get the same rules as security/manager. As part of bug 1359011 I'm getting the standard rule sets spread around the tree more.

Hence, we should move security/manager/.eslintrc.js to security/.eslintrc.js in preparation for this.

I'm happy to mentor this bug.

More details about ESLint here: https://developer.mozilla.org/docs/ESLint

Rough Todo list:

a) Move the file as per above
b) To fix most of the eslint issues raised, copy the contents of the file [1] below to a new file, security/sandbox/test/.eslintrc.js
c) Fix the remaining ESLint issues, http://eslint.org has details of the rules if you need them.

Note you should be able to run:

./mach eslint security

to check the files. You won't need to build to do that.

d) Do a build, and run:

./mach test security/sandbox/test/

to ensure the tests still pass.

e) Commit and push you patch for review.

[1] https://dxr.mozilla.org/mozilla-central/rev/abdcc8dfc28397b95338245390e12c56658ad182/browser/base/content/test/forms/.eslintrc.js
Assignee: nobody → hemantsingh1612
What I need to do here !
c) Fix the remaining ESLint issues, http://eslint.org has details of the rules if you need them.
Flags: needinfo?(standard8)
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #1)
> What I need to do here !
> c) Fix the remaining ESLint issues, http://eslint.org has details of the
> rules if you need them.

Once you've moved the file (use hg mv btw), and then run ./mach eslint security, there should be a series of errors listed. You'll need to edit the files to resolve those errors.

Some extra hints available here: https://developer.mozilla.org/docs/ESLint

Also, there's editor integration for eslint (depending on your editor): http://eslint.org/docs/user-guide/integrations
Flags: needinfo?(standard8)
Hi Mark

I integrated eslint with atom for error fixing.
There are still some errors left particularly(strict).
Majority of them were due to (no-undef).I have changed them to warn as of now(not sure whether to write them as /* global*/ in respective files instead ).
Flags: needinfo?(standard8)
Attachment #8865198 - Flags: review?(standard8)
Comment on attachment 8865199 [details]
Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised.

https://reviewboard.mozilla.org/r/136874/#review139976

Thank you for starting this.

For the "strict" rule, all you need to do is insert a line just under the license header in each file, that says:

"use strict";

That will be enough for fixing that part.

::: security/.eslintrc.js
(Diff revision 1)
>  "use strict";
>  
>  module.exports = {
> -  "extends": [
> -    "plugin:mozilla/recommended"

The extends section should stay - we need that.

::: security/.eslintrc.js:9
(Diff revision 1)
>    "rules": {
>      // Enforce return statements in callbacks of array methods.
>      "array-callback-return": "error",
>  
>      // Braces only needed for multi-line arrow function blocks
> -    "arrow-body-style": ["error", "as-needed"],
> +    "arrow-body-style": ["warn", "as-needed"],

If you run:

./mach eslint --fix security

It'll automatically fix a lot of the issues. Sorry I forgot to mention that earlier.

You should be able to leave this line as error - it'll fix it for you.

::: security/.eslintrc.js:99
(Diff revision 1)
>  
>      // Disallow throwing literals (eg. |throw "error"| instead of
>      // |throw new Error("error")|)
>      "no-throw-literal": "error",
>  
> +    "no-undef": ["warn", { "typeof": true }],

For this if you:

cp security/manager/ssl/tests/mochitest/browser/.eslintrc.js security/sandbox/test

that will fix most of the no-undef issues as it will tell the test directory to import from the browser-test config which has the right definitions.

(Don't forget to add the file to the repository).

Also, please remove this line once done, so we fallback to the recommended style.

::: security/sandbox/test/browser_content_sandbox_fs.js:7
(Diff revision 1)
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  var prefs = Cc["@mozilla.org/preferences-service;1"]
>              .getService(Ci.nsIPrefBranch);
>  
>  Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/" +

Just before this line, add:

/* import-globals-from browser_content_sandbox_utils.js */

do the same in browser_content_sandbox_syscalls.js.

That'll fix the rest of the no-undef issues.

::: security/sandbox/test/browser_content_sandbox_fs.js:93
(Diff revision 1)
>        break;
>      case "Linux":
>        fileIOSandboxMinLevel = 2;
>        break;
>      default:
>        Assert.ok(false, "Unknown OS");

This is showing as failing consistent-return. I think its safe enough to add a `return 0;` for the default case for both of the functions where this rule is failing.
Attachment #8865199 - Flags: review?(standard8)
Comment on attachment 8865198 [details]
Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised.

https://reviewboard.mozilla.org/r/136872/#review139984

This part is fine, though I think you could just merge the two patches together.
Attachment #8865198 - Flags: review?(standard8) → review+
Flags: needinfo?(standard8)
Attachment #8865199 - Attachment is obsolete: true
Hi Mark

Thanks for the above details.I have commited changes accordingly.|./mach eslint security| gives 0 errors & 0 warnings.
After artifact build |./mach test security/sandbox/test/| passes:

Browser Chrome Test Summary
        Passed: 20
        Failed: 0
        Todo: 0
        Mode: e10s
Flags: needinfo?(standard8)
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #9)
> Thanks for the above details.I have commited changes accordingly.|./mach
> eslint security| gives 0 errors & 0 warnings.
> After artifact build |./mach test security/sandbox/test/| passes:

Thank you Hemant, can you rebase this onto the latest mozilla-central? There are some recently landed changes that are conflicting with your ones.

(Sorry, I forgot to mention that earlier).
Flags: needinfo?(standard8)
Comment on attachment 8865198 [details]
Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised.

https://reviewboard.mozilla.org/r/136872/#review140586

I don't see the security/sandbox/test/.eslintrc.js file, did you forget to hg add it? (comment 6).

::: commit-message-37a5b:5
(Diff revision 3)
> +Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised. r?standard8
> +
> +MozReview-Commit-ID: GaUbbAFWtSk
> +***
> +Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised. r?standard8

Can you update the comment to remove the duplicate lines please?
Attachment #8865198 - Flags: review+
Flags: needinfo?(hemantsingh1612)
Done.
Flags: needinfo?(hemantsingh1612)
Comment on attachment 8865198 [details]
Bug 1359614 - Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised.

https://reviewboard.mozilla.org/r/136872/#review140692

Great, thank you for the updates and persistence. This is working well now.
Attachment #8865198 - Flags: review?(standard8) → review+
I've triggered the landing process, however the tree is currently closed so it might take a few hours before it lands in our integration branch.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9e97b89777e
Move the security/manager/.eslintrc.js to security/.eslintrc.js and fix the ESLint issues raised. r=standard8
https://hg.mozilla.org/mozilla-central/rev/d9e97b89777e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: