Closed Bug 1509042 Opened 5 years ago Closed 5 years ago

Enable ESLint for dom/notification

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

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

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files)

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

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)

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
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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.

Attachment

General

Creator:
Created:
Updated:
Size: