Change HUD colors to distinguish status easily.

Change HUD colors to distinguish status easily.

With red color on black background, the graph lines do not stand out
distinctly. So changed few colors to make HUD status more visible.

Also separated the titles for each display to make them visible and
added memory graph to easily understand the memory status.

BUG=541121
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/77e7f97042e41e91b9a7cb99fdc253b7b34187b2
Cr-Commit-Position: refs/heads/master@{#353727}

[prashant.n, 2015-09-29 13:57:22]

prashant.n@samsung.com changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[prashant.n, 2015-09-29 13:57:22]

ptal.

[prashant.n, 2015-10-05 15:45:25]

kindly ptal.

[danakj, 2015-10-07 16:00:39]

Can you give screenshots of before and after for all of the changes?

[prashant.n, 2015-10-08 12:41:29]

Attached the images in the bug created.

[weiliangc, 2015-10-08 17:34:06]

weiliangc@chromium.org changed reviewers:
+ weiliangc@chromium.org

[weiliangc, 2015-10-08 17:34:06]

Thanks for the images in the bug. This patch LGTM. Let's keeps further
discussions in the bug.

[danakj, 2015-10-08 17:48:10]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:334: if (rounded) {
I don't think it's really worth adding code to make a rounded border on the
display :/ Less code is better here.

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
Similar comment here, I don't know that these separators are adding that much.
The headers are nice how you did them. These lines seem extra. I'd remove that.

[prashant.n, 2015-10-09 01:01:52]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:334: if (rounded) {
On 2015/10/08 17:48:09, danakj_OOO_til_10-12 wrote:
> I don't think it's really worth adding code to make a rounded border on the
> display :/ Less code is better here.

Hmm. I will remove them.

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
On 2015/10/08 17:48:09, danakj_OOO_til_10-12 wrote:
> Similar comment here, I don't know that these separators are adding that much.
> The headers are nice how you did them. These lines seem extra. I'd remove
that.

Adding separators between different displays makes them as different categories
visually, so IMO we should have them.

[danakj, 2015-10-09 04:41:35]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
On 2015/10/09 01:01:52, prashant.n wrote:
> On 2015/10/08 17:48:09, danakj_OOO_til_10-12 wrote:
> > Similar comment here, I don't know that these separators are adding that
much.
> > The headers are nice how you did them. These lines seem extra. I'd remove
> that.
> 
> Adding separators between different displays makes them as different
categories
> visually, so IMO we should have them.

I disagee, as I would like as little code in the HUD as is reasonable, and I
don't think they are adding that much. You can tell what is going on without
them.

[prashant.n, 2015-10-09 05:05:12]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
Basically it shows that fps counter and GPU status displays are different. It
separates visually, so it is easier to understand what is contained in each
category of display by having visual boundary.

In the Chrome Menu, we have separators to differentiate the categories.
Similarly I thought to have separators in different displays to follow general
UX practice.

I understand that HUD should be simpler. But if we keep adding more displays on
hud then making boudaries between them will become essential and it makes
focusing on what developer expects easily.

[danakj, 2015-10-09 16:59:57]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
On 2015/10/09 05:05:12, prashant.n wrote:
> Basically it shows that fps counter and GPU status displays are different. It
> separates visually, so it is easier to understand what is contained in each
> category of display by having visual boundary.
> 
> In the Chrome Menu, we have separators to differentiate the categories.
> Similarly I thought to have separators in different displays to follow general
> UX practice.
> 
> I understand that HUD should be simpler. But if we keep adding more displays
on
> hud then making boudaries between them will become essential and it makes
> focusing on what developer expects easily.

This isn't user-targeted UI like the chrome UI is. It's an FPS meter for people
to use, and some other pieces of data for chrome developers more or less.
There's no need to build something extravagant here, it's not trying to be
devtools. The header titles already do a fine job explaining the UI.

[prashant.n, 2015-10-10 00:29:35]

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/40001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:673:
paint.setColor(DebugColors::HUDDisplaySeparatorColor());
IMO, even the ui is not user targetted, it is published and available using
chrome://flags, so that it can treated as important as devtools. On top of
chrome UI, it looks like different UI patched. It looks nice visually and
costwise it just adds simple rect with different color. If you feel it's
overhead, I'll remove it.

[prashant.n, 2015-10-12 17:26:04]

Regarding display separator, we can discuss in the bug.

[danakj, 2015-10-12 18:42:53]

On Mon, Oct 12, 2015 at 10:26 AM, <prashant.n@samsung.com> wrote:

> Regarding display separator, we can discuss in the bug.
>

If you want. I do agree it looks nicer with separator. I don't think it's
worth adding code for it.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[prashant.n, 2015-10-12 19:16:01]

Hmm. I'll readd separator then.

[danakj, 2015-10-12 20:47:58]

On Mon, Oct 12, 2015 at 12:16 PM, <prashant.n@samsung.com> wrote:

> Hmm. I'll readd separator then.
>

Maybe misread: I don't think it's worth adding code for it.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-10-12 20:51:56]

Patchset 5 LGTM w/ a couple nits.

https://codereview.chromium.org/1377823002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (left):

https://codereview.chromium.org/1377823002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:302: 
unrelated whitespace changes?

https://codereview.chromium.org/1377823002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1377823002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:277: 
dont add whitespace please

https://codereview.chromium.org/1377823002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:281: if (debug_state.ShowMemoryStats()
&& !!memory_entry_.total_bytes_used)
don't need !!

[prashant.n, 2015-10-13 03:59:23]

The CQ bit was checked by prashant.n@samsung.com

[prashant.n, 2015-10-13 03:59:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1377823002/#ps120001
(title: "nits.")

[commit-bot: I haz the power, 2015-10-13 04:00:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377823002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377823002/120001

[commit-bot: I haz the power, 2015-10-13 04:37:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-13 04:37:37]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[prashant.n, 2015-10-13 12:05:27]

The CQ bit was checked by prashant.n@samsung.com

[prashant.n, 2015-10-13 12:05:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/1377823002/#ps140001
(title: "fixed typecasting warning.")

[commit-bot: I haz the power, 2015-10-13 12:05:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377823002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377823002/140001

[commit-bot: I haz the power, 2015-10-13 13:06:46]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-13 13:07:31]

Patchset 8 (id:??) landed as
https://crrev.com/77e7f97042e41e91b9a7cb99fdc253b7b34187b2
Cr-Commit-Position: refs/heads/master@{#353727}