Tracks GVR version crossed with headset type using UMA.

chrome/browser/android/vr_shell/vr_usage_monitor.cc

Index: chrome/browser/android/vr_shell/vr_usage_monitor.cc
diff --git a/chrome/browser/android/vr_shell/vr_usage_monitor.cc b/chrome/browser/android/vr_shell/vr_usage_monitor.cc
index 48cc9e3d39fa76533ce7f28db0d2551b9abdb54a..2f0c05c876f62e45a5266427bd0dcd40cc953fa7 100644
--- a/chrome/browser/android/vr_shell/vr_usage_monitor.cc
+++ b/chrome/browser/android/vr_shell/vr_usage_monitor.cc
@@ -162,6 +162,75 @@ void SessionTimer::StopSession(bool continuable, base::Time stop_time) {
   }
 }
 
+#ifdef ANDROID
+bool VrMetricsUtil::logged_gvr_version_ = false;
+
+void VrMetricsUtil::LogGvrVersionForVrViewerType(
+    gvr_context* context,
+    GvrVersionStatus version_status,
+    gvr_version version) {
+  if (!logged_gvr_version_) {

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

Instead, return early. That simplifies the logic by removing one level of
nesting and avoids accidentally adding logic that does get executed below.

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

Done.

+    ViewerType vr_viewer_type =
+        context ? GetVrViewerType(context) : ViewerType::UNKNOWN_TYPE;
+    std::string histogram_name;
+    switch (vr_viewer_type) {
+      case ViewerType::CARDBOARD:
+        histogram_name = "GVRVersion.Cardboard";

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

Thinking ahead to supporting other platforms, maybe we should rename this
VRRuntimeVersion. Then we could have:
VRRuntimeVersion.GVR.Cardboard
VRRuntimeVersion.GVR.Daydream
VRRuntimeVersion.OpenVR
...

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

Done.

+        break;
+      case ViewerType::DAYDREAM:
+        histogram_name = "GVRVersion.Daydream";
+        break;
+      default:
+        histogram_name = "GVRVersion.Unknown";
+        break;
+    }
+
+    uint32_t encoded_gvr_version;
+    switch (version_status) {
+      case GvrVersionStatus::PRECISE:
+        if (version.major < 0 || version.minor < 0 || version.patch < 0) {

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

What are these checks doing? Why would one of them being less than 0 be correct?

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

I just thought you can basically call this function with any version number and
that would screw up the math below. Check is gone.

+          encoded_gvr_version = std::min(version.major, 999) * 1000000 +
+                                std::min(version.minor, 999) * 1000 +
+                                std::min(version.patch, 999);
+        } else {
+          encoded_gvr_version = -2;

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

Magic numbers should be constants.

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

Done.

+        }
+        break;
+      case GvrVersionStatus::OLDER:
+        encoded_gvr_version = 0;

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

Using 0 to mean something seems odd. Probably all exceptions should be negative.

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

Done.

+        break;
+      case GvrVersionStatus::UNKNOWN:
+      default:
+        encoded_gvr_version = -1;

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

As noted previously, we could report all the values, including "not installed.

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

Done.

+        break;
+    }
+
+    UMA_HISTOGRAM_SPARSE_SLOWLY(histogram_name.c_str(), encoded_gvr_version);
+
+    logged_gvr_version_ = true;
+  }
+}
+
+void VrMetricsUtil::LogVrViewerType(gvr_context* context) {
+  UMA_HISTOGRAM_ENUMERATION("VRViewerType",
+                            static_cast<int>(GetVrViewerType(context)),
+                            static_cast<int>(ViewerType::VIEWER_TYPE_MAX));
+}
+
+ViewerType VrMetricsUtil::GetVrViewerType(gvr_context* context) {
+  auto gvr_api = gvr::GvrApi::WrapNonOwned(context);
+  switch (gvr_api->GetViewerType()) {
+    case gvr::ViewerType::GVR_VIEWER_TYPE_DAYDREAM:
+      return ViewerType::DAYDREAM;
+    case gvr::ViewerType::GVR_VIEWER_TYPE_CARDBOARD:
+      return ViewerType::CARDBOARD;
+    default:
+      NOTREACHED();
+      return ViewerType::UNKNOWN_TYPE;
+  }
+}
+#endif  // ANDROID
+
 void VrMetricsHelper::UpdateMode() {
   VRMode mode;
   if (!is_vr_enabled_) {