Implement lazy initialization for VrShellDelegate

Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and webview?) so that they now correctly return no displays available.

BUG=655297, 689293
Review-Url: https://codereview.chromium.org/2727873002
Cr-Commit-Position: refs/heads/master@{#454474}
Committed: https://chromium.googlesource.com/chromium/src/+/182cf17def973f5d7a981025afb65ebd6f022ca9

[mthiesse, 2017-03-02 02:05:07]

mthiesse@chromium.org changed reviewers:
+ amp@chromium.org, bajones@chromium.org, bshe@chromium.org

[mthiesse, 2017-03-02 02:05:08]

PTAL. Haven't tested this very thoroughly yet, will do so tomorrow, but I'd like
to get this out for initial review.

[amp, 2017-03-02 03:34:57]

On 2017/03/02 02:05:08, mthiesse wrote:
> PTAL. Haven't tested this very thoroughly yet, will do so tomorrow, but I'd
like
> to get this out for initial review.

I haven't gone over it closely yet, but it looks like it will solve everything I
need for the install prompt (only when vr is initialized).  I do have a few
questions, but they are more relevant to my review so I'll rebase on top of this
patch and add the questions to my change.

[bajones, 2017-03-02 05:06:53]

Non-Java bits LGTM

[bshe, 2017-03-02 15:20:58]

On 2017/03/02 05:06:53, bajones wrote:
> Non-Java bits LGTM

vr_shell lgtm

[mthiesse, 2017-03-02 15:57:01]

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

[mthiesse, 2017-03-02 15:57:02]

tedchoc@chromium.org: Please review changes outside of vr directories.

[mthiesse, 2017-03-02 17:13:22]

bshe, amp, I made a bunch of changes to support registering for the daydream
intent without a VrShellDelegate. PTAL.

[mthiesse, 2017-03-02 18:05:42]

Description was changed from

==========
Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory
overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer
needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to
arbitrary ChromeActivities in the future (though for now only CTA is supported)

BUG=655297
==========

to

==========
Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory
overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer
needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to
arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and
webview?) so that they now correctly return no displays available.

BUG=655297, 689293
==========

[amp, 2017-03-02 18:11:54]

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

https://codereview.chromium.org/2727873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:323:
if (mVrSupportLevel == VR_NOT_AVAILABLE) return;
If VR is not available, we still contruct a VrShellDelegate and return it.

Do we need to have an opportunity to create a new instance if the Vr support
level changes (after they updated libraries for instance), or should that
require a restart?

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

https://codereview.chromium.org/2727873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:37: return
GetNativeVrShellDelegate(env, jdelegate.obj());
It looks it's possible for jdelegate to be non null, but it hasn't initialized
the native vr shell delegate yet (if vr was not available it will construct a
basic java vr shell delegate, but return before initializing native).

[mthiesse, 2017-03-02 20:01:58]

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

https://codereview.chromium.org/2727873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:323:
if (mVrSupportLevel == VR_NOT_AVAILABLE) return;
On 2017/03/02 18:11:54, amp wrote:
> If VR is not available, we still contruct a VrShellDelegate and return it.
> 
> Do we need to have an opportunity to create a new instance if the Vr support
> level changes (after they updated libraries for instance), or should that
> require a restart?

Oops, meant to remove the early return. The native side should always exist if
we have a VrShellDelegate at all.

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

https://codereview.chromium.org/2727873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:37: return
GetNativeVrShellDelegate(env, jdelegate.obj());
On 2017/03/02 18:11:54, amp wrote:
> It looks it's possible for jdelegate to be non null, but it hasn't initialized
> the native vr shell delegate yet (if vr was not available it will construct a
> basic java vr shell delegate, but return before initializing native).

Fixed. jdelegate is never null now.

[amp, 2017-03-02 20:39:49]

lgtm

[Ted C, 2017-03-02 21:29:31]

lgtm w/ comment comments

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:2034:
public void onEnterVR() {}
we try to javadoc all public methods.

some general lifecycle comments and what the activities should consider doing as
a result of these calls would be good.

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:267:
if (VrShellDelegate.isInVR()) return false;
These are also things that would need to move if ChromeActivity needs to support
VR (probably worth the same TODO as below).  If the goal is to support VR
outside of Tabbed mode, then it would be good to document all the places in this
file that would need to move (would intent handling need to?)

