Switch WebVR to handle GvrApi management through VrShellDelegate

chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java

Index: chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
index 826c000bcee49356b07d5d0d5b28390894035a8a..183a282df4927e0cf963823370716f34bb984044 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
@@ -15,6 +15,8 @@ import android.view.WindowManager;
 import android.widget.FrameLayout;
 
 import org.chromium.base.Log;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
 import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.tab.Tab;
 
@@ -24,6 +26,7 @@ import java.lang.reflect.InvocationTargetException;
 /**
  * Manages interactions with the VR Shell.
  */
+@JNINamespace("vr_shell")
 public class VrShellDelegate {
     private static final String TAG = "VrShellDelegate";
 
@@ -36,6 +39,7 @@ public class VrShellDelegate {
     private boolean mInVr;
     private int mRestoreSystemUiVisibilityFlag = -1;
     private String mVrExtra;
+    private long mNativeVrShellDelegate;
 
     public VrShellDelegate(ChromeTabbedActivity activity) {
         mActivity = activity;
@@ -52,6 +56,14 @@ public class VrShellDelegate {
         }
     }
 
+    /**
+     * Should be called once the native library is loaded so that the native portion of this
+     * class can be initialized.
+     */
+    public void onNativeLibraryReady() {
+        mNativeVrShellDelegate = nativeInit();

[Ted C, 2016/09/21 17:59:24]

do you need a check for mVrShellEnabled here?  otherwise, I believe you'll be
attempting to initialize native code that won't exist (when the build flag is
off).

+    }
+
     @SuppressWarnings("unchecked")
     private Class<? extends VrShellInterface> maybeFindVrShell() {
         try {
@@ -69,8 +81,9 @@ public class VrShellDelegate {
      * libraries are ready.
      * @Returns Whether or not we are in VR when this function returns.

[Ted C, 2016/09/21 17:59:24]

We should add an @param for inWebVR here

Also if we are here, can you change this to @return instead of @Returns

      */
-    public boolean enterVRIfNecessary() {
-        if (!mVrShellEnabled) return false;
+    @CalledByNative
+    public boolean enterVRIfNecessary(boolean inWebVR) {
+        if (!mVrShellEnabled || mNativeVrShellDelegate == 0) return false;
         Tab tab = mActivity.getActivityTab();
         // TODO(mthiesse): When we have VR UI for opening new tabs, etc., allow VR Shell to be
         // entered without any current tabs.
@@ -86,12 +99,26 @@ public class VrShellDelegate {
         }
         addVrViews();
         setupVrModeWindowFlags();
-        mVrShell.onNativeLibraryReady(tab);
+        mVrShell.onNativeLibraryReady(tab, this);

[Ted C, 2016/09/21 17:59:25]

to me, it's weird to have this not in the onNativeLibraryReady call above.  Do
these mean different things now?  Should this method be called initializeForTab
or "something"?

[bajones, 2016/09/21 19:16:25]

While "something" seems like a great function name, after consulting with the
other VrShell devlopers I went with "initializeNative" instead. ;)

+        if (inWebVR) {

[Ted C, 2016/09/21 17:59:25]

in java land (if it is readable), you can put the condition and statement on a
single line if it all fits

+            mVrShell.setWebVrModeEnabled(true);
+        }
         mVrShell.setVrModeEnabled(true);
         mInVr = true;
         return true;
     }
 
+    @CalledByNative
+    public boolean exitWebVR() {

[Ted C, 2016/09/21 17:59:25]

does this only get accessed by native?  If so, you can make it private.

+        if (!mInVr) return false;
+        mVrShell.setWebVrModeEnabled(false);

[Ted C, 2016/09/21 17:59:25]

Could we just move this line into shutdownVR (is it bad to call in non-webVR
cases)?  Then we could expose that via @CalledByNative and we could get rid of
the need for this method?

Ahh...based on the comment below it looks like exiting WebVR won't be calling
shutdown long term, so maybe my question above isn't right.

[bajones, 2016/09/21 19:16:25]

I think the comment below illustrates the reason for this. In future CLs we will
want to exit from WebVR back to the VrShell rather than exiting VR entirely. As
such, we want to disambiguate whether we're only exiting the WebVR content or VR
as a whole.

+        // TODO(bajones): Once VR Shell can be invoked outside of WebVR this
+        // should no longer exit the shell outright. Need a way to determine
+        // how VrShell was created.
+        shutdownVR();
+        return true;
+    }
+
     /**
      * Resumes VR Shell.
      */
@@ -108,11 +135,26 @@ public class VrShellDelegate {
     }
 
     /**
-     * Exits VR Shell, performing all necessary cleanup.
+     * Exits the current VR mode (WebVR or VRShell)
      * @Returns Whether or not we exited VR.

[Ted C, 2016/09/21 17:59:25]

@return

      */
     public boolean exitVRIfNecessary() {
         if (!mInVr) return false;
+        // If WebVR is presenting instruct it to exit. VR mode should not
+        // exit in this scenario, in case we want to return to the VrShell.
+        if (!nativeExitWebVRIfNecessary(mNativeVrShellDelegate)) {
+            // If WebVR was not presenting, shutdown VR mode entirely.
+            shutdownVR();
+        }
+
+        return true;
+    }
+
+    /**
+     * Exits VR Shell, performing all necessary cleanup.
+     */
+    private void shutdownVR() {
+        if (!mInVr) return;
         mActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED);
         mVrShell.setVrModeEnabled(false);
         mVrShell.pause();
@@ -120,7 +162,6 @@ public class VrShellDelegate {
         clearVrModeWindowFlags();
         destroyVrShell();
         mInVr = false;
-        return true;
     }
 
     private boolean createVrShell() {
@@ -205,4 +246,15 @@ public class VrShellDelegate {
     public boolean isVrShellEnabled() {
         return mVrShellEnabled;
     }
+
+    /**
+     * @return Pointer to the native VrShellDelegate object.
+     */
+    @CalledByNative
+    public long getNativePointer() {

[Ted C, 2016/09/21 17:59:24]

should be private

+        return mNativeVrShellDelegate;
+    }
+
+    private native long nativeInit();
+    private native boolean nativeExitWebVRIfNecessary(long nativeVrShellDelegate);
 }