1251728 - Eslint cleanup of memory tool code

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P2)

Comment 1

[Cody]

Hello,

I would like to take this bug as my University of Toronto Assignment for a good first bug. Please advise as to where to look for memory tool code.

Comment 2

[Tim Nguyen :ntim]

(In reply to Cody from comment #1)
> Hello,
> 
> I would like to take this bug as my University of Toronto Assignment for a
> good first bug. Please advise as to where to look for memory tool code.

Thanks for your interest!

First of all, you need to set up your development environment, the fastest way to do it is to follow the instructions here: https://hacks.mozilla.org/2016/02/introducing-devtools-reload/

Once you've done so:
- Setup eslint by running `./mach eslint --setup`
- Check all the eslint errors in the memory tool code by running: `./mach eslint devtools/client/memory/ --no-ignore`
- Fix those errors by editing the JS files in the `devtools/client/memory`
- Run eslint again to make sure you haven't forgotten any error or added any
- Run the Memory tool for a test drive (you need to enable it from the options panel inside the developer tools), if everything works fine, generate a diff and post it here. The blog post I linked above should give enough information on how to test your own copy of developer tools.

If you need any help, please use the need more information box located after the comment box. You can also ask other people around the #devtools or #introduction channel on IRC.

Comment 3

[Cody]

Hi Tim,

Thanks for the setup instructions.

I've been able to run eslint and check out some of the errors. Most them I understand and should be able to fix no problem. However, there are a lot of unused-vars errors, and while it seems like those variables aren't used in the particular file they are in error for, I'm assuming they may be needed elsewhere? If they should actually be removed that should be no problem.

Comment 4

[Tim Nguyen :ntim]

(In reply to Cody from comment #3)
> Hi Tim,
> 
> Thanks for the setup instructions.
> 
> I've been able to run eslint and check out some of the errors. Most them I
> understand and should be able to fix no problem. However, there are a lot of
> unused-vars errors, and while it seems like those variables aren't used in
> the particular file they are in error for, I'm assuming they may be needed
> elsewhere? If they should actually be removed that should be no problem.

If they aren't used elsewhere, then they should be removed. A simple search on http://dxr.mozilla.org (with your queries prefixed with "path:devtools/client/memory ") should tell you if they are used elsewhere or not. 
If they are, in that case, add `/* exported variable1, variable2 */` at the beginning of the file.

Comment 5

[Cody]

Attachment: Patch for Bug1251728

I've generated a patch. However there is still 1 error I'm not sure how to fix:

 0:00.08 Running /usr/bin/eslint
 0:00.08 /usr/bin/eslint --plugin html --ext [.js,.jsm,.jsx,.xml,.html] devtools/client/memory/ --no-ignore

/gecko-dev/devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
  30:40  error  "el" is already declared in the upper scope  no-shadow

✖ 1 problem (1 error, 0 warnings)

It seems that the variable is declared partially in terms of itself. 

Please advise how to move forward.

 0:02.97 Finished eslint. Errors encountered.

Comment 6

[Cody]

(In reply to Cody from comment #5)

Oops, the last line of the previous comment was supposed to be under "1 problem".

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8725455 [details] [diff] [review]
Patch for Bug1251728

Review of attachment 8725455 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
@@ +26,5 @@
>    [L10N.getStr("tree-item.nostack")].forEach(findNameCell);
>  
> +  function findNameCell(name) {
> +    let el = Array.prototype.find.call($$(".tree .heap-tree-item-name span"),
> +                                       el => el.textContent === name);

What you'd want to do here is use a different variable name for `el` in `el => el.textContent === name`. It should fix the issue without changing how things work.

Comment 8

[Cody]

Attachment: Patch for Bug1251728 with variable shadow fixed.

Ok thanks! Here's the patch with that issue fixed and currently showing no eslint errors.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8725490 [details] [diff] [review]
Patch for Bug1251728 with variable shadow fixed.

Review of attachment 8725490 [details] [diff] [review]:
-----------------------------------------------------------------

Can you fold this with your previous patch? This patch only contains the browser_memory_breakdowns_01.js change

::: devtools/client/memory/test/browser/browser_memory_breakdowns_01.js
@@ +31,1 @@
>      ok(el, `Found heap tree item cell for ${name}.`);

Maybe the outer variable should be called cell, while the inner variable should be named el.
Like this:     
  let cell = Array.prototype.find.call($$(".tree .heap-tree-item-name span"), 
                                       el => el.textContent === name);
  ok(cell, `Found heap tree item cell for ${name}.`);

Comment 10

[Tim Nguyen :ntim]

I'll be busy, so please ask Patrick Brosset for review, or if you need any information about eslint.

Comment 11

[Cody]

Attachment: Proposed patch for Bug 1251728

Ok, thanks Tim. Here are the patches folded together, with the variable name in question changed to cell. I've requested review from Patrick.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8725763 [details] [diff] [review]
Proposed patch for Bug 1251728

Review of attachment 8725763 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the cleanup.
I assume all eslint errors are gone with this change? If so, then you should also remove this line from .eslintignore:
devtools/client/memory/**
so that we start checking the whole directory during CI.

Also, minor comment: can you change the commit message to end with r=pbrosset instead of r=Patrick Brosset

Now, I have made some comments in the code changes below, but I have a few other remarks that apply to several files:

- about /*exported ... */ comments
I don't think this is the right solution, if a module exports something, then it most probably uses `exports.something = ...` already.
I see a lot of them do something like: `const something = exports.something = ...` and so eslint complains that `something` isn't used.
I would like to argue that we remove the `const something = ` part and just leave the `exports.something = ...` part.
Adding /*exported */ comments means that we have to maintain these comments manually when things change.

- there's a bunch of /*globals waitUntilState */ and other similar globals defined in tests.
There's a better way to import these globals for eslint to know about them.
First of all, you should know that globals from the corresponding head.js file are imported automatically. Then, if head.js loads some other files (like shared-head.js and shared-redux-head.js), then the globals from these files can be imported in eslint by using a special comment:
/* import-globals-from path/to/loaded/file.js */
So, in the case of /devtools/client/memory/test/browser/head.js
you can add these 2 comments:
/* import-globals-from ../../../framework/test/shared-head.js */
/* import-globals-from ../../../framework/test/shared-redux-head.js */
This way, eslint will know about the globals in these files, import them in head.js, and in turn, all tests in the same directory as this head.js will get them too. So no need for any /*globals */ comment in those tests.

- For head.js files: these files typically contain a lot of globals that are only used in other files. I see you've used a bunch of /*exported */ comments there too.
So far, we've been doing this instead:
/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
This tells eslint not to worry about global variables that aren't used inside head.js files.
I think this is less maintenance burden than listing all variables that are being exported.

::: devtools/client/memory/test/unit/test_dominator_trees_04.js
@@ +29,5 @@
>    for (let intermediateSnapshotState of [states.SAVING,
>                                           states.READING,
>                                           states.SAVING_CENSUS]) {
> +    dumpn(`Testing switching to the DOMINATOR_TREE view
> +          in the middle of the ${intermediateSnapshotState} snapshot state`);

The problem with wrapping in a template string is that whitespaces are going to appear in the output now:

Testing switching to the DOMINATOR_TREE view
         in the middle of the ... snapshot state

::: devtools/client/memory/utils.js
@@ +99,4 @@
>    } catch (e) {
>      DevToolsUtils.reportException(
> +      `String stored in "${CUSTOM_BREAKDOWN_PREF}"
> +       pref cannot be parsed by \`JSON.parse()\`.`);

Wrapping in a template string means that the string will wrap on 2 lines in the browser console too. Might not be a problem, but if you don't want this to happen, you should use a quoted string instead.

@@ +122,1 @@
>    // If it's in our custom breakdowns, use it

I believe It makes more sense to have the comment inside the if block instead of just above.

Comment 13

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 14

[Fuqiao Xue [:xfq]]

Hi Tim,

I'm interested in this bug.

I've installed ESLint and its plugins with './mach eslint --setup', but when trying to check the ESLint errors in the memory tool code, I got this:

    mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
    ✖ 0 problems (0 errors, 0 warnings)

Any insight?

Comment 15

[Tim Nguyen :ntim]

(In reply to Xue Fuqiao [:xfq] from comment #14)
> Hi Tim,
> 
> I'm interested in this bug.
> 
> I've installed ESLint and its plugins with './mach eslint --setup', but when
> trying to check the ESLint errors in the memory tool code, I got this:
> 
>     mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
>     ✖ 0 problems (0 errors, 0 warnings)
> 
> Any insight?
Simply running: eslint devtools/client/memory/ --no-ignore (without the ./mach) works for me, and displays the errors.

Also note that you can use eslint devtools/client/memory/ --fix to fix some errors (although it won't fix all of them).

Comment 16

[Fuqiao Xue [:xfq]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> (In reply to Xue Fuqiao [:xfq] from comment #14)
> > I've installed ESLint and its plugins with './mach eslint --setup', but when
> > trying to check the ESLint errors in the memory tool code, I got this:
> > 
> >     mozilla-central xfq$ ./mach eslint devtools/client/memory/ --no-ignore
> >     ✖ 0 problems (0 errors, 0 warnings)
> > 
> > Any insight?
> Simply running: eslint devtools/client/memory/ --no-ignore (without the
> ./mach) works for me, and displays the errors.
> 
> Also note that you can use eslint devtools/client/memory/ --fix to fix some
> errors (although it won't fix all of them).

$ eslint devtools/client/memory/ --no-ignore
-bash: eslint: command not found

Then I tried this:

$ tools/lint/eslint/node_modules/.bin/eslint devtools/client/memory/ --no-ignore

and finally succeeded with '✖ 401 problems (400 errors, 1 warning)'.

I'll try to fix them later. Thank you!

Comment 17

[Thomas Dräbing]

Attachment: Linted code files following ESLint rules

Comment 18

[Thomas Dräbing]

Hi,

I created a patch, in which I linted the respective files. I followed the hints given in earlier comments. ESLint is now giving 0 errors. Please review my patch. If anything should be done differently, I am happy to apply further changes to the patch.

Thanks and best,
Thomas

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8820607 [details] [diff] [review]
Linted code files following ESLint rules

Review of attachment 8820607 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great overall! Thanks for taking your time to go through these files.

My main concern is putting /* exported run_test */ on every unit test.

Creating an .eslintrc.js file should spare you from those comments.

::: devtools/client/memory/actions/census-display.js
@@ +30,1 @@
>  

You can write:
    "Breakdowns must be an object with a `by` property, attempted to set: " +
    uneval(display)

::: devtools/client/memory/actions/diffing.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* exported snapshotActions */
> +

Are you sure snapshotActions is used elsewhere ?

::: devtools/client/memory/actions/io.js
@@ +68,5 @@
>    };
>  };
>  
> +const importSnapshotAndCensus =
> +  exports.importSnapshotAndCensus = function (heapWorker, path) {

I'd rather do:
const importSnapshotAndCensus = function(...) {
 ...
};
exports.importSnapshotAndCensus = importSnapshotAndCensus;

since it spares us from indenting the whole block

::: devtools/client/memory/actions/label-display.js
@@ +28,5 @@
>    assert(typeof display === "object"
>           && display
>           && display.breakdown
>           && display.breakdown.by,
> +    `Breakdowns must be an object with a \`by\` property, attempted to set: ` +

Same comment as previously.

::: devtools/client/memory/actions/tree-map-display.js
@@ +28,5 @@
>           && display
>           && display.breakdown
>           && display.breakdown.by,
> +    `Breakdowns must be an object with a \`by\` property, attempted to set: ` +
> +    `${uneval(display)}`);

Same comment as previously

::: devtools/client/memory/initializer.js
@@ +53,5 @@
> +  gRoot = null;
> +  gApp = null;
> +  gProvider = null;
> +  unsubscribe = null;
> +  isHighlighted = null;

nit: 
use 
`gStore = gRoot = gApp = gProvider = unsubscribe = isHighlighted = null;`

::: devtools/client/memory/reducers/snapshots.js
@@ +273,1 @@
>  

This is the same as:
        "Should have already computed the dominator tree, found state = " +
        snapshot.dominatorTree.state

::: devtools/client/memory/test/browser/head.js
@@ +5,5 @@
> +/* import-globals-from ../../../framework/test/shared-redux-head.js */
> +
> +/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
> +
> +/* global censusState */

You should be able to add as eslintrc file like this one: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/test/browser/.eslintrc.js

And a lot of var-related errors should disappear.

::: devtools/client/memory/test/unit/test_action-clear-snapshots_01.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +/* exported run_test */

The eslintrc file should spare you from doing that.

::: devtools/client/memory/test/unit/test_dominator_trees_04.js
@@ +30,5 @@
>    yield front.attach();
>  
> +  for (let intermediateSnapshotState of [states.SAVING, states.READING, states.READ]) {
> +    dumpn(`Testing switching to the DOMINATOR_TREE view in the middle of the ` +
> +          `${intermediateSnapshotState} snapshot state`);

The first line can be a normal string now.

Comment 20

[Thomas Dräbing]

Attachment: Linted code files following ESLint rules. Changes now include comments of Tim Nguyen

Comment 21

[Thomas Dräbing]

Hi Tim,

thanks for the helpful comments. I made the corresponding changes and attached a patch containing the combined changes.

Best,
Thomas

Comment 22

[Tim Nguyen :ntim]

Attachment: memory-tool-eslint

Hey Thomas,
Thanks for your new patch. It had a few issues, so I've fixed them in this patch (I've kept your commit info though). It is now ready for landing.

I've pushed your patch to the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27267aefc28f7f74ba668480b728914307737e5c

Comment 23

[Tim Nguyen :ntim]

Just a reminder to check the try results and land if everything looks alright.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ad068aeaaf
Make devtools/client/memory/ eslint clean. r=ntim

Comment 25

[Tim Nguyen :ntim]

(In reply to Pulsebot from comment #24)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ad068aeaaf
> Make devtools/client/memory/ eslint clean. r=ntim

Thomas, I've just landed your patch on mozilla-inbound, it should be in Nightly in two days. Congrats on your first patch!

Comment 26

[Thomas Dräbing]

Thank you so much Tim for your reviews and support. It was a fun task to start out with. I will certainly look out for other opportunities to contribute.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8303c228d2d9
Followup: fix memory tool Xpcshell bustage due to introduction of strict mode. r=bustage

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9ad068aeaaf
https://hg.mozilla.org/mozilla-central/rev/8303c228d2d9