Show splash screen when entering VR from a deep-link

Show splash screen when entering VR from a deep-link

This CL adds the necessary splash screen ui to show when we are going to auto-present
WebVR content. The flow of things is that when we get a new auto-present intent, we enter
VR (not in web vr mode) and create VrShell with "web_vr_autopresentation_expected"
parameter. This tells UiSceneManager to show the splash screen. When the WebVR page
calles requestPresent, we enter WebVR mode and the ui scene manager hides the splash
screen.

Notes
- Currently, the UI not show up if Chrome is started for the first time. This has to do with the
  fact that we can't enter VR early in that case. bshe@ is working on that.
- The loading spinner on the splash screen is not added in this patch
- This patch expects a sane WebVR page in that it doesn't handle error cases such as
requestPresent never being called. 

BUG=699815
Review-Url: https://codereview.chromium.org/2955483003
Cr-Commit-Position: refs/heads/master@{#482295}
Committed: https://chromium.googlesource.com/chromium/src/+/2efa616e207873fdbbf1ae8f144d16708b4a0d62

[ymalik, 2017-06-23 09:08:06]

Description was changed from

==========
Show splash screen when entering VR from a deep-link

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

to

==========
Show splash screen when entering VR from a deep-link

This CL adds the necessary splash screen ui to show when we are going to
auto-present
WebVR content. The flow of things is that when we get a new auto-present intent,
we enter
VR (not in web vr mode) and create VrShell with
"web_vr_autopresentation_expected"
parameter. This tells UiSceneManager to show the splash screen. When the WebVR
page
calles requestPresent, we enter WebVR mode and the ui scene manager hides the
splash
screen.

Notes
- Currently, the UI not show up if Chrome is started for the first time. This
has to do with the
  fact that we can't enter VR early in that case. bshe@ is working on that.
- The loading spinner on the splash screen is not added in this patch
- This patch expects a sane WebVR page in that it doesn't handle error cases
such as
requestPresent never being called. 

BUG=699815
==========

[ymalik, 2017-06-23 09:08:07]

ymalik@chromium.org changed reviewers:
+ bshe@chromium.org, cjgrant@chromium.org

[ymalik, 2017-06-23 09:10:20]

hey folks, PTAL :)

@bshe, I'm currently only handling the case where chrome is already running in
the background. The VrShellDelegate changes in this CL ma be overwritten by your
work to make it work in the general case.

[cjgrant, 2017-06-23 19:26:33]

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:685:
//&& ChromeFeatureList.isEnabled(ChromeFeatureList.WEBVR_AUTOPRESENT)
This needs cleanup.

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:335:
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
Is this supposed to be floating out here?

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:43: virtual void
SetSplashScreenIcon(SkBitmap bitmap) = 0;
const reference here, shouldn't it be?

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:151:
scene_->set_showing_splash_screen(web_vr_autopresentation_expected_);
Why is this not in ConfigureScene()?

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:461: // Exit warning.
New-line before // Exit warning, as others below?

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:463:
screen_dimmer_->SetEnabled(scene_->is_exiting() && browsing_mode);
Why are these changing to not show in WebVR?

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:532:
splash_screen_icon_->SetSplashScreenIcon(bitmap);
SetSplashScreenBitmap() for clarity?  As is you're setting an icon on an icon.

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:40:
std::set<UiElementDebugId> kElementsVisibleForSplashScreen = {
If used in only one test, should be in the test, IMO.

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:229: weak_scene_manager_,
bitmap));
My binding could be rusty, but I think you want to pass the bitmap as const& to
this point, then make a copy if needed.  Otherwise, aren't you also copying the
icon when you call this function?

[cjgrant, 2017-06-23 20:53:37]

lgtm in general, assuming the nits and questions are addressed.

Yash, lets chat if any new issues crop up.

[mthiesse, 2017-06-23 23:11:24]

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

[mthiesse, 2017-06-23 23:11:25]

lgtm % chris' comments.

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:335:
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2017/06/23 19:26:32, cjgrant wrote:
> Is this supposed to be floating out here?

yes.

[ymalik, 2017-06-25 20:27:14]

@Chris, addressed your comments. Regarding your main question about copies of
bitmap, we're only doing one now when storing the texture in
SplashScreenIconTexture. I'll land this but please feel free to post comments
and I'll address in a follow-up CL if needed.

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:685:
//&& ChromeFeatureList.isEnabled(ChromeFeatureList.WEBVR_AUTOPRESENT)
On 2017/06/23 19:26:32, cjgrant wrote:
> This needs cleanup.

