Closed
Bug 1121196
Opened 9 years ago
Closed 9 years ago
FlameGraph should be toggled when switching recordings in perf tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 38
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 2 obsolete files)
11.48 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
This was missed in the patch that added multiple recordings due to them being landed concurrently
Assignee | ||
Updated•9 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
This rerenders the flamegraph when switching between recordings. Also, added few more isRecording checks so we dont rerender when going to a currently recording model. This looks a bit weird, but will be more performant, and will look nicer once bug 1121086 lands, which will hide details view for an active recording. Try tree closed, but this doesn't touch any other code anyway as no performance tests get run.
Attachment #8548948 -
Flags: review?(vporof)
Comment 2•9 years ago
|
||
Comment on attachment 8548948 [details] [diff] [review] 1121196-rerender-flame-graph.patch Review of attachment 8548948 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/views/details-flamegraph.js @@ +63,5 @@ > + > + /** > + * Called when a recording has been selected. > + */ > + _onRecordingSelected: function (_, recording) { Looks like 'stopped' and 'selected' are synonymous here. Maybe it'd be nice to refactor this into using a single function. Better yet, make sure that's the case everywhere in the new perf tool (if two event listeners do the exact same thing, then just have a single function).
Attachment #8548948 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Consolidated stopped/selected for all detail views since they're all the same
Attachment #8548948 -
Attachment is obsolete: true
Attachment #8549788 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 4•9 years ago
|
||
Comment on attachment 8549788 [details] [diff] [review] 1121196-rerender-flame-graph.patch Review of attachment 8549788 [details] [diff] [review]: ----------------------------------------------------------------- Awesomesauce
Assignee | ||
Comment 6•9 years ago
|
||
Freshly rebased, and with a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d93a5378a96
Attachment #8549788 -
Attachment is obsolete: true
Attachment #8550355 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2b94eb1efedf
Whiteboard: [fixed-in-fx-team]
Comment 8•9 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #7) > https://hg.mozilla.org/integration/fx-team/rev/2b94eb1efedf seems i can remove the checkin-needed request :)
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b94eb1efedf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•