1497604 - Enable ESLint for dom/cache

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/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

[Johanna]

Hi,

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

Comment 2

[Mark Banner (:standard8)]

Welcome Johanna, please take it and start working on it, if you have questions just ask.

Comment 3

[Johanna]

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

Comment 4

[Mark Banner (:standard8)]

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

Comment 5

[aditya81070]

Can I take this issue as my first contribution?

Comment 6

[aditya81070]

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

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Urvashi Verma]

I'd like to work on this if no one else is working.

Comment 9

[Mark Banner (:standard8)]

Hi Urvashi, please take it. Please see comment 0 for the basic instructions, feel free to ask questions if you have them.

Comment 10

[Mark Banner (:standard8)]

Urvashi, are you still working on this?

Comment 11

[Mark Banner (:standard8)]

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

Comment 12

[Monika Manuela Hengki]

I'd like to work on this, please

Comment 13

[Mark Banner (:standard8)]

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

Comment 14

[Monika Manuela Hengki]

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?

Comment 15

[Mark Banner (:standard8)]

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

Comment 16

[Monika Manuela Hengki]

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!

Comment 17

[Mark Banner (:standard8)]

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");

Comment 18

[Monika Manuela Hengki]

Attachment: Bug 1497604 - Enable ESLint for dom/cache (automatic changes)

Comment 19

[Monika Manuela Hengki]

Attachment: Bug 1497604 - Enable ESLint for dom/cache (manual changes)

Depends on D20943

Comment 20

[Mark Banner (:standard8)]

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

Comment 21

[Henri Sivonen (: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.

Comment 22

[Mark Banner (:standard8)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Mark Banner (:standard8)]

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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/36052e9424f7
https://hg.mozilla.org/mozilla-central/rev/0077a71108eb