1144034 - Flamegraph text is barely readable on non-retina display

Status: VERIFIED FIXED

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

Description

[Paul Rouget [:paul]]

(tested with Windows)

Comment 1

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

A screenshot would really help.

Comment 2

[Tim Nguyen :ntim]

Attachment: Screenshot

Yeah, this looks pretty terrible on Windows.
I think the theme switching support regressed this.

It should use the message-box font.

Comment 3

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

My eyes are literally dying. </insidejoke>

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: Screenshot on OS X

Also true on OS X, copying my screenshot form the other bug.

Comment 6

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

With the landing of bug 1121180, which increased the font size, block height, and changed the background colors, is this looking better? In m-c, or probably tomorrow's nightly.

Comment 7

[Paul Rouget [:paul]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> With the landing of bug 1121180, which increased the font size, block
> height, and changed the background colors, is this looking better? In m-c,
> or probably tomorrow's nightly.

I haven't tested, but without resizing and redrawing the canvas, I don't think it will work.

Comment 8

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

Looking at the screenshots again I think the DPI used is backwards,I have half the amount of blocks vertically on my retina.

Comment 9

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #7)
> I haven't tested, but without resizing and redrawing the canvas, I don't
> think it will work.

Sorry, I was thinking about another bug. Ignore this comment.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Victor, marking this for devedition 40. It's quite painful on non-retina screens, where is this in your queue?

Comment 11

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

Fairly high, P1 and I'm assigned.

Comment 12

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

Attachment: flame-font-non-retina.patch

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8604693 [details] [diff] [review]
flame-font-non-retina.patch

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

::: browser/devtools/shared/widgets/FlameGraph.js
@@ +42,3 @@
>  const FLAME_GRAPH_BLOCK_BORDER = 1; // px
> +const FLAME_GRAPH_BLOCK_TEXT_FONT_SIZE = 10; // px
> +const FLAME_GRAPH_BLOCK_TEXT_FONT_FAMILY = "Helvetica Neue, Helvetica, sans-serif";

Windows ? :(

Comment 14

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

Helvetica exists on windows by default no?

Comment 15

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

Probably safe to add Arial there though.

Comment 16

[Tim Nguyen :ntim]

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> Probably safe to add Arial there though.

The font: message-box shorthand works fine for all platforms.
You could fallback to Segoe UI otherwise.

Comment 17

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

(In reply to Tim Nguyen [:ntim] (limited availability) from comment #16)
> 
> The font: message-box shorthand works fine for all platforms.

Does it look good though?

Comment 18

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

Comment on attachment 8604693 [details] [diff] [review]
flame-font-non-retina.patch

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

This will most def require updates to tests.

On my I M A C the blocks are more easy to read, but still quite hard just because of the kerning. Maybe there's some other solution where we change the size based on res, and still let the user change the block size, because this one will be tricky to have look good in all scenarios, I think. Or we can go even bigger, because the user can always scroll up/down on the flames, but they can't increase the "zoom" [currently anyway].

With the brightness/opacity changes (esp in dark theme), the blocks are too dark/hard to distinguish from differences in colors between each other -- would prefer the older brightness/opacity, as I don't think that this improves readability (I think the contrast is more stark now too, making it a bit hard to read)

With tests passing, a revert to the opacity/brightness unless there's strong feelings otherwise, and see if making the blocks even bigger helps, like 1.25x what is here, r+.

We should try and get f+ from someone on linux/windows.

Comment 19

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #18)
> Comment on attachment 8604693 [details] [diff] [review]
> 
> With the brightness/opacity changes (esp in dark theme), the blocks are too
> dark/hard to distinguish from differences in colors between each other --
> would prefer the older brightness/opacity, as I don't think that this
> improves readability (I think the contrast is more stark now too, making it
> a bit hard to read)

The old colors were gnarly to my eyes. Meet you in the middle?

Comment 20

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

Ya sounds good. Mainly want each block not blending into its neighbor, text legible and not searing sharp (like #fff text on #000). The light colors looked good before but yeah sometimes dark theme got a little too bold..

Comment 21

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

Attachment: v2

Fixed tests, addressed comments.

Comment 22

[Pulsebot]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2041288ab441

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8607647 [details] [diff] [review]
v2


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None

Comment 25

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/222c2b8e7b91

Comment 26

[Brian Grinstead [:bgrins]]

Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8607647 [details] [diff] [review]
v2

Change approved to skip one train as part of the spring campaign.

Comment 28

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 40.0a2 (2015-06-09) using Windows 7 (x64), Ubuntu 13.10 (x64) and Mac OS X 10.9.5, each environment with non-hidpi displays. 

The data available in the "Flame Chart" detail view is now reasonably displayed on non-hidpi displays as well. Also (re)confirmed that the Flame Chart is still displayed properly on a MacBook with Retina (OS X 10.9.5).