[VrShell] Centralize color handling and enable close button on fullscreen

[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997, 729729
Review-Url: https://codereview.chromium.org/2914623003
Cr-Commit-Position: refs/heads/master@{#477411}
Committed: https://chromium.googlesource.com/chromium/src/+/f9189aa4473f6915826d070f6e3fb008a2904d8f

[amp, 2017-05-30 23:19:26]

amp@chromium.org changed reviewers:
+ cjgrant@chromium.org, vollick@google.com

[amp, 2017-05-30 23:19:27]

Tests still need to be updated, but I wanted to send this out to make sure we
are all ok with the naming.

Note that there are a few elements that define their own background and
foreground colors (the security warnings etc).  These don't change between modes
(I think they only show up in WebVR), so maybe they are fine being hardcoded,
but we could add them to the color scheme if we want a single location for all
colors.

Last note, I haven't been able to get the close button to respond correctly on
mode switch.  It still gets drawn with normal colors instead of fullscreen, and
if I hover over it it doesn't revert back to non-hover.  I'm not sure what I've
done.  But the button itself works fine for switching between modes.

[Ian Vollick, 2017-05-30 23:47:14]

Description was changed from

==========
[VrShell] Enable close button for fullscreen

This also updates the close button to use colors from the scheme instead
of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

to

==========
[VrShell] Enable close button for fullscreen

This also updates the close button to use colors from the scheme instead
of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

[Ian Vollick, 2017-05-30 23:47:14]

vollick@chromium.org changed reviewers:
+ vollick@chromium.org
- vollick@google.com

[cjgrant, 2017-05-31 15:39:28]

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:24: SkColor world_background;
This more verbose naming looks great to me.

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/close_button_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/close_button_texture.cc:45: SkColor
color = hovered() ? color_scheme().element_background_hover
In a ymalik@ CL, another element changed this ternary use to if/else if/else for
readability.  Feel free to do that here as well unless you prefer this.

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:341:
close_button_->SetEnabled(fullscreen_);
Does this break the button for CCT mode, by hiding it when not in fullscreen? 
If so, do the current tests catch this?tests cat

[amp, 2017-06-02 01:07:50]

Description was changed from

==========
[VrShell] Enable close button for fullscreen

This also updates the close button to use colors from the scheme instead
of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

to

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

[amp, 2017-06-02 01:08:36]

PTAL.  Still need to get tests working (and verify proper behavior with hover
and in cct), but otherwise it should be pretty close now.

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:24: SkColor world_background;
On 2017/05/31 15:39:28, cjgrant wrote:
> This more verbose naming looks great to me.

Acknowledged.

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/close_button_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/close_button_texture.cc:45: SkColor
color = hovered() ? color_scheme().element_background_hover
On 2017/05/31 15:39:28, cjgrant wrote:
> In a ymalik@ CL, another element changed this ternary use to if/else if/else
for
> readability.  Feel free to do that here as well unless you prefer this.

I think I saw that but it felt like slightly different scenario's so I left this
as is (if I'm thinking of a different one it might still be applicable, but I
think the ternary is fine).

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:341:
close_button_->SetEnabled(fullscreen_);
On 2017/05/31 15:39:28, cjgrant wrote:
> Does this break the button for CCT mode, by hiding it when not in fullscreen? 
> If so, do the current tests catch this?

It looks like it probably does.  I don't think we have any tests for this and
I'm not sure how to verify cct right now, but I'll do that and try to add some
tests before submitting (and make sure that the code works for cct in and out of
fullscreen).

[cjgrant, 2017-06-02 12:51:48]

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:341:
close_button_->SetEnabled(fullscreen_);
On 2017/06/02 01:08:35, amp wrote:
> On 2017/05/31 15:39:28, cjgrant wrote:
> > Does this break the button for CCT mode, by hiding it when not in
fullscreen? 
> > If so, do the current tests catch this?
> 
> It looks like it probably does.  I don't think we have any tests for this and
> I'm not sure how to verify cct right now, but I'll do that and try to add some
> tests before submitting (and make sure that the code works for cct in and out
of
> fullscreen).

We do have a CctButtonVisibleInCct test, which covers its visibility when the
scene manager is initiated with the relevant states.  I think you'd just want to
generalize the test name, and extend what's there to cover transitions as well. 
What do you think of this (note that you'd have to tweak the cases for your CL):

https://paste.googleplex.com/6588573297934336

[amp, 2017-06-02 21:03:20]

Description was changed from

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

to

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997,728147
==========

[amp, 2017-06-02 21:03:47]

Description was changed from

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997,728147
==========

to

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

[amp, 2017-06-02 21:40:24]

I still need to merge this with Aldo's last change (and make sure I don't lose
any of the color adjustments).

Also the cct test passes but I should probably verify it actually works in cct. 
What's the best way to verify cct?  (I'm guessing something like set the cct
enabled flag and then try and launch it from gmail or something?).

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2914623003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:341:
close_button_->SetEnabled(fullscreen_);
On 2017/06/02 12:51:47, cjgrant wrote:
> On 2017/06/02 01:08:35, amp wrote:
> > On 2017/05/31 15:39:28, cjgrant wrote:
> > > Does this break the button for CCT mode, by hiding it when not in
> fullscreen? 
> > > If so, do the current tests catch this?
> > 
> > It looks like it probably does.  I don't think we have any tests for this
and
> > I'm not sure how to verify cct right now, but I'll do that and try to add
some
> > tests before submitting (and make sure that the code works for cct in and
out
> of
> > fullscreen).
> 
> We do have a CctButtonVisibleInCct test, which covers its visibility when the
> scene manager is initiated with the relevant states.  I think you'd just want
to
> generalize the test name, and extend what's there to cover transitions as
well. 
> What do you think of this (note that you'd have to tweak the cases for your
CL):
> 
> https://paste.googleplex.com/6588573297934336

Updated.  I changed the test to be about the button specifically and covered
both in cct and not (let me know if you want a specific cct test).  I also had
the logic here wrong multiple patches.  Everything should be good on the 5th
patch set (tests are all passing).

[amp, 2017-06-02 23:49:37]

amp@chromium.org changed reviewers:
+ joshcarpenter@google.com

[amp, 2017-06-02 23:49:38]

PTAL.  Everything should be synced up and and tests passing now (haven't
verified the hover states in fullscreen yet though).

Please spend some extra effort in making sure I didn't inadvertently change a
color somewhere.

+Josh to look at the overall colors in general (all elements should be in the
scheme now).

[cjgrant, 2017-06-05 17:57:09]

lgtm with a few comments.  Thanks for rejigging all this!

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/color_scheme.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.cc:67:
fullscreen_scheme.text_emphasized = 0xFFFFFFFF;
Sk_ColorWHITE (used above).  I think we should use the constants for at least
white and black (see instances of FF000000), or alternatively, not use the
constants at all, as we have so many colors here.  Either way, not a mix of
both.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.cc:78:
incognito_scheme.element_foreground = 0xFFBCBCBC;
The order here is different than the block above, which I think could be
confusing (see previous comment).

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.h:30: SkColor element_foreground;
Should live with other element_ members below, and also, the order of elements
here should match the order in initialization below, for readability.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/ui_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/ui_texture.cc:16: #include
"chrome/browser/android/vr_shell/color_scheme.h"
This is already in ui_texture.h

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:82: emphasis ?
color_scheme.text_emphasized : color_scheme.text_deemphasized;
Could these be url_emphasized, etc, to call out that they are URL-specific (ie.
security concerns)?

[amp, 2017-06-05 21:06:55]

Description was changed from

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997
==========

to

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997,729729
==========

[amp, 2017-06-05 21:39:47]

The CQ bit was checked by amp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 21:40:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amp, 2017-06-05 21:40:58]

