Closed
Bug 1153473
Opened 10 years ago
Closed 2 years ago
Idle blocks should be uniquely colored and consistent in flame graph
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: jsantell, Unassigned)
References
Details
(Whiteboard: [difficulty=medium][lang=js])
Attachments
(2 files)
15.66 KB,
image/png
|
Details | |
7.20 KB,
patch
|
Details | Diff | Splinter Review |
The flame graph renders idle blocks as "(idle)" when displaying only content (when platform data is shown, determining what is "idle" is handled in bug 1152839) - other top level frames can get the same color, making it hard to distinguish from afar what is idle and what is activity.
At the very least, idle nodes should be unique to the entire graph. Better yet, a good color, as colors are generated randomly on first use per toolbox, so users can get used to seeing "idle" nodes as an identifying color.
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
I don't think this is a blocker, and also we'd need more platform support on this.
No longer blocks: perf-tool-papercuts
Comment 2•10 years ago
|
||
I could still special case them and add an extendable mechanism in place.
Comment 3•10 years ago
|
||
Attachment #8604683 -
Flags: review?(jsantell)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8604683 [details] [diff] [review]
idle-colors.patch
Review of attachment 8604683 [details] [diff] [review]:
-----------------------------------------------------------------
Needs some tests and good to go I think if the meta category stuff was handled in Shu's patch or something. But otherwise, may need bug 1147693, and at the very least, tests for the getColor/getLabel utils, if not doing a full flame graph test
::: browser/devtools/performance/views/details-js-call-tree.js
@@ +115,5 @@
> this.container.innerHTML = "";
> root.attachTo(this.container);
>
> // When platform data isn't shown, hide the cateogry labels, since they're
> + // only available for C++ frames. // Pass *false* to make them invisible.
cateogry
comment within a comment
@@ +116,5 @@
> root.attachTo(this.container);
>
> // When platform data isn't shown, hide the cateogry labels, since they're
> + // only available for C++ frames. // Pass *false* to make them invisible.
> + root.toggleCategories(!options.contentOnly);
Oh. Is this bug 1152631?
::: browser/devtools/shared/widgets/FlameGraph.js
@@ +1179,5 @@
> + if (FrameUtils.isRoot(frame)) {
> + return SPECIAL_COLORS.ROOT;
> + }
> + if (frame.isMetaCategory) {
> + return CATEGORY_MAPPINGS[frame.category].color;
Nice. Make those metacategories all look the same too! Should have a bug for using themeable "colorNames" like markers or something
@@ +1199,5 @@
> // anyway.
> if (frame.idle) {
> return frame.location;
> }
> + if (frame.isMetaCategory) {
Doesn't this require, and all the meta category stuff, bug 1147693?
Attachment #8604683 -
Flags: review?(jsantell)
Updated•9 years ago
|
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Whiteboard: [difficulty=medium][good first bug][mentor=vp][lang=js]
Updated•7 years ago
|
Keywords: good-first-bug
Whiteboard: [difficulty=medium][good first bug][mentor=vp][lang=js] → [difficulty=medium][lang=js]
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 6•2 years ago
|
||
Now that we replaced the old performance panel by the Firefox Profiler (see Bug 1668219) we're closing bugs related to this. If you see similar issues on the new profiler, please file another bug, and we'll be happy to have a look.
Filter on MASSCLOSEOLDPERFTOOLBUGS.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•