1121180 - Support dark theme for flamechart

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Tim Nguyen :ntim]

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

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 2

[Tim Nguyen :ntim]

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 3

[Victor Porof [:vporof][:vp]]

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.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Screenshot from bug 1138928 has this patch applied https://bug1138928.bugzilla.mozilla.org/attachment.cgi?id=8588127

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Bug 1144034 for flamechart readability is partially size, but the windows screenshots there I think is something else.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1121180-dark-flamegraph.patch

Updated timeline stroke color -- looks good!

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 9

[Victor Porof [:vporof][:vp]]

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.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1121180-dark-flamegraph.patch

Tests now use FLAME_GRAPH_BLOCK_HEIGHT constant
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e240af53dc2c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/dbc2bad09146

Comment 12

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/dbc2bad09146