Closed Bug 1121180 Opened 5 years ago Closed 5 years ago

Support dark theme for flamechart

Categories

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

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: ntim, Assigned: jsantell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

On dark theme, there are 2 issues :
- The timestamp text is invisible (black text on dark background)
- The time "dividers" are too bright
No longer blocks: perf-tool-v2, 1077459
No longer blocks: 1110550
Attached patch 1121180-dark-flamegraph.patch (obsolete) — Splinter Review
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)
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+
Bug 1144034 for flamechart readability is partially size, but the windows screenshots there I think is something else.
Attached patch 1121180-dark-flamegraph.patch (obsolete) — Splinter Review
Updated timeline stroke color -- looks good!
Attachment #8588116 - Attachment is obsolete: true
Attachment #8588350 - Flags: review+
Attached patch 1121180-dark-flamegraph.patch (obsolete) — Splinter Review
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+
Attached patch 1121180-dark-flamegraph.patch (obsolete) — Splinter Review
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+
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+
https://hg.mozilla.org/integration/fx-team/rev/dbc2bad09146
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dbc2bad09146
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.