Enable ESLint for dom/notification

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: standard8, Assigned: nagy.ferenc.jr, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments)

Reporter

Description

7 months ago
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.
Assignee

Comment 1

7 months ago
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?
Assignee

Comment 2

7 months ago
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
Reporter

Comment 3

7 months ago
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 */
Assignee: nobody → nagy.ferenc.jr
Status: NEW → ASSIGNED
Attachment #9027311 - Attachment description: Bug 1509042 - Enable Eslint for dom/notification r=Standard8 → Bug 1509042 - Enable Eslint for dom/notification (automatic changes) r=Standard8
Reporter

Comment 6

5 months ago

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).

Flags: needinfo?(nagy.ferenc.jr)
Reporter

Comment 7

4 months ago

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

Assignee: nagy.ferenc.jr → standard8
Flags: needinfo?(nagy.ferenc.jr)
Attachment #9027311 - Attachment description: Bug 1509042 - Enable Eslint for dom/notification (automatic changes) r=Standard8 → Bug 1509042 - Enable Eslint for dom/notification (automatic changes) r=mrbkap
Attachment #9027312 - Attachment description: Bug 1509042 - Enable Eslint for dom/notification (manual changes) r=Standard8 → Bug 1509042 - Enable Eslint for dom/notification (manual changes) r=ehsan

Comment 8

4 months ago
Pushed by mbanner@mozilla.com:
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

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Reporter

Comment 10

4 months ago

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

Assignee: standard8 → nagy.ferenc.jr
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.