Closed Bug 1251011 Opened 4 years ago Closed 4 years ago

Enable ESLint "no-undef" rule for PSM

Categories

(Core :: Security: PSM, defect)

defect
Not set

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;|.
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+
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.
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)
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.
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.
(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.
Attachment #8730509 - Flags: review?(ted) → review?(dtownsend)
Attachment #8730509 - Flags: review?(dtownsend) → review+
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)
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+
https://hg.mozilla.org/mozilla-central/rev/6e0ff2a3c740
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.