VR: Add a loading indicator to the scene.

VR: Add a loading indicator to the scene.

- Report loading state transiently, if loading is in progress.  The
  element disappears shortly after loading completes.
- Prevent elements from attempt to draw themselves before
  Initialization.

BUG=715620
Review-Url: https://codereview.chromium.org/2877133002
Cr-Commit-Position: refs/heads/master@{#471602}
Committed: https://chromium.googlesource.com/chromium/src/+/2a7558705385f6bcfa8fa31e1466733cd97725dd

[cjgrant, 2017-05-12 17:19:51]

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

[cjgrant, 2017-05-12 17:19:52]

This works as is, but depending on when Ian's change lands, it should be rebased
onto that to to the same dirty-flag approach.

[mthiesse, 2017-05-12 18:13:23]

lgtm

[amp, 2017-05-12 18:46:44]

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

[amp, 2017-05-12 18:46:44]

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

https://codereview.chromium.org/2877133002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:222: void
UiSceneManager::ConfigureScene() {
Should we have the fullscreen changes go through here as well?

It would require passing in a flag for the mode being configured, but it would
pull all the scene configuration into a single location.

Or maybe it would make sense to have a method for each mode, something like
'ConfigureSceneForWebVR, ConfigureSceneForBrowsing, ConfigureSceneForFullscreen,
...)

WDYT?

[cjgrant, 2017-05-12 18:52:12]

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

https://codereview.chromium.org/2877133002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:222: void
UiSceneManager::ConfigureScene() {
On 2017/05/12 18:46:44, amp wrote:
> Should we have the fullscreen changes go through here as well?
> 
> It would require passing in a flag for the mode being configured, but it would
> pull all the scene configuration into a single location.
> 
> Or maybe it would make sense to have a method for each mode, something like
> 'ConfigureSceneForWebVR, ConfigureSceneForBrowsing,
ConfigureSceneForFullscreen,
> ...)
> 
> WDYT?

We could do that.  It'd look more like the original "catch all" method in the
HTML UI.

[cjgrant, 2017-05-12 21:03:06]

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

https://codereview.chromium.org/2877133002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:222: void
UiSceneManager::ConfigureScene() {
On 2017/05/12 18:52:12, cjgrant wrote:
> On 2017/05/12 18:46:44, amp wrote:
> > Should we have the fullscreen changes go through here as well?
> > 
> > It would require passing in a flag for the mode being configured, but it
would
> > pull all the scene configuration into a single location.
> > 
> > Or maybe it would make sense to have a method for each mode, something like
> > 'ConfigureSceneForWebVR, ConfigureSceneForBrowsing,
> ConfigureSceneForFullscreen,
> > ...)
> > 
> > WDYT?
> 
> We could do that.  It'd look more like the original "catch all" method in the
> HTML UI.

Done.  I refactored as suggested and retested.  PTAL when you can...

[cjgrant, 2017-05-12 21:09:19]

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

[commit-bot: I haz the power, 2017-05-12 21:10: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-12 21:13:46]

lgtm assuming the backplane comment below isn't a problem.

a couple of nit's on comments that can safely be ignored if everything else is
good to go.

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

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:250: // Content quad.
s/quad/elements/

Now that there are more than one elements.

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:255: // Update content quad
parameters.
Change to 'Update content parameters depending on fullscreen'

Although this one may be fine as is.

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
main_content_->set_translation(
do we need to change the backplane as well (now that it is included in the
'content_elements_' and should be enabled from line 252)?

[Ian Vollick, 2017-05-12 21:44:54]

On 2017/05/12 21:13:46, amp wrote:
> lgtm assuming the backplane comment below isn't a problem.
> 
> a couple of nit's on comments that can safely be ignored if everything else is
> good to go.
> 
>
https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):
> 
>
https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:250: // Content quad.
> s/quad/elements/
> 
> Now that there are more than one elements.
> 
>
https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:255: // Update content
quad
> parameters.
> Change to 'Update content parameters depending on fullscreen'
> 
> Although this one may be fine as is.
> 
>
https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
> main_content_->set_translation(
> do we need to change the backplane as well (now that it is included in the
> 'content_elements_' and should be enabled from line 252)?

dirty bit changes lgtm. thanks!

[cjgrant, 2017-05-12 21:45:53]

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

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:250: // Content quad.
On 2017/05/12 21:13:46, amp wrote:
> s/quad/elements/
> 
> Now that there are more than one elements.

Done.

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:255: // Update content quad
parameters.
On 2017/05/12 21:13:46, amp wrote:
> Change to 'Update content parameters depending on fullscreen'
> 
> Although this one may be fine as is.

Done.

https://codereview.chromium.org/2877133002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
main_content_->set_translation(
On 2017/05/12 21:13:46, amp wrote:
> do we need to change the backplane as well (now that it is included in the
> 'content_elements_' and should be enabled from line 252)?

Nope, we're fine - the backplane is a child of the content quad, so it moves
around with the content automatically.

[cjgrant, 2017-05-12 21:46:09]

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

[commit-bot: I haz the power, 2017-05-12 21:47:52]

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-12 21:59:13]

The CQ bit was unchecked by cjgrant@chromium.org

[cjgrant, 2017-05-12 21:59:30]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-05-12 21:59:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2877133002/#ps80001
(title: "Make clearing dirty bit bulletproof; nits.")

[commit-bot: I haz the power, 2017-05-12 21:59:47]

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-13 01:23:19]

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

[commit-bot: I haz the power, 2017-05-13 01:23:19]

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

[cjgrant, 2017-05-13 01:58:18]

The CQ bit was checked by cjgrant@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:58:46]

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-13 02:08:25]

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

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

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...)

[cjgrant, 2017-05-13 18:34:01]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-05-13 18:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, mthiesse@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2877133002/#ps100001
(title: "Rebase onto render text change.")

[commit-bot: I haz the power, 2017-05-13 18:34:13]

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-13 19:30:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494700441913220,
"parent_rev": "290a91a1ba67abcef76dda7e43fd80323ed1c110", "commit_rev":
"2a7558705385f6bcfa8fa31e1466733cd97725dd"}

[commit-bot: I haz the power, 2017-05-13 19:30:36]

Description was changed from

==========
VR: Add a loading indicator to the scene.

- Report loading state transiently, if loading is in progress.  The
  element disappears shortly after loading completes.
- Prevent elements from attempt to draw themselves before
  Initialization.

BUG=715620
==========

to

==========
VR: Add a loading indicator to the scene.

- Report loading state transiently, if loading is in progress.  The
  element disappears shortly after loading completes.
- Prevent elements from attempt to draw themselves before
  Initialization.

BUG=715620

Review-Url: https://codereview.chromium.org/2877133002
Cr-Commit-Position: refs/heads/master@{#471602}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a7558705385f6bcfa8fa31e1466...
==========

[commit-bot: I haz the power, 2017-05-13 19:30:37]

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