[vr] Add incognito coloring

[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring
when in incognito mode.

BUG=715664
Review-Url: https://codereview.chromium.org/2902043005
Cr-Commit-Position: refs/heads/master@{#475234}
Committed: https://chromium.googlesource.com/chromium/src/+/c619c829f502b17a0a71a9d0e5c68df9baff5825

[Ian Vollick, 2017-05-25 00:16:58]

Description was changed from

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring.

BUG=
==========

to

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring.
Also incorporates coloring improvements from amp@ and
alpha premultiplication changes from acondor@

BUG=715664
==========

[Ian Vollick, 2017-05-25 00:16:58]

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

[Ian Vollick, 2017-05-25 00:19:38]

Description was changed from

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring.
Also incorporates coloring improvements from amp@ and
alpha premultiplication changes from acondor@

BUG=715664
==========

to

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring.
Also incorporates coloring improvements from amp@ and
alpha premultiplication changes from acondor@

This also introduces the notion of a color scheme.
Although this is only used for background-related
colors, we could easily add more.

BUG=715664
==========

[tiborg, 2017-05-25 00:39:06]

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

[tiborg, 2017-05-25 00:47:18]

lgtm % nits

Also, maybe change the title reflect the other changes.

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

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:12: // normal
Nit: capital N and period in the end.

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:19: // fullscreen
Here too.

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:26: // incognito -
TODO(joshcarpenter): pick real colors for this.
Here too.

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:37: /* static */
Do we need this comment?

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

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:141: // premultiply opacity
Nit: capital P and period in the end.

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:255: // premultiply alpha
Here too.

https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:420: // Our textures have
premultiplied alpha
Nit: period in the end.

[Ian Vollick, 2017-05-25 01:07:23]

On 2017/05/25 00:47:18, tiborg wrote:
> lgtm % nits
> 
> Also, maybe change the title reflect the other changes.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/color_scheme.cc (right):
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/color_scheme.cc:12: // normal
> Nit: capital N and period in the end.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/color_scheme.cc:19: // fullscreen
> Here too.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/color_scheme.cc:26: // incognito -
> TODO(joshcarpenter): pick real colors for this.
> Here too.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/color_scheme.cc:37: /* static */
> Do we need this comment?
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_renderer.cc:141: // premultiply
opacity
> Nit: capital P and period in the end.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_renderer.cc:255: // premultiply alpha
> Here too.
> 
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_renderer.cc:420: // Our textures have
> premultiplied alpha
> Nit: period in the end.

Thanks. I've addressed all feedback, though I think this issue should probably
be closed and landed in these pieces
 * introduce color scheme
 * add plumbing for incognito
 * premultiply alpha.

[Ian Vollick, 2017-05-26 04:05:00]

vollick@chromium.org changed reviewers:
+ cjgrant@chromium.org

[Ian Vollick, 2017-05-26 04:05:15]

On 2017/05/25 01:07:23, Ian Vollick wrote:
> On 2017/05/25 00:47:18, tiborg wrote:
> > lgtm % nits
> > 
> > Also, maybe change the title reflect the other changes.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > File chrome/browser/android/vr_shell/color_scheme.cc (right):
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/color_scheme.cc:12: // normal
> > Nit: capital N and period in the end.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/color_scheme.cc:19: // fullscreen
> > Here too.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/color_scheme.cc:26: // incognito -
> > TODO(joshcarpenter): pick real colors for this.
> > Here too.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/color_scheme.cc:37: /* static */
> > Do we need this comment?
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell_renderer.cc:141: // premultiply
> opacity
> > Nit: capital P and period in the end.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell_renderer.cc:255: // premultiply
alpha
> > Here too.
> > 
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > chrome/browser/android/vr_shell/vr_shell_renderer.cc:420: // Our textures
have
> > premultiplied alpha
> > Nit: period in the end.
> 
> Thanks. I've addressed all feedback, though I think this issue should probably
> be closed and landed in these pieces
>  * introduce color scheme
>  * add plumbing for incognito
>  * premultiply alpha.

Now that the color scheme and alpha pre-multiplication CLs have landed, this is
now just adding support for incognito mode and so I've reopened.

PTAL

[Ian Vollick, 2017-05-26 04:05:56]

Description was changed from

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring.
Also incorporates coloring improvements from amp@ and
alpha premultiplication changes from acondor@

This also introduces the notion of a color scheme.
Although this is only used for background-related
colors, we could easily add more.

BUG=715664
==========

to

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring
when in incognito mode.

BUG=715664
==========

[Ian Vollick, 2017-05-26 13:08:07]

On 2017/05/26 04:05:15, Ian Vollick wrote:
> On 2017/05/25 01:07:23, Ian Vollick wrote:
> > On 2017/05/25 00:47:18, tiborg wrote:
> > > lgtm % nits
> > > 
> > > Also, maybe change the title reflect the other changes.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > File chrome/browser/android/vr_shell/color_scheme.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/color_scheme.cc:12: // normal
> > > Nit: capital N and period in the end.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/color_scheme.cc:19: // fullscreen
> > > Here too.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/color_scheme.cc:26: // incognito -
> > > TODO(joshcarpenter): pick real colors for this.
> > > Here too.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/color_scheme.cc:37: /* static */
> > > Do we need this comment?
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/vr_shell_renderer.cc:141: // premultiply
> > opacity
> > > Nit: capital P and period in the end.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/vr_shell_renderer.cc:255: // premultiply
> alpha
> > > Here too.
> > > 
> > >
> >
>
https://codereview.chromium.org/2902043005/diff/20001/chrome/browser/android/...
> > > chrome/browser/android/vr_shell/vr_shell_renderer.cc:420: // Our textures
> have
> > > premultiplied alpha
> > > Nit: period in the end.
> > 
> > Thanks. I've addressed all feedback, though I think this issue should
probably
> > be closed and landed in these pieces
> >  * introduce color scheme
> >  * add plumbing for incognito
> >  * premultiply alpha.
> 
> Now that the color scheme and alpha pre-multiplication CLs have landed, this
is
> now just adding support for incognito mode and so I've reopened.
> 
> PTAL

Actually, hold off on this for a moment. I'd like to land
https://codereview.chromium.org/2911663002/ first.

[cjgrant, 2017-05-26 16:29:25]

lgtm % comments

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:172: ui_->SetURL(GURL());
It looks like a previous rebase yielded a duplicate SetURL call here.  Could you
pull out the second one?

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:177:
ui_->SetIncognito(web_contents_->GetBrowserContext()->IsOffTheRecord());
Shouldn't this be in the if() case above, setting it to false?

[amp, 2017-05-26 17:48:20]

lgtm.

Maybe add a small test to scene manager unit test to verify that the background
changes when going into icognito.

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

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:263:
element->set_center_color(color_scheme().horizon);
Aren't all the colors for the background being handled in the
'UpdateBackgroundColor' method now?

[Ian Vollick, 2017-05-27 00:30:18]

Biggish overhaul. Have added tests for both ui scene manager and url bar
texture.

PTAL

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

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:263:
element->set_center_color(color_scheme().horizon);
On 2017/05/26 17:48:20, amp wrote:
> Aren't all the colors for the background being handled in the
> 'UpdateBackgroundColor' method now?

Done.

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:172: ui_->SetURL(GURL());
On 2017/05/26 16:29:25, cjgrant wrote:
> It looks like a previous rebase yielded a duplicate SetURL call here.  Could
you
> pull out the second one?

Done.

https://codereview.chromium.org/2902043005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:177:
ui_->SetIncognito(web_contents_->GetBrowserContext()->IsOffTheRecord());
On 2017/05/26 16:29:25, cjgrant wrote:
> Shouldn't this be in the if() case above, setting it to false?

Done.

[amp, 2017-05-27 00:47:48]

Still lgtm modulo the floor grid color comment and maybe the tests (but the
tests are more of a nit).

The background naming question can wait until a future change.

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc
(right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:40: static
const SkColor kIncognitoDeemphasizedColor = 0xFFE6E6E6;
Are these redefining constants in the color scheme?  Is that the preferred way
for tests to verify them (instead of depending on the constants directly)?

I'm still coming up to speed on c++ testing, but it feels like more things to
keep in sync.

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:75: void SetBackgroundDistance(float
distance);
I think there is some overlap with the word 'background' now.

Perhaps we should change the non-text background to be something like
'environment'?

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

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.cc:369:
floor_grid_->set_center_color(color_scheme().horizon);
wait, why are we setting this to horizon?  We just set it to floor_grid on line
366?

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:147:
manager_->SetFullscreen(true);
Why do we need to check fullscreen?

Shouldn't we just set incognito and verify the colors between that and normal?

Although, cjgrant and I were discussing having one big test that went through
all the transitions.  I think I agree with him when he prefers that feature
specific tests should be more tightly focused.

[Ian Vollick, 2017-05-27 01:25:51]

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

[commit-bot: I haz the power, 2017-05-27 01:26:14]

Dry run: CQ is trying da patch.

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

[Ian Vollick, 2017-05-27 01:29:13]

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc
(right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:40: static
const SkColor kIncognitoDeemphasizedColor = 0xFFE6E6E6;
On 2017/05/27 00:47:47, amp wrote:
> Are these redefining constants in the color scheme?  Is that the preferred way
> for tests to verify them (instead of depending on the constants directly)?
> 
> I'm still coming up to speed on c++ testing, but it feels like more things to
> keep in sync.

I had commented on the same thing on the patch that added this test (and had
suggested that we share the color definition), but Chris had duplicated the
colors intentionally: the idea was that since these colors were chosen very
carefully, so if they change, this test should break.

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene.h:75: void SetBackgroundDistance(float
distance);
On 2017/05/27 00:47:47, amp wrote:
> I think there is some overlap with the word 'background' now.
> 
> Perhaps we should change the non-text background to be something like
> 'environment'?

Hmm. Perhaps we should change the others to text_background and text_foreground?

It'd be good to discuss some possible names next week.

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

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.cc:369:
floor_grid_->set_center_color(color_scheme().horizon);
On 2017/05/27 00:47:47, amp wrote:
> wait, why are we setting this to horizon?  We just set it to floor_grid on
line
> 366?

Whoops. Fixed.

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2902043005/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:147:
manager_->SetFullscreen(true);
On 2017/05/27 00:47:47, amp wrote:
> Why do we need to check fullscreen?
> 
> Shouldn't we just set incognito and verify the colors between that and normal?
> 
> Although, cjgrant and I were discussing having one big test that went through
> all the transitions.  I think I agree with him when he prefers that feature
> specific tests should be more tightly focused.

I was hoping to show that incognito "wins". That is, if we were both full screen
and incognito, we should be showing the incognito colors.

[commit-bot: I haz the power, 2017-05-27 02:05:41]

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

[commit-bot: I haz the power, 2017-05-27 02:05:43]

Dry run: This issue passed the CQ dry run.

[Ian Vollick, 2017-05-27 03:31:43]

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

[commit-bot: I haz the power, 2017-05-27 03:32:00]

Dry run: CQ is trying da patch.

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

[Ian Vollick, 2017-05-27 03:52:40]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2017-05-27 03:52:41]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2017-05-27 03:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tiborg@chromium.org,
cjgrant@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2902043005/#ps200001
(title: ".")

[commit-bot: I haz the power, 2017-05-27 03:52:52]

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-05-27 04:11:35]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495857161696170,
"parent_rev": "99abef20f559ceb8e825399df63d728c64330110", "commit_rev":
"c619c829f502b17a0a71a9d0e5c68df9baff5825"}

[commit-bot: I haz the power, 2017-05-27 04:11:48]

Description was changed from

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring
when in incognito mode.

BUG=715664
==========

to

==========
[vr] Add incognito coloring

This changes the background, grid, and URL bar coloring
when in incognito mode.

BUG=715664

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

[commit-bot: I haz the power, 2017-05-27 04:11:49]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c619c829f502b17a0a71a9d0e5c6...