Enable ESLint for dom/cache

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: standard8, Assigned: manuela.monika97, Mentored)

Tracking

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

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox64 wontfix, 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/cache/ 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. It helps with reviewing if these appear as a commit series in phabricator.

Here's some approximate steps:

1. In .eslintignore, remove the `dom/cache/test/mochitest/**` and `dom/cache/test/xpcshell/**` lines.

2. Run eslint:

./mach eslint --fix dom/cache

This should fix most/all of the issues.

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

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

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

5. 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/cache` to check you've fixed them).

6. Add the previously copied .eslintrc.js files to source control, e.g.

$ hg add dom/cache/test/xpcshell/.eslintrc.js dom/cache/test/mochitest/.eslintrc.js

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

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

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

7 months ago
Hi,

I would like to work on this as my first contribution please.
(Reporter)

Comment 2

7 months ago
Welcome Johanna, please take it and start working on it, if you have questions just ask.
Assignee: nobody → jyherman
Priority: -- → P2

Comment 3

7 months ago
Hi

Sorry about this but I am an Outreachy applicant and my mentor has given me something else to work on for my contribution.  Can I be unassigned please
Assignee: jyherman → nobody
(Reporter)

Comment 4

7 months ago
Sure, you've already done it :-) Thank you for letting us know.

Comment 5

7 months ago
Can I take this issue as my first contribution?

Comment 6

7 months ago
(In reply to Mark Banner (:standard8) (afk 15 - 18 Oct) from comment #4)
> Sure, you've already done it :-) Thank you for letting us know.

I would like to work on this issue as my first contribution.
(Reporter)

Comment 7

7 months ago
Please do, feel free to just attach a patch if no-one is working on a bug.

Comment 8

6 months ago
I'd like to work on this if no one else is working.
(Reporter)

Comment 9

6 months ago
Hi Urvashi, please take it. Please see comment 0 for the basic instructions, feel free to ask questions if you have them.
Assignee: nobody → vurvashi11
Status: NEW → ASSIGNED
(Reporter)

Comment 10

4 months ago

Urvashi, are you still working on this?

Flags: needinfo?(vurvashi11)
(Reporter)

Comment 11

3 months ago

I've not heard anything back from Urvashi, so I'm opening this up again.

Assignee: vurvashi11 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(vurvashi11)
(Assignee)

Comment 12

3 months ago

I'd like to work on this, please

(Reporter)

Comment 13

3 months ago

Hi Monika, please do. Feel free to ask questions if you have them.

Assignee: nobody → manuela.monika97
(Assignee)

Comment 14

3 months ago

Hi Mark, right now I'm doing the manual fix for this:

c:\mozilla-source\mozilla-central\dom\cache\test\mochitest\browser_cache_pb_window.js
   4:52  error  'name' is already declared in the upper scope.  no-shadow (eslint)

Any suggestion as to what I should rename this parameter to?

Flags: needinfo?(standard8)
(Reporter)

Comment 15

3 months ago

(In reply to Monika Manuela Hengki from comment #14)

c:\mozilla-source\mozilla-central\dom\cache\test\mochitest\browser_cache_pb_window.js
   4:52  error  'name' is already declared in the upper scope.  no-shadow (eslint)

Any suggestion as to what I should rename this parameter to?

Hi Monika, the interesting thing with this file is that no-shadow doesn't really apply here. The ContentTask.spawn(browser, name, function(name) { causes the function(name) {...} to be run in a different process, so from a processing perspective there's no overlap of variable names.

I think you could just do /* eslint-disable no-shadow */ near the top of the file, and that would be fine.

Flags: needinfo?(standard8)
(Assignee)

Comment 16

3 months ago

Hi Mark, currently I'm trying to fix these:

c:\mozilla-source\mozilla-central\dom\cache\test\xpcshell\head.js
  24:26  error  Use Services.dirsvc rather than getService().  mozilla/use-services (eslint)
  73:20  error  Use Services.dirsvc rather than getService().  mozilla/use-services (eslint)

Can I check if this is the correct way to fix?
What I understand is that I need to replace
var directoryService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
with Services.dirsvc to get an instance

I'm having doubts because the docs here says that dirsvc has 2 Service interfaces, and the fix I suggested above doesn't specifically mention which interface to use.

Thanks!

Flags: needinfo?(standard8)
(Reporter)

Comment 17

3 months ago

Hi Monika,

What I understand is that I need to replace
var directoryService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
with Services.dirsvc to get an instance

So you need to change this to:

var directoryServices = Services.dirsvc;

The Services.jsm file has already done the .getService and made sure the interfaces are available, so you don't need to handle that.

If there aren't too many usages, then you can also drop the intermediate variable, and do Services.dirsvc.get('ProfD', Ci.nsIFile); direct.

Note, you will need to import Services.jsm, I've just updated it to use the latest format which landed recently:

const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

Flags: needinfo?(standard8)
(Reporter)

Comment 20

3 months ago

Henri, did you see the review requests here? Would it better if I pushed them to someone else?

Flags: needinfo?(hsivonen)

(In reply to Mark Banner (:standard8) from comment #20)

Henri, did you see the review requests here?

I didn't not sure why. Sorry.

Flags: needinfo?(hsivonen)
(Reporter)

Comment 22

2 months ago

(In reply to Henri Sivonen (:hsivonen) from comment #21)

(In reply to Mark Banner (:standard8) from comment #20)

Henri, did you see the review requests here?

I didn't not sure why. Sorry.

No problem. Thank you for the reviews.

Comment 23

2 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36052e9424f7
Enable ESLint for dom/cache (automatic changes) r=Standard8,hsivonen
https://hg.mozilla.org/integration/autoland/rev/0077a71108eb
Enable ESLint for dom/cache (manual changes) r=Standard8,hsivonen
(Reporter)

Comment 24

2 months ago

Hi Monika, now you've got approved patches, I've just landed them on our integration branch for you. If not issues are found, they'll be merged to our master branch within around 24 hours.

Congratulations on getting your first bug fixed.

If you want to find more, please drop me a line with the sort of thing you'd like to do, or take a look on https://codetribute.mozilla.org/ for more.

Comment 25

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.