1509042 - Enable ESLint for dom/notification

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for the dom/notification/ directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Please note: We want to end up with two separate commits. One with the automatic changes, the second with the manual changes and the .eslintignore change. It helps with reviewing if these appear as a commit series in Phabricator.

Here's some approximate steps:

- In .eslintignore, remove the `dom/notification/Notification*.*`, `dom/notification/test/browser/**` and `dom/notification/test/mochitest/**` lines.

- Run eslint:

./mach eslint --fix dom/notification

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for dom/notification
 (automatic changes)" dom/cache

- For the remaining issues, you'll need to fix them by hand. Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint#Common_issues_and_how_to_solve_them
http://eslint.org/docs/rules/

(you can just run `./mach eslint dom/notification` to check you've fixed them).

- Create a second commit of the manual changes, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for dom/notification (manual changes)"

- Post the two commits via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
When you post, please request initial review from myself (`standard8`), I'll then redirect to the owners of the code if they look good.

Comment 1

[Ferenc Nagy]

Hi,

I would like to work on this bug. This is my very first bug in Mozilla, so I'll definitely need help.
I have a javascript background with eslint knowledge. Based on your description I already fixed this bug, currently I'm trying to configure phabricator.
Can you assign this bug to me?

Comment 2

[Ferenc Nagy]

I think I've fixed every file, but due to eslint's automatic corrections mochitest is failing with the errors below.
I think eslint modified the code due to the rule: mozilla/use-chromeutils-generateqi, and this causes at least 2 of the issues. Do I need to import ChromeUtils somehow? I cannot find any examples.

p.s.: Does Bugzilla support code blocks, or markdown editing?


dom/notification/test/mochitest/test_notification_basics.html
  FAIL uncaught exception - ReferenceError: ChromeUtils is not defined at MockServices<@http://mochi.test:8888/tests/dom/notification/test/mochitest/MockServices.js:79:5
@http://mochi.test:8888/tests/dom/notification/test/mochitest/MockServices.js:1:21

    simpletestOnerror@SimpleTest/SimpleTest.js:1644:13
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1624:1
  FAIL uncaught exception - TypeError: MockServices is undefined; can't access its "register" property at @http://mochi.test:8888/tests/dom/notification/test/mochitest/test_notification_basics.html:123:3

    simpletestOnerror@SimpleTest/SimpleTest.js:1644:13
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1624:1
dom/notification/test/mochitest/test_notification_storage.html
  FAIL uncaught exception - TypeError: MockServices is undefined; can't access its "register" property at @http://mochi.test:8888/tests/dom/notification/test/mochitest/test_notification_storage.html:152:3

    simpletestOnerror@SimpleTest/SimpleTest.js:1644:13
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1624:1
dom/notification/test/mochitest/test_notification_tag.html
  FAIL uncaught exception - ReferenceError: ChromeUtils is not defined at @http://mochi.test:8888/tests/dom/notification/test/mochitest/test_notification_tag.html:39:5

    simpletestOnerror@SimpleTest/SimpleTest.js:1644:13
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1624:1

Comment 3

[Mark Banner (:standard8)]

Hi Ferenc, thank you for working on this bug.

I think that the test is working in a non-privileged location, or in a different sandbox, so ChromeUtils isn't going to be available. Probably the best you can do here is to disable the rule for the whole MockServices.js file:

/* eslint-disable mozilla/use-chromeutils-generateqi */

Comment 4

[Ferenc Nagy]

Attachment: Bug 1509042 - Enable Eslint for dom/notification (automatic changes) r=mrbkap

Comment 5

[Ferenc Nagy]

Attachment: Bug 1509042 - Enable Eslint for dom/notification (manual changes) r=ehsan

Depends on D12815

Comment 6

[Mark Banner (:standard8)]

Hi Ferenc, did you see https://phabricator.services.mozilla.com/D12816#358514 ?

You'll probably need to rebase the patches as well - you can use hg pull --rebase (enable the rebase extension if necessary). Then using moz-phab submit you should be able to update the existing commits (note: keep the revision information in the commit header).

Comment 7

[Mark Banner (:standard8)]

Unfortunately I've not heard from Ferenc, so I'm going to take and push the good work forward.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/677240cf5140
Enable Eslint for dom/notification (automatic changes) r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/b1463424f116
Enable Eslint for dom/notification (manual changes) r=Ehsan

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/677240cf5140
https://hg.mozilla.org/mozilla-central/rev/b1463424f116

Comment 10

[Mark Banner (:standard8)]

Assigning back to Ferenc for the record as they did most of the work here.