Done.

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2955483003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:335:
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2017/06/23 23:11:25, mthiesse wrote:
> On 2017/06/23 19:26:32, cjgrant wrote:
> > Is this supposed to be floating out here?
> 
> yes.

Yeah the formatter is weird. Others are also like this.

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:43: virtual void
SetSplashScreenIcon(SkBitmap bitmap) = 0;
On 2017/06/23 19:26:32, cjgrant wrote:
> const reference here, shouldn't it be?

Absolutely.

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:151:
scene_->set_showing_splash_screen(web_vr_autopresentation_expected_);
On 2017/06/23 19:26:32, cjgrant wrote:
> Why is this not in ConfigureScene()?

Done.

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:461: // Exit warning.
On 2017/06/23 19:26:32, cjgrant wrote:
> New-line before // Exit warning, as others below?

Done.

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:463:
screen_dimmer_->SetEnabled(scene_->is_exiting() && browsing_mode);
On 2017/06/23 19:26:33, cjgrant wrote:
> Why are these changing to not show in WebVR?

You're right. this shouldn't be the case. We can be exiting when in WebVR
because we show the transient url bar now.

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:532:
splash_screen_icon_->SetSplashScreenIcon(bitmap);
On 2017/06/23 19:26:32, cjgrant wrote:
> SetSplashScreenBitmap() for clarity?  As is you're setting an icon on an icon.

The "icon" here refers to the actual logo. SplashScreenBitmap is a bit ambiguous
since the loading indicator (to come) can also have a bitmap. SetBitmap or
SetSplashScreenIconBitmap seems fine. Changed to latter.

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

https://codereview.chromium.org/2955483003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:40:
std::set<UiElementDebugId> kElementsVisibleForSplashScreen = {
On 2017/06/23 19:26:33, cjgrant wrote:
> If used in only one test, should be in the test, IMO.

Done.

[ymalik, 2017-06-25 20:27:33]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-25 20:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2955483003/#ps40001
(title: "address comments")

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

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-25 20:29:48]

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

[commit-bot: I haz the power, 2017-06-25 20:29:49]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ymalik, 2017-06-26 04:11:27]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-26 04:11:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2955483003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-26 04:11:36]

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-26 04:54:46]

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

[commit-bot: I haz the power, 2017-06-26 04:54:47]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[ymalik, 2017-06-26 15:26:27]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-26 15:26:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2955483003/#ps80001
(title: "fix compile error on bots")

[commit-bot: I haz the power, 2017-06-26 15:26:50]

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-26 16:40:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498490787574660,
"parent_rev": "2f5d2f7b0721ace2fcbe408d8374f1c8277c71de", "commit_rev":
"2efa616e207873fdbbf1ae8f144d16708b4a0d62"}

[commit-bot: I haz the power, 2017-06-26 16:40:25]

Description was changed from

==========
Show splash screen when entering VR from a deep-link

This CL adds the necessary splash screen ui to show when we are going to
auto-present
WebVR content. The flow of things is that when we get a new auto-present intent,
we enter
VR (not in web vr mode) and create VrShell with
"web_vr_autopresentation_expected"
parameter. This tells UiSceneManager to show the splash screen. When the WebVR
page
calles requestPresent, we enter WebVR mode and the ui scene manager hides the
splash
screen.

Notes
- Currently, the UI not show up if Chrome is started for the first time. This
has to do with the
  fact that we can't enter VR early in that case. bshe@ is working on that.
- The loading spinner on the splash screen is not added in this patch
- This patch expects a sane WebVR page in that it doesn't handle error cases
such as
requestPresent never being called. 

BUG=699815
==========

to

==========
Show splash screen when entering VR from a deep-link

This CL adds the necessary splash screen ui to show when we are going to
auto-present
WebVR content. The flow of things is that when we get a new auto-present intent,
we enter
VR (not in web vr mode) and create VrShell with
"web_vr_autopresentation_expected"
parameter. This tells UiSceneManager to show the splash screen. When the WebVR
page
calles requestPresent, we enter WebVR mode and the ui scene manager hides the
splash
screen.

Notes
- Currently, the UI not show up if Chrome is started for the first time. This
has to do with the
  fact that we can't enter VR early in that case. bshe@ is working on that.
- The loading spinner on the splash screen is not added in this patch
- This patch expects a sane WebVR page in that it doesn't handle error cases
such as
requestPresent never being called. 

BUG=699815

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

[commit-bot: I haz the power, 2017-06-26 16:40:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2efa616e207873fdbbf1ae8f144d...