PTAL.  Reorganized the scheme a bit and had to change some of the OnModeSet
wiring to get the button to show up in the proper color when entering
fullscreen.

Still trying to verify in cct, but I have to make at least one more patchset to
remove the cta change anyway.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/color_scheme.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.cc:67:
fullscreen_scheme.text_emphasized = 0xFFFFFFFF;
On 2017/06/05 17:57:08, cjgrant wrote:
> Sk_ColorWHITE (used above).  I think we should use the constants for at least
> white and black (see instances of FF000000), or alternatively, not use the
> constants at all, as we have so many colors here.  Either way, not a mix of
> both.

Done.  Actually this value is never used in fullscreen (no url bar) so I just
removed it.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.cc:78:
incognito_scheme.element_foreground = 0xFFBCBCBC;
On 2017/06/05 17:57:08, cjgrant wrote:
> The order here is different than the block above, which I think could be
> confusing (see previous comment).

Done.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/color_scheme.h:30: SkColor element_foreground;
On 2017/06/05 17:57:08, cjgrant wrote:
> Should live with other element_ members below, and also, the order of elements
> here should match the order in initialization below, for readability.

Done.  I moved the text (now url) colors down with the security parts which
seemed to make more sense and collapsed the element_xxx parts together

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/ui_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/ui_texture.cc:16: #include
"chrome/browser/android/vr_shell/color_scheme.h"
On 2017/06/05 17:57:08, cjgrant wrote:
> This is already in ui_texture.h

