Closed Bug 1290142 Opened 9 years ago Closed 9 years ago

Eslint the performance tool client

Categories

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

50 Branch
defect

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

Details

Attachments

(6 files)

No description provided.
Assignee: nobody → gtatum
Priority: -- → P1
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)
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+
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+
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.
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.
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.
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 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 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 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 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 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 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 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/
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: