Closed
Bug 1290142
Opened 9 years ago
Closed 9 years ago
Eslint the performance tool client
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)
Tracking
(firefox50 affected, firefox51 fixed)
RESOLVED
FIXED
Firefox 51
People
(Reporter: gregtatum, Assigned: gregtatum)
Details
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gtatum
Priority: -- → P1
Assignee | ||
Comment 1•9 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 2•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68906/
Assignee | ||
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68908/
Assignee | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68910/
Assignee | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68912/
Assignee | ||
Comment 6•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68914/
Assignee | ||
Comment 7•9 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.
Updated•9 years ago
|
Attachment #8777331 -
Flags: review?(jsantell) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8777331 [details]
Bug 1290142 - Lint devtools/client/performance/components;
https://reviewboard.mozilla.org/r/68904/#review66090
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
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•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8777334 -
Flags: review?(jsantell) → review+
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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 15•9 years ago
|
||
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+
Comment 16•9 years ago
|
||
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•9 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•9 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•9 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 20•9 years ago
|
||
Assignee | ||
Comment 21•9 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•9 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•9 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•9 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•9 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•9 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•9 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/
Assignee | ||
Comment 28•9 years ago
|
||
Comment 29•9 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
Comment 30•9 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•