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)

37 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

This was missed in the patch that added multiple recordings due to them being landed concurrently
Blocks: perf-tool-v2
Depends on: 1121086, 1111004
No longer depends on: 1121086
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
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 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+
Consolidated stopped/selected for all detail views since they're all the same
Attachment #8548948 - Attachment is obsolete: true
Attachment #8549788 - Flags: review+
Comment on attachment 8549788 [details] [diff] [review]
1121196-rerender-flame-graph.patch

Review of attachment 8549788 [details] [diff] [review]:
-----------------------------------------------------------------

Awesomesauce
Try link? :)
Keywords: checkin-needed
(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
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: