FlameGraph should be toggled when switching recordings in perf tool

RESOLVED FIXED in Firefox 38

Status

DevTools
Performance Tools (Profiler/Timeline)
RESOLVED FIXED
4 years ago
9 days ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

37 Branch
Firefox 38
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This was missed in the patch that added multiple recordings due to them being landed concurrently
(Assignee)

Updated

4 years ago
Blocks: 1075567
Depends on: 1121086, 1111004
(Assignee)

Updated

4 years ago
No longer depends on: 1121086
(Assignee)

Updated

4 years ago
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Created attachment 8548948 [details] [diff] [review]
1121196-rerender-flame-graph.patch

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+
Created attachment 8549788 [details] [diff] [review]
1121196-rerender-flame-graph.patch

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

4 years ago
Keywords: checkin-needed
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
Created attachment 8550355 [details] [diff] [review]
1121196-rerender-flame-graph.patch

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

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38

Updated

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