Eslint the performance tool client

RESOLVED FIXED in Firefox 51

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: gregtatum, Assigned: gregtatum)

Tracking

50 Branch
Firefox 51

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

3 years ago
No description provided.
Assignee

Updated

3 years ago
Assignee: nobody → gtatum
Priority: -- → P1
Assignee

Comment 1

3 years ago
Review commit: https://reviewboard.mozilla.org/r/68904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68904/
Attachment #8777331 - Flags: review?(jsantell)
Attachment #8777332 - Flags: review?(jsantell)
Attachment #8777333 - Flags: review?(jsantell)
Attachment #8777334 - Flags: review?(jsantell)
Attachment #8777335 - Flags: review?(jsantell)
Attachment #8777336 - Flags: review?(jsantell)
Assignee

Comment 7

3 years ago
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.
Attachment #8777331 - Flags: review?(jsantell) → review+
Comment on attachment 8777332 [details]
Bug 1290142 - Lint devtools/client/performance/legacy;

https://reviewboard.mozilla.org/r/68906/#review66094
Attachment #8777332 - Flags: review?(jsantell) → review+
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 on attachment 8777333 [details]
Bug 1290142 - Lint devtools/client/performance/modules/ subfolders;

https://reviewboard.mozilla.org/r/68908/#review66102
Attachment #8777333 - Flags: review?(jsantell) → review+
Attachment #8777334 - Flags: review?(jsantell) → review+
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 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
Attachment #8777335 - Flags: review?(jsantell) → review+
Comment on attachment 8777336 [details]
Bug 1290142 - Lint devtools/client/performance/test/;

https://reviewboard.mozilla.org/r/68914/#review66112
Attachment #8777336 - Flags: review?(jsantell) → review+
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.
Assignee

Comment 17

3 years ago
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.
Assignee

Comment 18

3 years ago
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.
Assignee

Comment 19

3 years ago
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.
Assignee

Comment 21

3 years ago
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/
Assignee

Comment 22

3 years ago
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/
Assignee

Comment 23

3 years ago
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/
Assignee

Comment 24

3 years ago
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/
Assignee

Comment 25

3 years ago
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/
Assignee

Comment 26

3 years ago
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/
Assignee

Comment 27

3 years ago
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 29

3 years ago
Pushed by gtatum@mozilla.com:
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

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.