Support rendering Android Native Pages in VR Shell.

chrome/browser/android/vr_shell/vr_shell.cc

Index: chrome/browser/android/vr_shell/vr_shell.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell.cc b/chrome/browser/android/vr_shell/vr_shell.cc
index d77ea0bc040bed3e4a4c0d53920e6f198d74578f..3e140082d7af43e9ee429bae9adbacbea7f7c421 100644
--- a/chrome/browser/android/vr_shell/vr_shell.cc
+++ b/chrome/browser/android/vr_shell/vr_shell.cc
@@ -55,7 +55,7 @@ vr_shell::VrShell* g_instance;
 static const char kVrShellUIURL[] = "chrome://vr-shell-ui";
 
 void SetIsInVR(content::WebContents* contents, bool is_in_vr) {
-  if (contents->GetRenderWidgetHostView())
+  if (contents && contents->GetRenderWidgetHostView())
     contents->GetRenderWidgetHostView()->SetIsInVR(is_in_vr);
 }
 
@@ -63,7 +63,6 @@ void SetIsInVR(content::WebContents* contents, bool is_in_vr) {
 
 VrShell::VrShell(JNIEnv* env,
                  jobject obj,
-                 content::WebContents* main_contents,
                  ui::WindowAndroid* content_window,
                  content::WebContents* ui_contents,
                  ui::WindowAndroid* ui_window,
@@ -72,13 +71,12 @@ VrShell::VrShell(JNIEnv* env,
                  gvr_context* gvr_api,
                  bool reprojected_rendering)
     : WebContentsObserver(ui_contents),
-      main_contents_(main_contents),
+      content_window_(content_window),
       content_compositor_(
-          base::MakeUnique<VrCompositor>(content_window, false)),
+          base::MakeUnique<VrCompositor>(content_window_, false)),
       ui_contents_(ui_contents),
       ui_compositor_(base::MakeUnique<VrCompositor>(ui_window, true)),
       delegate_provider_(delegate),
-      metrics_helper_(base::MakeUnique<VrMetricsHelper>(main_contents_)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       reprojected_rendering_(reprojected_rendering),
       gvr_api_(gvr_api),
@@ -87,10 +85,7 @@ VrShell::VrShell(JNIEnv* env,
   g_instance = this;
   j_vr_shell_.Reset(env, obj);
 
-  content_input_manager_ = base::MakeUnique<VrInputManager>(main_contents_);
   ui_input_manager_ = base::MakeUnique<VrInputManager>(ui_contents_);
-
-  content_compositor_->SetLayer(main_contents_);
   ui_compositor_->SetLayer(ui_contents_);
 
   gl_thread_ = base::MakeUnique<VrGLThread>(
@@ -101,15 +96,8 @@ VrShell::VrShell(JNIEnv* env,
   options.priority = base::ThreadPriority::DISPLAY;
   gl_thread_->StartWithOptions(options);
 
-  if (for_web_vr)
-    metrics_helper_->SetWebVREnabled(true);
   html_interface_ = base::MakeUnique<UiInterface>(
-      for_web_vr ? UiInterface::Mode::WEB_VR : UiInterface::Mode::STANDARD,
-      main_contents_->IsFullscreen());
-  vr_web_contents_observer_ = base::MakeUnique<VrWebContentsObserver>(
-      main_contents_, html_interface_.get(), this);
-
-  SetIsInVR(main_contents_, true);
+      for_web_vr ? UiInterface::Mode::WEB_VR : UiInterface::Mode::STANDARD);
 }
 
 void VrShell::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
@@ -118,16 +106,28 @@ void VrShell::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
 
 void VrShell::SwapContents(JNIEnv* env, const JavaParamRef<jobject>& obj,
                            const JavaParamRef<jobject>& web_contents) {
-  SetIsInVR(main_contents_, false);
   content::WebContents* contents =
       content::WebContents::FromJavaWebContents(web_contents);
+  if (contents == main_contents_)
+    return;
+
+  SetIsInVR(main_contents_, false);
+
   main_contents_ = contents;
-  content_input_manager_ = base::MakeUnique<VrInputManager>(main_contents_);
   content_compositor_->SetLayer(main_contents_);
-  vr_web_contents_observer_ = base::MakeUnique<VrWebContentsObserver>(
-      main_contents_, html_interface_.get(), this);
   SetIsInVR(main_contents_, true);

[bshe, 2017/02/10 20:15:44]

if (main_contents_) before this line? 

[mthiesse, 2017/02/13 15:27:35]

No, we just do the check inside SetIsInVR

+  ContentFrameWasResized(false /* unused */);
+  SetUiState();
 
+  if (!main_contents_) {
+    content_input_manager_ = nullptr;
+    vr_web_contents_observer_ = nullptr;
+    metrics_helper_ = nullptr;
+    return;
+  }
+  content_input_manager_ = base::MakeUnique<VrInputManager>(main_contents_);
+  vr_web_contents_observer_ = base::MakeUnique<VrWebContentsObserver>(
+      main_contents_, html_interface_.get(), this);
   // TODO(billorr): Make VrMetricsHelper tab-aware and able to track multiple
   // tabs. crbug.com/684661
   metrics_helper_ = base::MakeUnique<VrMetricsHelper>(main_contents_);
@@ -135,6 +135,19 @@ void VrShell::SwapContents(JNIEnv* env, const JavaParamRef<jobject>& obj,
   metrics_helper_->SetWebVREnabled(webvr_mode_);
 }
 
+void VrShell::SetUiState() {
+  if (!main_contents_) {
+    // TODO(mthiesse): Properly handle native page URLs.

[bshe, 2017/02/10 20:15:44]

nit: file a bug or reference to existing bug?

[mthiesse, 2017/02/13 15:27:35]

Done.

+    html_interface_->SetURL(GURL());
+    html_interface_->SetLoading(false);
+    html_interface_->SetFullscreen(false);
+  } else {
+    html_interface_->SetURL(main_contents_->GetVisibleURL());
+    html_interface_->SetLoading(main_contents_->IsLoading());
+    html_interface_->SetFullscreen(main_contents_->IsFullscreen());
+  }
+}
+
 void VrShell::LoadUIContent(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   GURL url(kVrShellUIURL);
   ui_contents_->GetController().LoadURL(
@@ -202,7 +215,8 @@ void VrShell::OnPause(JNIEnv* env, const JavaParamRef<jobject>& obj) {
       FROM_HERE, base::Bind(&VrShellGl::OnPause, gl_thread_->GetVrShellGl()));
 
   // exit vr session
-  metrics_helper_->SetVRActive(false);
+  if (metrics_helper_)

[bshe, 2017/02/10 20:15:44]

does this mean we may miss recording the metrics?

[mthiesse, 2017/02/13 15:27:36]

Yes, when on a native page.

+    metrics_helper_->SetVRActive(false);
   SetIsInVR(main_contents_, false);
 }
 
@@ -210,7 +224,8 @@ void VrShell::OnResume(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   gl_thread_->task_runner()->PostTask(
       FROM_HERE, base::Bind(&VrShellGl::OnResume, gl_thread_->GetVrShellGl()));
 
-  metrics_helper_->SetVRActive(true);
+  if (metrics_helper_)
+    metrics_helper_->SetVRActive(true);
   SetIsInVR(main_contents_, true);
 }
 
@@ -235,8 +250,7 @@ base::WeakPtr<VrShell> VrShell::GetWeakPtr(
 }
 
 void VrShell::OnDomContentsLoaded() {
-  html_interface_->SetURL(main_contents_->GetVisibleURL());
-  html_interface_->SetLoading(main_contents_->IsLoading());
+  SetUiState();
   html_interface_->OnDomContentsLoaded();
 }
 
@@ -244,7 +258,8 @@ void VrShell::SetWebVrMode(JNIEnv* env,
                            const base::android::JavaParamRef<jobject>& obj,
                            bool enabled) {
   webvr_mode_ = enabled;
-  metrics_helper_->SetWebVREnabled(enabled);
+  if (metrics_helper_)
+    metrics_helper_->SetWebVREnabled(enabled);
   PostToGlThreadWhenReady(base::Bind(&VrShellGl::SetWebVrMode,
                                      gl_thread_->GetVrShellGl(), enabled));
   html_interface_->SetMode(
@@ -324,9 +339,30 @@ void VrShell::CreateVRDisplayInfo(
                                      callback, device_id));
 }
 
-void VrShell::SurfacesChanged(jobject content_surface, jobject ui_surface) {
-  content_compositor_->SurfaceChanged(content_surface);
-  ui_compositor_->SurfaceChanged(ui_surface);
+base::android::ScopedJavaGlobalRef<jobject> VrShell::TakeContentSurface(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj) {
+  content_compositor_->SurfaceChanged(nullptr);
+  base::android::ScopedJavaGlobalRef<jobject> surface(env, content_surface_);
+  content_surface_ = nullptr;
+  return surface;
+}
+
+void VrShell::RestoreContentSurface(JNIEnv* env,
+                                    const JavaParamRef<jobject>& obj) {
+  PostToGlThreadWhenReady(
+      base::Bind(&VrShellGl::CreateContentSurface, gl_thread_->GetVrShellGl()));
+}
+
+void VrShell::UiSurfaceChanged(jobject surface) {
+  ui_compositor_->SurfaceChanged(surface);
+}
+
+void VrShell::ContentSurfaceChanged(jobject surface) {
+  content_surface_ = surface;
+  content_compositor_->SurfaceChanged(surface);
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_VrShellImpl_contentSurfaceCreated(env, j_vr_shell_.obj());

[bshe, 2017/02/10 20:15:44]

I am not sure I understand correctly here. So contentSurfaceCreated will
indirectly trigger TakeContentSurface. Why not just passing the surface directly
as a parameter of contentSufaceCreated so the TakeContentSurface is no longer
necessary.
Also, is there a reason to not name the java function to contentSurfaceChanged
to match the function name at 361?

[mthiesse, 2017/02/13 15:27:35]

We can't just pass the surface up as a parameter because we need to know if java
took ownership of the surface. I could return a boolean, but I think it's
cleaner to just have a separate function call for java taking ownership of the
surface.

Note that once java takes ownership of the surface, native can't reclaim it - we
have to create a new one.

I'll rename the function.

 }
 
 void VrShell::GvrDelegateReady() {
@@ -371,6 +407,22 @@ void VrShell::UpdateScene(const base::ListValue* args) {
 
 void VrShell::DoUiAction(const UiAction action,
                          const base::DictionaryValue* arguments) {
+  switch (action) {
+    case OMNIBOX_CONTENT:

[bshe, 2017/02/10 20:15:44]

braces here too? Or remove brances at 414

[cjgrant, 2017/02/10 20:20:22]

The braces on 414 keep "bool paused" in the scope of that case.  What's wrong
with braces on some bigger cases, but not trivially short ones?  If this was a
tiny switch block, consistency would seem more important...  My 2 cents.

[mthiesse, 2017/02/13 15:27:35]

Leaving braces as-is.

[bshe, 2017/02/13 15:56:11]

It is not a blocker. It is a suggestion to keep style consistent for each
"case". Either have braces for all cases or no braces at all. I am not saying
there is anything wrong with braces. But I am not sure I understand the life
scope argument. The code wont be any different after you remove the braces. It
is still out of scope after return.

+      html_interface_->HandleOmniboxInput(*arguments);
+      return;
+    case SET_CONTENT_PAUSED: {
+      bool paused;
+      CHECK(arguments->GetBoolean("paused", &paused));
+      SetContentPaused(paused);
+      return;
+    }
+    default:
+      break;
+  }
+  // TODO(mthiesse): Handles these in java through the Tab.
+  if (!main_contents_)
+    return;

[bshe, 2017/02/10 20:15:44]

Does this mean we can't use the navigate controls if it is a native page? If so,
perhaps create a bug?

[mthiesse, 2017/02/13 15:27:35]

No need for a separate bug I think, this is part of the implementation that
isn't finished.

   content::NavigationController& controller = main_contents_->GetController();
   switch (action) {
     case HISTORY_BACK:
@@ -398,20 +450,11 @@ void VrShell::DoUiAction(const UiAction action,
                          std::string(""));
       break;
     }
-    case OMNIBOX_CONTENT:
-      html_interface_->HandleOmniboxInput(*arguments);
-      break;
-    case SET_CONTENT_PAUSED: {
-      bool paused;
-      CHECK(arguments->GetBoolean("paused", &paused));
-      SetContentPaused(paused);
-      break;
-    }
 #if defined(ENABLE_VR_SHELL_UI_DEV)
     case RELOAD_UI:
       ui_contents_->GetController().Reload(content::ReloadType::NORMAL, false);
-      html_interface_.reset(new UiInterface(UiInterface::Mode::STANDARD,
-                                            main_contents_->IsFullscreen()));
+      html_interface_.reset(new UiInterface(UiInterface::Mode::STANDARD));
+      SetUiState();
       vr_web_contents_observer_->SetUiInterface(html_interface_.get());
       break;
 #endif
@@ -437,8 +480,8 @@ void VrShell::MainFrameWasResized(bool width_changed) {
 }
 
 void VrShell::ContentFrameWasResized(bool width_changed) {
-  display::Display display = display::Screen::GetScreen()
-      ->GetDisplayNearestWindow(main_contents_->GetNativeView());
+  display::Display display =
+      display::Screen::GetScreen()->GetDisplayNearestWindow(content_window_);
   PostToGlThreadWhenReady(
       base::Bind(&VrShellGl::ContentBoundsChanged, gl_thread_->GetVrShellGl(),
                  display.size().width(), display.size().height()));
@@ -464,7 +507,8 @@ void VrShell::ContentWasHidden() {
 }
 
 void VrShell::ContentWasShown() {
-  content_input_manager_ = base::MakeUnique<VrInputManager>(main_contents_);
+  if (main_contents_)
+    content_input_manager_ = base::MakeUnique<VrInputManager>(main_contents_);
 }
 
 void VrShell::ForceExitVr() {
@@ -595,19 +639,20 @@ device::mojom::VRDisplayInfoPtr VrShell::CreateVRDisplayInfo(
 // Native JNI methods
 // ----------------------------------------------------------------------------
 
-jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj,
-           const JavaParamRef<jobject>& content_web_contents,
-           jlong content_window_android,
+jlong Init(JNIEnv* env,
+           const JavaParamRef<jobject>& obj,
            const JavaParamRef<jobject>& ui_web_contents,
-           jlong ui_window_android, jboolean for_web_vr,
+           jlong content_window_android,
+           jlong ui_window_android,
+           jboolean for_web_vr,
            const base::android::JavaParamRef<jobject>& delegate,
-           jlong gvr_api, jboolean reprojected_rendering) {
+           jlong gvr_api,
+           jboolean reprojected_rendering) {
   return reinterpret_cast<intptr_t>(new VrShell(
-      env, obj, content::WebContents::FromJavaWebContents(content_web_contents),
-      reinterpret_cast<ui::WindowAndroid*>(content_window_android),
+      env, obj, reinterpret_cast<ui::WindowAndroid*>(content_window_android),
       content::WebContents::FromJavaWebContents(ui_web_contents),
-      reinterpret_cast<ui::WindowAndroid*>(ui_window_android),
-      for_web_vr, VrShellDelegate::GetNativeVrShellDelegate(env, delegate),
+      reinterpret_cast<ui::WindowAndroid*>(ui_window_android), for_web_vr,
+      VrShellDelegate::GetNativeVrShellDelegate(env, delegate),
       reinterpret_cast<gvr_context*>(gvr_api), reprojected_rendering));
 }