1290142 - Eslint the performance tool client

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

Comment 1

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/components;

Review commit: https://reviewboard.mozilla.org/r/68904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68904/

Comment 2

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/legacy;

Review commit: https://reviewboard.mozilla.org/r/68906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68906/

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/modules/ subfolders;

Review commit: https://reviewboard.mozilla.org/r/68908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68908/

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/modules/;

Review commit: https://reviewboard.mozilla.org/r/68910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68910/

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/test/unit;

Review commit: https://reviewboard.mozilla.org/r/68912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68912/

Comment 6

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290142 - Lint devtools/client/performance/test/;

Review commit: https://reviewboard.mozilla.org/r/68914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68914/

Comment 7

[Greg Tatum [:gregtatum]]

jsantell: Sorry for the huge dump of code changes. I figured this would be the most manageable way to lint the rest of the code. Take your time, I can always rebase changes.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777331 [details]
Bug 1290142 - Lint devtools/client/performance/components;

https://reviewboard.mozilla.org/r/68904/#review66090

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

https://reviewboard.mozilla.org/r/68906/#review66092

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777332 [details]
Bug 1290142 - Lint devtools/client/performance/legacy;

https://reviewboard.mozilla.org/r/68906/#review66094

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

https://reviewboard.mozilla.org/r/68908/#review66100

