Tracks GVR version crossed with headset type using UMA.

chrome/browser/android/vr_shell/vr_shell_delegate.cc

Index: chrome/browser/android/vr_shell/vr_shell_delegate.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell_delegate.cc b/chrome/browser/android/vr_shell/vr_shell_delegate.cc
index 6440fd0179783da1ff76b152e8fa2d3cbc34081c..d85aae044939621e09753d2acc51e60e27948add 100644
--- a/chrome/browser/android/vr_shell/vr_shell_delegate.cc
+++ b/chrome/browser/android/vr_shell/vr_shell_delegate.cc
@@ -9,6 +9,7 @@
 #include "base/android/jni_android.h"
 #include "base/callback_helpers.h"
 #include "chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h"
+#include "chrome/browser/android/vr_shell/vr_usage_monitor.h"
 #include "device/vr/android/gvr/gvr_delegate.h"
 #include "device/vr/android/gvr/gvr_device.h"
 #include "device/vr/android/gvr/gvr_device_provider.h"
@@ -16,6 +17,9 @@
 
 using base::android::JavaParamRef;
 using base::android::AttachCurrentThread;
+using base::android::ScopedJavaLocalRef;
+
+static constexpr int kVrOutOfDate = 2;

[ddorwin, 2017/05/16 00:15:51]

Is this a constant defined in Java? If so, we should be explicit about that and
maybe have some sort of compile-time assert. Follow whatever the pattern for
that is.

[tiborg, 2017/05/23 15:47:24]

It's gone in favor of the native VrCoreCompatibility.

 
 namespace vr_shell {
 
@@ -38,8 +42,7 @@ VrShellDelegate::~VrShellDelegate() {
 
 device::GvrDelegateProvider* VrShellDelegate::CreateVrShellDelegate() {
   JNIEnv* env = AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jobject> jdelegate =
-      Java_VrShellDelegate_getInstance(env);
+  ScopedJavaLocalRef<jobject> jdelegate = Java_VrShellDelegate_getInstance(env);
   if (!jdelegate.is_null())
     return GetNativeVrShellDelegate(env, jdelegate.obj());
   return nullptr;
@@ -56,9 +59,9 @@ void VrShellDelegate::SetPresentingDelegate(
     gvr_context* context) {
   presenting_delegate_ = delegate;
   // Clean up the non-presenting delegate.
+  JNIEnv* env = AttachCurrentThread();
   if (presenting_delegate_ && non_presenting_delegate_) {
     non_presenting_delegate_ = nullptr;
-    JNIEnv* env = AttachCurrentThread();
     Java_VrShellDelegate_shutdownNonPresentingNativeContext(
         env, j_vr_shell_delegate_.obj());
   }
@@ -74,6 +77,10 @@ void VrShellDelegate::SetPresentingDelegate(
     base::ResetAndReturn(&present_callback_).Run(true);
     pending_successful_present_request_ = false;
   }
+
+  auto gvr_version = GetGvrVersion(env);
+  VrMetricsUtil::LogGvrVersionForVrViewerType(context, gvr_version.first,
+                                              gvr_version.second);
 }
 
 void VrShellDelegate::RemoveDelegate() {
@@ -197,6 +204,37 @@ void VrShellDelegate::ExitWebVRPresent() {
   }
 }
 
+std::pair<GvrVersionStatus, gvr_version> VrShellDelegate::GetGvrVersion(
+    JNIEnv* env) {
+  ScopedJavaLocalRef<jobject> j_vr_core_info =
+      Java_VrShellDelegate_getVrCoreInfo(env, j_vr_shell_delegate_.obj());
+  jclass j_vr_core_info_class = env->GetObjectClass(j_vr_core_info.obj());
+  jfieldID j_field_id = env->GetFieldID(j_vr_core_info_class, "version",
+                                        "Lcom/google/vr/ndk/base/Version;");
+  jobject j_version_object =
+      env->GetObjectField(j_vr_core_info.obj(), j_field_id);
+  gvr_version version;
+  if (j_version_object) {
+    j_vr_core_info_class = env->GetObjectClass(j_version_object);
+    j_field_id = env->GetFieldID(j_vr_core_info_class, "majorVersion", "I");

[mthiesse, 2017/05/16 01:15:12]

The canonical way of doing what you're doing here is to instead make VrCoreInfo
a native class. Then add a method similar to 'nativeInit' for vr_shell that lets
you create it from java. The java getter function would return the native
pointer to the object it just created and you would just cast it to the native
object, rather than doing all of this hard to maintain and understand parsing.

[tiborg, 2017/05/23 15:47:24]

That tip is golden. Made it much better!

+    version.major = env->GetIntField(j_version_object, j_field_id);
+    j_field_id = env->GetFieldID(j_vr_core_info_class, "minorVersion", "I");
+    version.minor = env->GetIntField(j_version_object, j_field_id);
+    j_field_id = env->GetFieldID(j_vr_core_info_class, "patchVersion", "I");
+    version.patch = env->GetIntField(j_version_object, j_field_id);
+    return std::make_pair(GvrVersionStatus::PRECISE, version);
+  } else {
+    j_field_id = env->GetFieldID(j_vr_core_info_class, "compatibility", "I");
+    jint compatibility = env->GetIntField(j_vr_core_info.obj(), j_field_id);
+
+    if (compatibility == kVrOutOfDate) {
+      return std::make_pair(GvrVersionStatus::OLDER, version);
+    } else {
+      return std::make_pair(GvrVersionStatus::UNKNOWN, version);

[ddorwin, 2017/05/16 00:15:51]

It seems that VR_NOT_AVAILABLE would be important to track too. Really, there
are just four values. Why don't we handle all of them? Convert one enum to
another and log it. We could also report that instead of PRECISE.

[tiborg, 2017/05/23 15:47:25]

Done. All of them are handled.

+    }
+  }
+}
+
 void VrShellDelegate::CreateNonPresentingDelegate() {
   JNIEnv* env = AttachCurrentThread();
   gvr_context* context = reinterpret_cast<gvr_context*>(
@@ -206,6 +244,9 @@ void VrShellDelegate::CreateNonPresentingDelegate() {
       base::MakeUnique<NonPresentingGvrDelegate>(context);
   non_presenting_delegate_->UpdateVSyncInterval(timebase_nanos_,
                                                 interval_seconds_);
+  auto gvr_version = GetGvrVersion(env);
+  VrMetricsUtil::LogGvrVersionForVrViewerType(context, gvr_version.first,
+                                              gvr_version.second);
 }
 
 void VrShellDelegate::OnActivateDisplayHandled(bool present_requested) {