Add VR gradient and grid background to non-WebVR scene.

Add VR gradient and grid background to non-WebVR scene.

- Add the floor, ceiling and grid quads.
- Add the content backplane to avoid reticle jumping around the edges.
- Configure the scene reticle distance limit based on content.

BUG=715573
TEST=Verified background on regular page; no background in WebVR.

Review-Url: https://codereview.chromium.org/2844313002
Cr-Commit-Position: refs/heads/master@{#467814}
Committed: https://chromium.googlesource.com/chromium/src/+/20d7dfb2f40463c2921495a48a3f571094978d2b

[cjgrant, 2017-04-27 15:08:36]

cjgrant@chromium.org changed reviewers:
+ amp@chromium.org, mthiesse@chromium.org, tiborg@chromium.org

[cjgrant, 2017-04-27 15:08:37]

Folks, here's the background for 2D mode.

[cjgrant, 2017-04-27 15:11:14]

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:84: // Place an invisible
but hittable plane behind the content quad, to keep the
Thoughts on this wall of initialization.  I thought about classes or helpers to
represent these background quads, I don't think it's worth it.  Each is a
different one-off element, so if anything I'd break out this init to a separate
method, called by the constructor.

[mthiesse, 2017-04-27 15:47:12]

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:46: element->name =
"Permanent security warning";
Hmm. Is there an easy way to make these element names show up in debug builds
only? We should avoid unnecessary strings in release builds.

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:84: // Place an invisible
but hittable plane behind the content quad, to keep the
On 2017/04/27 15:11:14, cjgrant wrote:
> Thoughts on this wall of initialization.  I thought about classes or helpers
to
> represent these background quads, I don't think it's worth it.  Each is a
> different one-off element, so if anything I'd break out this init to a
separate
> method, called by the constructor.

I think member functions for each element creation would be good for
readability, even if little to no code is shared.

[amp, 2017-04-27 17:28:53]

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:23: static constexpr float
kBackplaneSize = 1000.0;
As long as you are changing things here want to merge in my change to add more
constants instead of magic numbers throughout the file?

https://codereview.chromium.org/2832843004/

[cjgrant, 2017-04-27 18:50:20]

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:23: static constexpr float
kBackplaneSize = 1000.0;
On 2017/04/27 17:28:53, amp wrote:
> As long as you are changing things here want to merge in my change to add more
> constants instead of magic numbers throughout the file?
> 
> https://codereview.chromium.org/2832843004/

I would have said yes, but your CL is now in the CQ. :)

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:46: element->name =
"Permanent security warning";
On 2017/04/27 15:47:11, mthiesse wrote:
> Hmm. Is there an easy way to make these element names show up in debug builds
> only? We should avoid unnecessary strings in release builds.

Agreed on no strings.  I don't know of any fanciness other than the usual #if
DEBUG / #if NDEBUG type approach (assuming that works).  Looking back, we added
these names to aid in E2E testing that talked to HTML UI and native.  Maybe we
ought to just turf out the name field now, as it doesn't help with much else.

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:84: // Place an invisible
but hittable plane behind the content quad, to keep the
On 2017/04/27 15:47:11, mthiesse wrote:
> On 2017/04/27 15:11:14, cjgrant wrote:
> > Thoughts on this wall of initialization.  I thought about classes or helpers
> to
> > represent these background quads, I don't think it's worth it.  Each is a
> > different one-off element, so if anything I'd break out this init to a
> separate
> > method, called by the constructor.
> 
> I think member functions for each element creation would be good for
> readability, even if little to no code is shared.

Done.  Let me know what you think...

[amp, 2017-04-27 18:54:27]

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:23: static constexpr float
kBackplaneSize = 1000.0;
On 2017/04/27 18:50:20, cjgrant wrote:
> On 2017/04/27 17:28:53, amp wrote:
> > As long as you are changing things here want to merge in my change to add
more
> > constants instead of magic numbers throughout the file?
> > 
> > https://codereview.chromium.org/2832843004/
> 
> I would have said yes, but your CL is now in the CQ. :)

Indeed, thanks Michael!

It was small enough I didn't care either way if it went in on it's own or with
another change.

[tiborg, 2017-04-27 19:20:45]

On 2017/04/27 18:54:27, amp wrote:
>
https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):
> 
>
https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:23: static constexpr float
> kBackplaneSize = 1000.0;
> On 2017/04/27 18:50:20, cjgrant wrote:
> > On 2017/04/27 17:28:53, amp wrote:
> > > As long as you are changing things here want to merge in my change to add
> more
> > > constants instead of magic numbers throughout the file?
> > > 
> > > https://codereview.chromium.org/2832843004/
> > 
> > I would have said yes, but your CL is now in the CQ. :)
> 
> Indeed, thanks Michael!
> 
> It was small enough I didn't care either way if it went in on it's own or with
> another change.

lgtm

[cjgrant, 2017-04-27 21:28:33]

GTG. PTAL, SVP. TY.

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2844313002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:46: element->name =
"Permanent security warning";
On 2017/04/27 18:50:19, cjgrant wrote:
> On 2017/04/27 15:47:11, mthiesse wrote:
> > Hmm. Is there an easy way to make these element names show up in debug
builds
> > only? We should avoid unnecessary strings in release builds.
> 
> Agreed on no strings.  I don't know of any fanciness other than the usual #if
> DEBUG / #if NDEBUG type approach (assuming that works).  Looking back, we
added
> these names to aid in E2E testing that talked to HTML UI and native.  Maybe we
> ought to just turf out the name field now, as it doesn't help with much else.

Done.

[mthiesse, 2017-04-27 21:36:55]

Looks lgtm to moi

[cjgrant, 2017-04-27 21:37:29]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-04-27 21:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tiborg@chromium.org
Link to the patchset: https://codereview.chromium.org/2844313002/#ps60001
(title: "Rebase onto amp@'s magic number CL.")

[commit-bot: I haz the power, 2017-04-27 21:37:48]

CQ is trying da patch.

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

[amp, 2017-04-27 21:51:23]

lgtm

[commit-bot: I haz the power, 2017-04-28 01:48:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493329049013660,
"parent_rev": "f8ef156d997fe415dfb94b4917a2a6ed648d626b", "commit_rev":
"20d7dfb2f40463c2921495a48a3f571094978d2b"}

[commit-bot: I haz the power, 2017-04-28 01:49:05]

Description was changed from

==========
Add VR gradient and grid background to non-WebVR scene.

- Add the floor, ceiling and grid quads.
- Add the content backplane to avoid reticle jumping around the edges.
- Configure the scene reticle distance limit based on content.

BUG=715573
TEST=Verified background on regular page; no background in WebVR.
==========

to

==========
Add VR gradient and grid background to non-WebVR scene.

- Add the floor, ceiling and grid quads.
- Add the content backplane to avoid reticle jumping around the edges.
- Configure the scene reticle distance limit based on content.

BUG=715573
TEST=Verified background on regular page; no background in WebVR.

Review-Url: https://codereview.chromium.org/2844313002
Cr-Commit-Position: refs/heads/master@{#467814}
Committed:
https://chromium.googlesource.com/chromium/src/+/20d7dfb2f40463c2921495a48a3f...
==========

[commit-bot: I haz the power, 2017-04-28 01:49:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/20d7dfb2f40463c2921495a48a3f...