Closed
Bug 1251011
Opened 8 years ago
Closed 8 years ago
Enable ESLint "no-undef" rule for PSM
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
Details
Attachments
(1 file)
This rule catches references to undefined variables, as well as accidentally setting global variables by doing something like |foo = bar;| instead of |let foo = bar;|.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39955/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39955/
Attachment #8730509 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•8 years ago
|
||
https://reviewboard.mozilla.org/r/39955/#review36509 ::: security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureCSS.html:12 (Diff revision 1) > > <link rel="stylesheet" type="text/css" Trailing whitespace. ::: testing/xpcshell/xpcshell.eslintrc:32 (Diff revision 1) > + "greater": false, > + "greaterOrEqual": false, I apparently don't know that g comes after e in the alphabet.
Comment on attachment 8730509 [details] MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop https://reviewboard.mozilla.org/r/39955/#review36681 Great! ::: security/manager/pki/resources/content/createCertInfo.js:34 (Diff revision 1) > keygenThread.startKeyGeneration(obs); > } > > function onClose() > { > - setCursor("default"); > + window.setCursor("default"); Huh - I think this should have always been "auto" instead of "default". ::: security/manager/pki/resources/content/protectedAuth.js:40 (Diff revision 1) > } > } > > function onClose() > { > - setCursor("default"); > + window.setCursor("default"); Same here. ::: security/manager/ssl/tests/mochitest/mixedcontent/test_secureAll.html:13 (Diff revision 1) > <script type="text/javascript" src="/MochiKit/Signal.js"></script> > <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > <script type="text/javascript" src="mixedContentTest.js"></script> > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > > <link rel="stylesheet" type="text/css" Trailing whitespace here if you feel like fixing it. ::: security/manager/ssl/tests/unit/test_toolkit_securityreporter.js:20 (Diff revision 1) > > "use strict"; > const CC = Components.Constructor; > const Cm = Components.manager; > > Cu.import("resource://testing-common/AppInfo.jsm"); Hmmm - can we do const \{ AppInfo \} = Cu.import("resource://testing-common/AppInfo.jsm", \{\}); AppInfo.updateAppInfo(); ? No big deal, though - looks like other lint files handle this similarly. ::: testing/xpcshell/xpcshell.eslintrc:45 (Diff revision 1) > "notEqual": false, > "notStrictEqual": false, > "ok": false, > "run_next_test": false, > "run_test": false, > + "run_test_in_child": false, Since changes here affect all xpcshell tests that have linting enabled, I feel like you should get sign-off from maybe a test harness peer. Perhaps Ted Mielczarek?
Attachment #8730509 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/39955/#review36681 Thanks for the review! > Huh - I think this should have always been "auto" instead of "default". Hmm, indeed. I'll update this. > Hmmm - can we do const \{ AppInfo \} = Cu.import("resource://testing-common/AppInfo.jsm", \{\}); AppInfo.updateAppInfo(); ? > No big deal, though - looks like other lint files handle this similarly. Sadly, no. AppInfo.jsm exports its methods directly, so we can do this: > const { updateAppInfo } = Cu.import("resource://testing-common/AppInfo.jsm", {}); ... but that looks kinda weird. > Since changes here affect all xpcshell tests that have linting enabled, I feel like you should get sign-off from maybe a test harness peer. Perhaps Ted Mielczarek? Sure.
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8730509 [details] MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39955/diff/1-2/
Attachment #8730509 -
Flags: review?(ted)
Assignee | ||
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/39955/#review36805 Ted: If you could take a look at the xpcshell.eslintrc changes, that would be great. If not, feel free to redirect. Thanks.
Comment 7•8 years ago
|
||
Despite being the module owner here I'm not familiar with these eslintrc files. I can't edit your MozReview request, but I'd recommend you tag Mossop for review since he authored that file.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7) > Despite being the module owner here I'm not familiar with these eslintrc > files. I can't edit your MozReview request, but I'd recommend you tag Mossop > for review since he authored that file. I'll do that, thanks.
Assignee | ||
Updated•8 years ago
|
Attachment #8730509 -
Flags: review?(ted) → review?(dtownsend)
Updated•8 years ago
|
Attachment #8730509 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8730509 [details] MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39955/diff/2-3/
Attachment #8730509 -
Attachment description: MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. → MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop
Attachment #8730509 -
Flags: review+ → review?(dtownsend)
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8730509 [details] MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop (Bugzilla set r+ and MozReview flags don't mesh that well ATM I guess.)
Attachment #8730509 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Thanks for the reviews! https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d1dfe490961
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0ff2a3c740
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e0ff2a3c740
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•