Update fullscreen ux, make floor and ceiling visible.

Update fullscreen ux, make floor and ceiling visible.

BUG=643434, 661762
Review-Url: https://codereview.chromium.org/2887773008
Cr-Commit-Position: refs/heads/master@{#474172}
Committed: https://chromium.googlesource.com/chromium/src/+/458c5bc259563201f122ce96d612fe89ff305748

[amp, 2017-05-17 21:47:53]

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

[amp, 2017-05-17 21:47:54]

I think I want to combine the floor and grid color handling into a separate
method, but sending this out so I can start iterating on it.

Also still need final colors to use.

[cjgrant, 2017-05-18 16:07:17]

lgtm with a nit.

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

https://codereview.chromium.org/2887773008/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:57: static constexpr float
kFullscreenWidthDms = 1.138;
We're implying Dms elsewhere (ie. lateral meters at 1 meter viewing distance). 
Seems weird to add them here.

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

https://codereview.chromium.org/2887773008/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:69: UiElement* ceiling_ =
nullptr;
It really is looking like scene manager should own elements now that it's
native.  Thinking about this for future cleanup...

[amp, 2017-05-18 16:14:40]

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

[commit-bot: I haz the power, 2017-05-18 16:15: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-05-18 16:15:34]

I'll hold off on submitting to see if I can get this refactored using enum's
before branch point.  But if not I'll submit as is before next Thur.

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

https://codereview.chromium.org/2887773008/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:57: static constexpr float
kFullscreenWidthDms = 1.138;
On 2017/05/18 16:07:16, cjgrant wrote:
> We're implying Dms elsewhere (ie. lateral meters at 1 meter viewing distance).

> Seems weird to add them here.

The only reason I added them here was to calculate the fullscreen distance (see
the comment on line 60) and because we then need to multiple them by that
distance for the actual height and width later.

What would you prefer to see instead, maybe just hardcoded values that were
pre-calculated?

Should we call the units something else, or should this live elsewhere?

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

https://codereview.chromium.org/2887773008/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:69: UiElement* ceiling_ =
nullptr;
On 2017/05/18 16:07:16, cjgrant wrote:
> It really is looking like scene manager should own elements now that it's
> native.  Thinking about this for future cleanup...

I threw this together to get it working quickly in case branch point came at me
quicker than expected, but I still have it in mind set up enums for each element
(as we discussed for testing) and use those instead of hardcoding in elements in
here.

If I have time I want to finish off the tests with the new enum and then rework
this to use those.

As I'm out tomorrow for our move in KIR (our desktops will be off so I can't
even WFH) time will be a little bit tighter, but maybe still doable before
branch point next Thur.

[commit-bot: I haz the power, 2017-05-18 16:20:40]

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

[commit-bot: I haz the power, 2017-05-18 16:20:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amp, 2017-05-18 16:59:54]

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

[commit-bot: I haz the power, 2017-05-18 17:00:37]

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-05-18 17:46:33]

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

[commit-bot: I haz the power, 2017-05-18 17:46:36]

Dry run: This issue passed the CQ dry run.

[amp, 2017-05-23 18:44:57]

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

[commit-bot: I haz the power, 2017-05-23 18:45:29]

Dry run: CQ is trying da patch.

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

[amp, 2017-05-23 18:47:13]

I updated this slightly to check the new tests work with the added elements
(they do).

It might make more sense to just commit everything here rather than have a
separate change for the tests only.

WDYT?

[commit-bot: I haz the power, 2017-05-23 19:50:24]

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

[commit-bot: I haz the power, 2017-05-23 19:50:25]

Dry run: This issue passed the CQ dry run.

[amp, 2017-05-23 20:55:10]

Description was changed from

==========
Update fullscreen ux, make floor and ceiling visible.

BUG=643434
==========

to

==========
Update fullscreen ux, make floor and ceiling visible.

BUG=643434,661762
==========

[amp, 2017-05-23 20:56:23]

PTAL.  Addressed comments on tests from
https://codereview.chromium.org/2873043002/

[amp, 2017-05-23 21:10:06]

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

[commit-bot: I haz the power, 2017-05-23 21:11:09]

Dry run: CQ is trying da patch.

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

[amp, 2017-05-23 21:14:14]

The CQ bit was unchecked by amp@chromium.org

[amp, 2017-05-23 21:15:43]

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

[commit-bot: I haz the power, 2017-05-23 21:16:48]

Dry run: CQ is trying da patch.

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

[cjgrant, 2017-05-23 21:36:02]

On 2017/05/23 20:56:23, amp wrote:
> PTAL.  Addressed comments on tests from
> https://codereview.chromium.org/2873043002/

lgtm again, with the conclusion (from offline discussion) that:

The before and after cases should stay the same, but the "in fullscreen" middle
test case would be doing "if anything that isn't in a small set of allowed
elements is visible, fail".  This way, we'll catch any new elements that
accidentally appear visible in fullscreen, when they should not.

[amp, 2017-05-23 21:53:41]

On 2017/05/23 21:36:02, cjgrant wrote:
> On 2017/05/23 20:56:23, amp wrote:
> > PTAL.  Addressed comments on tests from
> > https://codereview.chromium.org/2873043002/
> 
> lgtm again, with the conclusion (from offline discussion) that:
> 
> The before and after cases should stay the same, but the "in fullscreen"
middle
> test case would be doing "if anything that isn't in a small set of allowed
> elements is visible, fail".  This way, we'll catch any new elements that
> accidentally appear visible in fullscreen, when they should not.

Done!

I'll hold of on submitting a bit longer to see if Josh has any changes for the
translation and sizes, but other than different numbers everything else should
stay as is.

[amp, 2017-05-23 21:55:40]

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

[commit-bot: I haz the power, 2017-05-23 21:56:16]

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-05-23 23:33:45]

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

[commit-bot: I haz the power, 2017-05-23 23:33:46]

Dry run: This issue passed the CQ dry run.

[amp, 2017-05-23 23:34:48]

The CQ bit was checked by amp@chromium.org

[amp, 2017-05-23 23:34:48]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2887773008/#ps100001
(title: "adjust tests to fail if a new added element becomes visible at some
future point")

[commit-bot: I haz the power, 2017-05-23 23:38:33]

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-23 23:50:45]

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

[commit-bot: I haz the power, 2017-05-23 23:50:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[amp, 2017-05-23 23:58:02]

amp@chromium.org changed reviewers:
+ mthiesse@chromium.org

[amp, 2017-05-23 23:58:03]

+mthiesse, need java owner approval

[mthiesse, 2017-05-24 05:25:43]

lgtm. Feel free to TBR me if you need to make further changes to constants like
this.

[mthiesse, 2017-05-24 05:25:48]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-24 05:26:26]

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-24 05:34:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495603548369580,
"parent_rev": "b2b09428c58030ecf7b1a92a547115fa5bd361b6", "commit_rev":
"458c5bc259563201f122ce96d612fe89ff305748"}

[commit-bot: I haz the power, 2017-05-24 05:35:00]

Description was changed from

==========
Update fullscreen ux, make floor and ceiling visible.

BUG=643434,661762
==========

to

==========
Update fullscreen ux, make floor and ceiling visible.

BUG=643434,661762

Review-Url: https://codereview.chromium.org/2887773008
Cr-Commit-Position: refs/heads/master@{#474172}
Committed:
https://chromium.googlesource.com/chromium/src/+/458c5bc259563201f122ce96d612...
==========

[commit-bot: I haz the power, 2017-05-24 05:35:01]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/458c5bc259563201f122ce96d612...