[mthiesse, 2017-03-02 22:11:47]

FYI, also fixed a test file I missed as part of the cleanup.

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:2034:
public void onEnterVR() {}
On 2017/03/02 21:29:31, Ted C wrote:
> we try to javadoc all public methods.
> 
> some general lifecycle comments and what the activities should consider doing
as
> a result of these calls would be good.

Done.

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2727873002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:267:
if (VrShellDelegate.isInVR()) return false;
On 2017/03/02 21:29:31, Ted C wrote:
> These are also things that would need to move if ChromeActivity needs to
support
> VR (probably worth the same TODO as below).  If the goal is to support VR
> outside of Tabbed mode, then it would be good to document all the places in
this
> file that would need to move (would intent handling need to?)

Done. (Yes, intent handling would also need to move)

[mthiesse, 2017-03-02 22:15:29]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-03-02 22:15:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, bshe@chromium.org, tedchoc@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2727873002/#ps60001
(title: "Add comments + Fix test file")

[commit-bot: I haz the power, 2017-03-02 22:16:02]

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-03-02 22:19:14]

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

[commit-bot: I haz the power, 2017-03-02 22:19:15]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bsheedy, 2017-03-02 22:25:11]

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

[bsheedy, 2017-03-02 22:25:12]

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:47:
public static void forceEnterVr(final VrShellDelegate vrDelegate) {
Nit: This input parameter is no longer used now that enterVRIfNecessary() is
static.

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:119:
public static void waitForVrSupported(final VrShellDelegate delegate) {
Nit: Also no longer used.

[mthiesse, 2017-03-02 22:35:53]

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
(right):

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:47:
public static void forceEnterVr(final VrShellDelegate vrDelegate) {
On 2017/03/02 22:25:12, bsheedy wrote:
> Nit: This input parameter is no longer used now that enterVRIfNecessary() is
> static.

Done.

https://codereview.chromium.org/2727873002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java:119:
public static void waitForVrSupported(final VrShellDelegate delegate) {
On 2017/03/02 22:25:12, bsheedy wrote:
> Nit: Also no longer used.

Done.

[mthiesse, 2017-03-02 22:35:58]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-03-02 22:35:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2727873002/#ps80001
(title: "rebase + comments")

[commit-bot: I haz the power, 2017-03-02 22:36:35]

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-03-02 23:43:14]

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

[commit-bot: I haz the power, 2017-03-02 23:43:15]

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

[mthiesse, 2017-03-03 00:30:17]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-03-03 00:30:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2727873002/#ps100001
(title: "Fix compile")

[commit-bot: I haz the power, 2017-03-03 00:30:47]

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

[mthiesse, 2017-03-03 01:43:42]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-03-03 01:43:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, bajones@chromium.org, bshe@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2727873002/#ps120001
(title: "Fix FindBugs errors - neat!")

[commit-bot: I haz the power, 2017-03-03 01:44:12]

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-03-03 02:26:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488505422069060,
"parent_rev": "521d60812176375b2a0f196c7ccd31f38d524efa", "commit_rev":
"2e1e2ee92568e1470d00b8f094ae1c30cc7ec090"}

[commit-bot: I haz the power, 2017-03-03 02:31:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488505422069060,
"parent_rev": "7613176285d46fbc5b4712e42bd135aae99cbba5", "commit_rev":
"182cf17def973f5d7a981025afb65ebd6f022ca9"}

[commit-bot: I haz the power, 2017-03-03 02:39:38]

Description was changed from

==========
Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory
overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer
needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to
arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and
webview?) so that they now correctly return no displays available.

BUG=655297, 689293
==========

to

==========
Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory
overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer
needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to
arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and
webview?) so that they now correctly return no displays available.

BUG=655297, 689293

Review-Url: https://codereview.chromium.org/2727873002
Cr-Commit-Position: refs/heads/master@{#454474}
Committed:
https://chromium.googlesource.com/chromium/src/+/182cf17def973f5d7a981025afb6...
==========

[commit-bot: I haz the power, 2017-03-03 02:39:39]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/182cf17def973f5d7a981025afb6...