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)
Core
Security: PSM
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
Updated•7 years ago
|
status-firefox57:
affected → ---
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → hemantsingh1612
Assignee | ||
Comment 1•7 years ago
|
||
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)
Reporter | ||
Comment 2•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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)
Reporter | ||
Updated•7 years ago
|
Attachment #8865198 -
Flags: review?(standard8)
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
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)
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(standard8)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8865199 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
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)
Reporter | ||
Comment 10•7 years ago
|
||
(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 hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(hemantsingh1612)
Assignee | ||
Comment 13•7 years ago
|
||
Done.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(hemantsingh1612)
Reporter | ||
Comment 15•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 16•7 years ago
|
||
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.
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9e97b89777e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•