Done.

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2914623003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:82: emphasis ?
color_scheme.text_emphasized : color_scheme.text_deemphasized;
On 2017/06/05 17:57:08, cjgrant wrote:
> Could these be url_emphasized, etc, to call out that they are URL-specific
(ie.
> security concerns)?

Done.  sgtm since they aren't used anywhere else.

https://codereview.chromium.org/2914623003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2914623003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:2171:
return !VrShellDelegate.isInVr();
This change wasn't supposed to show up (it's supposed to be a dependent
patchset, but I guess that stopped working for some reason).

It will be removed before submitting.

[commit-bot: I haz the power, 2017-06-06 00:16:07]

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

[commit-bot: I haz the power, 2017-06-06 00:16:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amp, 2017-06-06 19:21:48]

The CQ bit was checked by amp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 19:22:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 20:11:43]

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

[commit-bot: I haz the power, 2017-06-06 20:11:44]

Dry run: This issue passed the CQ dry run.

[amp, 2017-06-06 20:35:14]

The CQ bit was checked by amp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 20:35:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cjgrant, 2017-06-06 20:44:01]

lgtm

[amp, 2017-06-06 20:47:54]

The CQ bit was unchecked by amp@chromium.org

[amp, 2017-06-06 20:47:58]

The CQ bit was checked by amp@chromium.org

[commit-bot: I haz the power, 2017-06-06 20:48:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 21:33:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496782078699090,
"parent_rev": "bb2a003f3be6c19476e36fb1b12141b2d8139e21", "commit_rev":
"f9189aa4473f6915826d070f6e3fb008a2904d8f"}

[commit-bot: I haz the power, 2017-06-06 21:33:22]

Description was changed from

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997,729729
==========

to

==========
[VrShell] Centralize color handling and enable close button on fullscreen

This updates all of the existing hard coded colors to use colors from the scheme
instead of hardcoding (so they can change with the mode).

The naming of colors in the scheme was also updated for better
consistency and to differentiate from world environment colors.

BUG=719997,729729

Review-Url: https://codereview.chromium.org/2914623003
Cr-Commit-Position: refs/heads/master@{#477411}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9189aa4473f6915826d070f6e3f...
==========

[commit-bot: I haz the power, 2017-06-06 21:33:23]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f9189aa4473f6915826d070f6e3f...