::: devtools/client/performance/modules/logic/tree-model.js:292
(Diff revision 1)
>  
>      // The new root.
>      let rootCalls = [];
>  
>      // Walk depth-first and keep the current spine (e.g., callstack).
> -    while (entry = workstack.pop()) {
> +    do {

I think this change makes this code a bit harder to understand, but guessing it's because of the lint rule of assignment in conditionals and while loops, ugh

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777333 [details]
Bug 1290142 - Lint devtools/client/performance/modules/ subfolders;

https://reviewboard.mozilla.org/r/68908/#review66102

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777334 [details]
Bug 1290142 - Lint devtools/client/performance/modules/;

https://reviewboard.mozilla.org/r/68910/#review66106

::: devtools/client/performance/modules/marker-formatters.js:25
(Diff revision 1)
>  const JS_MARKER_MAP = {
> -  "<script> element":          L10N.getStr("marker.label.javascript.scriptElement"),
> -  "promise callback":          L10N.getStr("marker.label.javascript.promiseCallback"),
> -  "promise initializer":       L10N.getStr("marker.label.javascript.promiseInit"),
> -  "Worker runnable":           L10N.getStr("marker.label.javascript.workerRunnable"),
> -  "javascript: URI":           L10N.getStr("marker.label.javascript.jsURI"),
> +  "<script> element": L10N.getStr("marker.label.javascript.scriptElement"),
> +  "promise callback": L10N.getStr("marker.label.javascript.promiseCallback"),
> +  "promise initializer": L10N.getStr("marker.label.javascript.promiseInit"),
> +  "Worker runnable": L10N.getStr("marker.label.javascript.workerRunnable"),
> +  "javascript:": L10N.getStr("marker.label.javascript.jsURI"),

This looks like this would break -- the keys on this object are marker names from the platform, and cannot change unless the platform also changes, and we have the old marker names for older platforms -- in short, this key name should not change

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777335 [details]
Bug 1290142 - Lint devtools/client/performance/test/unit;

https://reviewboard.mozilla.org/r/68912/#review66108

::: devtools/client/performance/test/unit/test_perf-utils-allocations-to-samples.js:69
(Diff revision 1)
>        "prefix": 0,
>        "frame": 1
>      },
>      "data": [
>        null,
> -      [ null, 1 ], // x (A:1:2)
> +      // x (A:1:2)

These changes make the data structure harder to infer, IMO

::: devtools/client/performance/test/unit/test_tree-model-allocations-02.js:31
(Diff revision 1)
>     * +-------------+------------+-------------+-------------+------------------------------+
>     * | 1790272 41% | 8307   17% | 1790372 42% | 8317    18% | V someFunc @ a.j:345:6       |
>     * |     100  1% | 10      1% |     100  1% |   10     1% |   > callerFunc @ b.j:765:34  |
>     * +-------------+------------+-------------+-------------+------------------------------+
>     */
> +  /* eslint-enable max-len */

Nice, I'd much rather disabling lint if it means more legible code

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777336 [details]
Bug 1290142 - Lint devtools/client/performance/test/;

https://reviewboard.mozilla.org/r/68914/#review66112

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Thanks for taking the lead on this, Greg! Only thing that for sure should be checked out is the JS_MARKER_MAP key change, that looks like it'll break stuff.

Comment 17

[Greg Tatum [:gregtatum]]

https://reviewboard.mozilla.org/r/68912/#review66108

> These changes make the data structure harder to infer, IMO

Good call, I'll put it back to the way it was before, and disable the eslint rule.

Comment 18

[Greg Tatum [:gregtatum]]

https://reviewboard.mozilla.org/r/68910/#review66106

> This looks like this would break -- the keys on this object are marker names from the platform, and cannot change unless the platform also changes, and we have the old marker names for older platforms -- in short, this key name should not change

Thanks for catching this! It was totally unintended on my part.

Comment 19

[Greg Tatum [:gregtatum]]

https://reviewboard.mozilla.org/r/68908/#review66100

> I think this change makes this code a bit harder to understand, but guessing it's because of the lint rule of assignment in conditionals and while loops, ugh

Yeah, it felt gross changing it.

Comment 20

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3297025e72

Comment 21

[Greg Tatum [:gregtatum]]

Comment on attachment 8777331 [details]
Bug 1290142 - Lint devtools/client/performance/components;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68904/diff/1-2/

Comment 22

[Greg Tatum [:gregtatum]]

Comment on attachment 8777332 [details]
Bug 1290142 - Lint devtools/client/performance/legacy;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68906/diff/1-2/

Comment 23

[Greg Tatum [:gregtatum]]

Comment on attachment 8777333 [details]
Bug 1290142 - Lint devtools/client/performance/modules/ subfolders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68908/diff/1-2/

Comment 24

[Greg Tatum [:gregtatum]]

Comment on attachment 8777334 [details]
Bug 1290142 - Lint devtools/client/performance/modules/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68910/diff/1-2/

Comment 25

[Greg Tatum [:gregtatum]]

Comment on attachment 8777335 [details]
Bug 1290142 - Lint devtools/client/performance/test/unit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68912/diff/1-2/

Comment 26

[Greg Tatum [:gregtatum]]

Comment on attachment 8777336 [details]
Bug 1290142 - Lint devtools/client/performance/test/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68914/diff/1-2/

Comment 27

[Greg Tatum [:gregtatum]]

Comment on attachment 8777336 [details]
Bug 1290142 - Lint devtools/client/performance/test/;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68914/diff/2-3/

Comment 28

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=198db8249e51

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/642df8f89dbb
Lint devtools/client/performance/components; r=jsantell
https://hg.mozilla.org/integration/autoland/rev/7f81600de73c
Lint devtools/client/performance/legacy; r=jsantell
https://hg.mozilla.org/integration/autoland/rev/4f309f497720
Lint devtools/client/performance/modules/ subfolders; r=jsantell
https://hg.mozilla.org/integration/autoland/rev/3da4773d50ba
Lint devtools/client/performance/modules/; r=jsantell
https://hg.mozilla.org/integration/autoland/rev/6e88bf1b9a41
Lint devtools/client/performance/test/unit; r=jsantell
https://hg.mozilla.org/integration/autoland/rev/cda0d6b3f71b
Lint devtools/client/performance/test/; r=jsantell

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/642df8f89dbb
https://hg.mozilla.org/mozilla-central/rev/7f81600de73c
https://hg.mozilla.org/mozilla-central/rev/4f309f497720
https://hg.mozilla.org/mozilla-central/rev/3da4773d50ba
https://hg.mozilla.org/mozilla-central/rev/6e88bf1b9a41
https://hg.mozilla.org/mozilla-central/rev/cda0d6b3f71b