Support dark theme for flamechart

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ntim, Assigned: jsantell)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
On dark theme, there are 2 issues :
- The timestamp text is invisible (black text on dark background)
- The time "dividers" are too bright
(Assignee)

Updated

3 years ago
Blocks: 1110550
Blocks: 1075567
No longer blocks: 1075567, 1077459
(Assignee)

Updated

3 years ago
No longer blocks: 1110550
(Assignee)

Updated

3 years ago
Blocks: 1145697
(Assignee)

Updated

3 years ago
Blocks: 1149846
Created attachment 8588116 [details] [diff] [review]
1121180-dark-flamegraph.patch

Lowered opacity, increased saturation/brightness on blocks -- looks better.
Moved the timeline stroke ticks to render behind the flames (visible because blocks are more transparent, but do not get in the way of the text, which was really apparent in the dark theme)
Colors now appropriate for each theme
FlameGraph now a commonjs module
FlameGraph now has `force` option for `refresh`

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4591a6eafb86
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8588116 - Flags: review?(vporof)
(Assignee)

Updated

3 years ago
Blocks: 1138928
(Reporter)

Comment 2

3 years ago
Comment on attachment 8588116 [details] [diff] [review]
1121180-dark-flamegraph.patch

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

This looks much better ! One thing though, can you reduce the opacity of the dividers a bit ? They are much brighter than the ones in the overview view.
Comment on attachment 8588116 [details] [diff] [review]
1121180-dark-flamegraph.patch

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

Screenshot?

::: browser/devtools/shared/widgets/FlameGraph.js
@@ +31,5 @@
>  const OVERVIEW_HEADER_TEXT_FONT_SIZE = 9; // px
>  const OVERVIEW_HEADER_TEXT_FONT_FAMILY = "sans-serif";
>  const OVERVIEW_HEADER_TEXT_PADDING_LEFT = 6; // px
>  const OVERVIEW_HEADER_TEXT_PADDING_TOP = 5; // px
> +const OVERVIEW_HEADER_TIMELINE_STROKE_COLOR = "#aaaaaa";

Possibly rgba this, so the dividers aren't so gnarly. Using rgba(128,136,144,0.15) like the netmonitor could possibly work, or just (128 ..., 0.25).

@@ +928,5 @@
>      }
>    }
>  };
>  
> +const FLAME_GRAPH_BLOCK_HEIGHT = 12; // px

Might want to change this in the other bug regarding text readability.
Attachment #8588116 - Flags: review?(vporof) → review+
Screenshot from bug 1138928 has this patch applied https://bug1138928.bugzilla.mozilla.org/attachment.cgi?id=8588127
Bug 1144034 for flamechart readability is partially size, but the windows screenshots there I think is something else.
Created attachment 8588350 [details] [diff] [review]
1121180-dark-flamegraph.patch

Updated timeline stroke color -- looks good!
Attachment #8588116 - Attachment is obsolete: true
Attachment #8588350 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Created attachment 8588372 [details] [diff] [review]
1121180-dark-flamegraph.patch

Had to update tests due to size changes. Flame graph tests have always failed locally for me, so this'll be fun to test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=403f551d5adc
Attachment #8588350 - Attachment is obsolete: true
Attachment #8588372 - Flags: review+
Created attachment 8588394 [details] [diff] [review]
1121180-dark-flamegraph.patch

You should probably check out the changes to the tests, to make sure everything still makes sense, because of the text size change

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b1ca9d91b4a
Attachment #8588372 - Attachment is obsolete: true
Attachment #8588394 - Flags: review?(vporof)
Comment on attachment 8588394 [details] [diff] [review]
1121180-dark-flamegraph.patch

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

::: browser/devtools/shared/test/browser_flame-graph-utils-01.js
@@ +253,2 @@
>      width: 40,
> +    height: 12,

Maybe take the opportunity to make all of these computations dynamic based on the const? Here and in all the other tests. Or file a followup.
Attachment #8588394 - Flags: review?(vporof) → review+
Created attachment 8588684 [details] [diff] [review]
1121180-dark-flamegraph.patch

Tests now use FLAME_GRAPH_BLOCK_HEIGHT constant
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e240af53dc2c
Attachment #8588394 - Attachment is obsolete: true
Attachment #8588684 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/dbc2bad09146
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]

Comment 12

3 years ago
https://hg.mozilla.org/mozilla-central/rev/dbc2bad09146
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.