Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865463003:
Tracks GVR version crossed with headset type using UMA. (Closed)
DescriptionTrack GVR version for each headset type using UMA.
- A data point is only collected at most once between Chrome restarts.
- The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell.
- The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature.
- The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN.
BUG=706436
Review-Url: https://codereview.chromium.org/2865463003
Cr-Original-Commit-Position: refs/heads/master@{#476705}
Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e302360ece09f83
Review-Url: https://codereview.chromium.org/2865463003
Cr-Commit-Position: refs/heads/master@{#477308}
Committed: https://chromium.googlesource.com/chromium/src/+/7c35bee6935777ecc39bc419325703fb391d0159
Patch Set 1 #
Total comments: 25
Patch Set 2 : Rebased on ToT, changed logging to UMA_HISTOGRAM_SPARSE_SLOWLY #
Total comments: 34
Patch Set 3 : Incorporated review feedback #
Total comments: 1
Patch Set 4 : Nit: only create native-Java enum if enable_vr is set #
Total comments: 22
Patch Set 5 : Incorporated review feedback #
Total comments: 3
Patch Set 6 : Nit: moved fallthrough comment out of else-block #
Total comments: 10
Patch Set 7 : Incorporated review feedback #Patch Set 8 : Removed we from comment #Patch Set 9 : Added multilevel suffixing #Patch Set 10 : Rebased #Patch Set 11 : Renamed gvr_version to gvr_sdk_version #Patch Set 12 : Fixed tests #Patch Set 13 : Compiles with enable_vr=false. #Patch Set 14 : Fixed comments #Messages
Total messages: 74 (31 generated)
tiborg@chromium.org changed reviewers: + billorr@chromium.org
Hi Bill, Can you please take a look at this UMA tracking. Thanks! Cheers, Tibor
https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:409: VrMetricsHelper::LogVrViewerType(gvr_api); VrMetricsHelper doesn't need to be the only thing that sends usage data. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:15: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" We should try to keep gvr-related code limited to as few places as possible. VRMetricsHelper could be used for Windows. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:80: GVR_NEWER_AND_CARDBOARD = 12, what is the plan when newer versions come out? do we have to rush out new events (and wait 12 weeks for the our changes to reach stable?) should we preemtively add some new buckets? Can we make gvr_version a component in the system info metadata?
https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:409: VrMetricsHelper::LogVrViewerType(gvr_api); On 2017/05/05 17:48:40, billorr wrote: > VrMetricsHelper doesn't need to be the only thing that sends usage data. Agreed. I was moving the function to VrMetricsHelper since I'm using the viewer type for the GVR metric as well. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:15: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" On 2017/05/05 17:48:41, billorr wrote: > We should try to keep gvr-related code limited to as few places as possible. > VRMetricsHelper could be used for Windows. Good point. What about putting an #ifdef ANDROID around this and the function which use GVR? Alternatively, we could add a VrMetricsHelperAndroid. But that's maybe overkill for only three methods. What do you think? https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:80: GVR_NEWER_AND_CARDBOARD = 12, On 2017/05/05 17:48:41, billorr wrote: > what is the plan when newer versions come out? do we have to rush out new > events (and wait 12 weeks for the our changes to reach stable?) > > should we preemtively add some new buckets? > > Can we make gvr_version a component in the system info metadata? Putting the GVR version in the metadata is probably the best solution. But I would put that in a follow up CL since it is likely not finished for M60. I don't know how we could preemtively add buckets since I don't even know how to recognize future versions. The next version could be 1.50, 1.75, 2.3 or whatever.
https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:15: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" On 2017/05/05 19:03:34, tiborg wrote: > On 2017/05/05 17:48:41, billorr wrote: > > We should try to keep gvr-related code limited to as few places as possible. > > VRMetricsHelper could be used for Windows. > > Good point. What about putting an #ifdef ANDROID around this and the function > which use GVR? Alternatively, we could add a VrMetricsHelperAndroid. But that's > maybe overkill for only three methods. What do you think? #ifdef could work, or the histograms could be moved out to vrshell/gl. I'm fine with either. The original purpose of this helper class was to be a webcontentsobserver, and aggregate other data needed to have some of the metrics. If the data is available elsewhere we could log metrics there instead of doing extra plumbing to get here. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:80: GVR_NEWER_AND_CARDBOARD = 12, On 2017/05/05 19:03:35, tiborg wrote: > On 2017/05/05 17:48:41, billorr wrote: > > what is the plan when newer versions come out? do we have to rush out new > > events (and wait 12 weeks for the our changes to reach stable?) > > > > should we preemtively add some new buckets? > > > > Can we make gvr_version a component in the system info metadata? > > Putting the GVR version in the metadata is probably the best solution. But I > would put that in a follow up CL since it is likely not finished for M60. I > don't know how we could preemtively add buckets since I don't even know how to > recognize future versions. The next version could be 1.50, 1.75, 2.3 or > whatever. indeed. One minor thing we could do to make it easier when new versions do come out is change our numbering here: GVR_OLDER_AND_UNKNOWN_TYPE = 0, GVR_OLDER_AND_CARDBOARD = 1, GVR_OLDER_AND_DAYDREAM = 2, GVR_1_10_AND_UNKNOWN_TYPE = 3, GVR_1_10_AND_CARDBOARD = 4, GVR_1_10_AND_DAYDREAM = 5, GVR_1_20_AND_UNKNOWN_TYPE = 6, GVR_1_20_AND_CARDBOARD = 7, GVR_1_20_AND_DAYDREAM = 8, Then when we do have a new version, it just extends our enum (without having weird values out of place).
ddorwin@chromium.org changed reviewers: + amp@chromium.org, ddorwin@chromium.org
+amp who wrote some of this code https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:34: return new VrCoreInfo(null, VrCoreVersionChecker.VR_NOT_SUPPORTED); OOC, why aren't these in an enum instead of ints. amp@? https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:735: int vrCoreCompatibility = versionChecker.getVrCoreInfo().compatibility; Can this throw an exception? See line 875. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_delegate.cc:212: j_field_id = env->GetFieldID(j_vr_core_info_class, "majorVersion", "I"); Aren't these members on a member of this class? https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_delegate.cc:222: if (compatibility == 2) { s/2/constant? https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.cc:241: int datum = static_cast<int>(vr_viewer_type) * 7 + gvr_version_numeric; Magic number. Also, this doesn't seem maintainable. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:67: GVR_OLDER_AND_UNKNOWN_TYPE = 0, Does unknown == not selected? Do we need the GVR update to know for sure? https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:80: GVR_NEWER_AND_CARDBOARD = 12, On 2017/05/05 20:10:21, billorr wrote: > On 2017/05/05 19:03:35, tiborg wrote: > > On 2017/05/05 17:48:41, billorr wrote: > > > what is the plan when newer versions come out? do we have to rush out new > > > events (and wait 12 weeks for the our changes to reach stable?) > > > > > > should we preemtively add some new buckets? > > > > > > Can we make gvr_version a component in the system info metadata? > > > > Putting the GVR version in the metadata is probably the best solution. But I > > would put that in a follow up CL since it is likely not finished for M60. I > > don't know how we could preemtively add buckets since I don't even know how to > > recognize future versions. The next version could be 1.50, 1.75, 2.3 or > > whatever. > > indeed. > > One minor thing we could do to make it easier when new versions do come out is > change our numbering here: > GVR_OLDER_AND_UNKNOWN_TYPE = 0, > GVR_OLDER_AND_CARDBOARD = 1, > GVR_OLDER_AND_DAYDREAM = 2, > > GVR_1_10_AND_UNKNOWN_TYPE = 3, > GVR_1_10_AND_CARDBOARD = 4, > GVR_1_10_AND_DAYDREAM = 5, > > GVR_1_20_AND_UNKNOWN_TYPE = 6, > GVR_1_20_AND_CARDBOARD = 7, > GVR_1_20_AND_DAYDREAM = 8, > > Then when we do have a new version, it just extends our enum (without having > weird values out of place). > We should definitely be able to report versions beyond what already exist because GVR will undoubtedly be updated before the end of this version of Chrome's lifetime. Also, we don't want to keep updating this code. More generally, this doesn't scale and requires maintenance. Can we instead have a separate metric for CardboardViewerGvrVersion and DaydreamViewerGvrVersion, for example? That will also be much easier to reason about on the dashboards. The values could be a string or a number (e.g. for x.y.z, multiple x 8 100 and add y). P.S. Regarding the M60 comment, UMAs are "forever." You can't change the enums and we have to maintain the definitions indefinitely.
https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:34: return new VrCoreInfo(null, VrCoreVersionChecker.VR_NOT_SUPPORTED); On 2017/05/05 20:29:59, ddorwin wrote: > OOC, why aren't these in an enum instead of ints. amp@? I was just following the existing patterns. I assumed there was some reason we couldn't do enum's but I don't really know. Maybe something about having them defined as annotation values was the reason? But then I'm not sure why they needed to be an annotation either (perhaps some GVR api requirement?).
https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:735: int vrCoreCompatibility = versionChecker.getVrCoreInfo().compatibility; On 2017/05/05 20:29:59, ddorwin wrote: > Can this throw an exception? See line 875. getVrCoreInfo() always returns non-null and this function is never called with a null versionChecker parameter (at least that was the assumption so far). It's not the function from line 875 (even though it has the same name). https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_delegate.cc:212: j_field_id = env->GetFieldID(j_vr_core_info_class, "majorVersion", "I"); On 2017/05/05 20:29:59, ddorwin wrote: > Aren't these members on a member of this class? Yep, pretty much. Are you aware of an easier way to get those? https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_delegate.cc:222: if (compatibility == 2) { On 2017/05/05 20:29:59, ddorwin wrote: > s/2/constant? Done. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.cc (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.cc:241: int datum = static_cast<int>(vr_viewer_type) * 7 + gvr_version_numeric; On 2017/05/05 20:29:59, ddorwin wrote: > Magic number. Also, this doesn't seem maintainable. It's gone in favor of the UMA_HISTOGRAM_SPARSE_SLOWLY solution. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:15: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" On 2017/05/05 20:10:21, billorr wrote: > On 2017/05/05 19:03:34, tiborg wrote: > > On 2017/05/05 17:48:41, billorr wrote: > > > We should try to keep gvr-related code limited to as few places as possible. > > > > VRMetricsHelper could be used for Windows. > > > > Good point. What about putting an #ifdef ANDROID around this and the function > > which use GVR? Alternatively, we could add a VrMetricsHelperAndroid. But > that's > > maybe overkill for only three methods. What do you think? > > #ifdef could work, or the histograms could be moved out to vrshell/gl. I'm fine > with either. > > The original purpose of this helper class was to be a webcontentsobserver, and > aggregate other data needed to have some of the metrics. If the data is > available elsewhere we could log metrics there instead of doing extra plumbing > to get here. I put an #ifdef guard around it. Also, moved the static helper logging functions into VrMetricsUtil (a new class). https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:67: GVR_OLDER_AND_UNKNOWN_TYPE = 0, On 2017/05/05 20:29:59, ddorwin wrote: > Does unknown == not selected? Do we need the GVR update to know for sure? It's gone. See comment below. https://codereview.chromium.org/2865463003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_usage_monitor.h:80: GVR_NEWER_AND_CARDBOARD = 12, On 2017/05/05 20:29:59, ddorwin wrote: > On 2017/05/05 20:10:21, billorr wrote: > > On 2017/05/05 19:03:35, tiborg wrote: > > > On 2017/05/05 17:48:41, billorr wrote: > > > > what is the plan when newer versions come out? do we have to rush out new > > > > events (and wait 12 weeks for the our changes to reach stable?) > > > > > > > > should we preemtively add some new buckets? > > > > > > > > Can we make gvr_version a component in the system info metadata? > > > > > > Putting the GVR version in the metadata is probably the best solution. But I > > > would put that in a follow up CL since it is likely not finished for M60. I > > > don't know how we could preemtively add buckets since I don't even know how > to > > > recognize future versions. The next version could be 1.50, 1.75, 2.3 or > > > whatever. > > > > indeed. > > > > One minor thing we could do to make it easier when new versions do come out is > > change our numbering here: > > GVR_OLDER_AND_UNKNOWN_TYPE = 0, > > GVR_OLDER_AND_CARDBOARD = 1, > > GVR_OLDER_AND_DAYDREAM = 2, > > > > GVR_1_10_AND_UNKNOWN_TYPE = 3, > > GVR_1_10_AND_CARDBOARD = 4, > > GVR_1_10_AND_DAYDREAM = 5, > > > > GVR_1_20_AND_UNKNOWN_TYPE = 6, > > GVR_1_20_AND_CARDBOARD = 7, > > GVR_1_20_AND_DAYDREAM = 8, > > > > Then when we do have a new version, it just extends our enum (without having > > weird values out of place). > > > > We should definitely be able to report versions beyond what already exist > because GVR will undoubtedly be updated before the end of this version of > Chrome's lifetime. Also, we don't want to keep updating this code. > > More generally, this doesn't scale and requires maintenance. > > Can we instead have a separate metric for CardboardViewerGvrVersion and > DaydreamViewerGvrVersion, for example? That will also be much easier to reason > about on the dashboards. > > The values could be a string or a number (e.g. for x.y.z, multiple x 8 100 and > add y). > > > P.S. > Regarding the M60 comment, UMAs are "forever." You can't change the enums and we > have to maintain the definitions indefinitely. Ok, these are good points. As discussed offline, changed the logging so that the GVR version is encoded in one value and logged for each headset type separately using UMA_HISTOGRAM_SPARSE_SLOWLY.
billorr@google.com changed reviewers: + billorr@google.com
lgtm
Description was changed from ========== Tracks GVR version crossed with headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. BUG=706436 ========== to ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 ==========
lgtm, one optional follow up comment https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:55: return new VrCoreInfo(null, VrCoreVersionChecker.VR_OUT_OF_DATE); We might be able to pull some kind of version from package utils (the if check on line 52). I'm not sure it would compare with the other version we will get from vr core, though, so I'm not sure it's worth it. Maybe spend a few minutes looking into it, but if it isn't usable feel free to keep the version null here.
ddorwin@chromium.org changed reviewers: + mthiesse@chromium.org
mthiesse, there is a question for you. https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:34: return new VrCoreInfo(null, VrCoreVersionChecker.VR_NOT_SUPPORTED); On 2017/05/05 23:51:54, amp wrote: > On 2017/05/05 20:29:59, ddorwin wrote: > > OOC, why aren't these in an enum instead of ints. amp@? > > I was just following the existing patterns. I assumed there was some reason we > couldn't do enum's but I don't really know. Maybe something about having them > defined as annotation values was the reason? But then I'm not sure why they > needed to be an annotation either (perhaps some GVR api requirement?). mthiesse@, do you know? https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:30: * Chromium. nit: We refer to the product as Chrome in comments. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:22: static constexpr int kVrOutOfDate = 2; 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. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:233: return std::make_pair(GvrVersionStatus::UNKNOWN, version); 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. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_usage_monitor.cc (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:172: if (!logged_gvr_version_) { Instead, return early. That simplifies the logic by removing one level of nesting and avoids accidentally adding logic that does get executed below. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:178: histogram_name = "GVRVersion.Cardboard"; Thinking ahead to supporting other platforms, maybe we should rename this VRRuntimeVersion. Then we could have: VRRuntimeVersion.GVR.Cardboard VRRuntimeVersion.GVR.Daydream VRRuntimeVersion.OpenVR ... https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:191: if (version.major < 0 || version.minor < 0 || version.patch < 0) { What are these checks doing? Why would one of them being less than 0 be correct? https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:196: encoded_gvr_version = -2; Magic numbers should be constants. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:200: encoded_gvr_version = 0; Using 0 to mean something seems odd. Probably all exceptions should be negative. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:204: encoded_gvr_version = -1; As noted previously, we could report all the values, including "not installed. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.h:16: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" We should avoid adding GVR dependencies to more files. That makes more work in order to make this cross-platform. Also, it require the ifdef. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.h:82: static bool logged_gvr_version_; has_... https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22643: +<histogram name="GVRVersion" units="code"> It appears units can be anything. SafeBrowsing.V4StoreVersionRead uses "version number". We could use "normalized version" or something like that. https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22645: + <summary>The GVR version being used for VR.</summary> We should document the magic numbers here so they are seen in the dashboard. https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:85858: + <suffix name="Unknown" label="GVR version used with an unknown headset."/> This would need to be GVR.Unknown if we're going to report a version for it. Can this ever happen?
https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:34: return new VrCoreInfo(null, VrCoreVersionChecker.VR_NOT_SUPPORTED); One of the bizarre quirks of Android/Java is that this is the canonical way to make a c-style enum. See the @IntDef garbage in VrCoreVersionChecker.java. I think there are a couple reasons for it, one being c-like compile-time guarantees, another being performance, another being simple mappings to actual c/c++ enums. Senior Android reviewers actually asked us to use this style of enum.
https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java:17: public VrCoreInfo(Version version, int compatibility) { If you want to share the compatibility enum across java and c++, see https://codesearch.chromium.org/chromium/src/content/public/android/BUILD.gn?... browser_controls_state.h is an example of what you want to do. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:219: j_field_id = env->GetFieldID(j_vr_core_info_class, "majorVersion", "I"); 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.
https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java:17: public VrCoreInfo(Version version, int compatibility) { On 2017/05/16 01:15:12, mthiesse wrote: > If you want to share the compatibility enum across java and c++, see > https://codesearch.chromium.org/chromium/src/content/public/android/BUILD.gn?... > > browser_controls_state.h is an example of what you want to do. Oh yes. Very good advice. Pretty neat. https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:30: * Chromium. On 2017/05/16 00:15:51, ddorwin wrote: > nit: We refer to the product as Chrome in comments. Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:55: return new VrCoreInfo(null, VrCoreVersionChecker.VR_OUT_OF_DATE); On 2017/05/15 23:28:54, amp wrote: > We might be able to pull some kind of version from package utils (the if check > on line 52). > > I'm not sure it would compare with the other version we will get from vr core, > though, so I'm not sure it's worth it. Maybe spend a few minutes looking into > it, but if it isn't usable feel free to keep the version null here. Checked with the GVR team. There seems to be no easy way to do it. I think for the logging we are fine with just logging that the GVR version is older. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:22: static constexpr int kVrOutOfDate = 2; On 2017/05/16 00:15:51, ddorwin wrote: > 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. It's gone in favor of the native VrCoreCompatibility. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:219: j_field_id = env->GetFieldID(j_vr_core_info_class, "majorVersion", "I"); On 2017/05/16 01:15:12, mthiesse wrote: > 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. That tip is golden. Made it much better! https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:233: return std::make_pair(GvrVersionStatus::UNKNOWN, version); On 2017/05/16 00:15:51, ddorwin wrote: > 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. Done. All of them are handled. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_usage_monitor.cc (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:172: if (!logged_gvr_version_) { On 2017/05/16 00:15:51, ddorwin wrote: > Instead, return early. That simplifies the logic by removing one level of > nesting and avoids accidentally adding logic that does get executed below. Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:178: histogram_name = "GVRVersion.Cardboard"; On 2017/05/16 00:15:51, ddorwin wrote: > Thinking ahead to supporting other platforms, maybe we should rename this > VRRuntimeVersion. Then we could have: > VRRuntimeVersion.GVR.Cardboard > VRRuntimeVersion.GVR.Daydream > VRRuntimeVersion.OpenVR > ... Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:191: if (version.major < 0 || version.minor < 0 || version.patch < 0) { On 2017/05/16 00:15:51, ddorwin wrote: > What are these checks doing? Why would one of them being less than 0 be correct? I just thought you can basically call this function with any version number and that would screw up the math below. Check is gone. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:196: encoded_gvr_version = -2; On 2017/05/16 00:15:51, ddorwin wrote: > Magic numbers should be constants. Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:200: encoded_gvr_version = 0; On 2017/05/16 00:15:51, ddorwin wrote: > Using 0 to mean something seems odd. Probably all exceptions should be negative. Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.cc:204: encoded_gvr_version = -1; On 2017/05/16 00:15:51, ddorwin wrote: > As noted previously, we could report all the values, including "not installed. Done. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_usage_monitor.h (right): https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.h:16: #include "third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h" On 2017/05/16 00:15:51, ddorwin wrote: > We should avoid adding GVR dependencies to more files. That makes more work in > order to make this cross-platform. Also, it require the ifdef. Ok, moved it to a separate file. https://codereview.chromium.org/2865463003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_usage_monitor.h:82: static bool logged_gvr_version_; On 2017/05/16 00:15:51, ddorwin wrote: > has_... Done. https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22643: +<histogram name="GVRVersion" units="code"> On 2017/05/16 00:15:51, ddorwin wrote: > It appears units can be anything. > SafeBrowsing.V4StoreVersionRead uses "version number". > We could use "normalized version" or something like that. Done. https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22645: + <summary>The GVR version being used for VR.</summary> On 2017/05/16 00:15:51, ddorwin wrote: > We should document the magic numbers here so they are seen in the dashboard. Done. https://codereview.chromium.org/2865463003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:85858: + <suffix name="Unknown" label="GVR version used with an unknown headset."/> On 2017/05/16 00:15:51, ddorwin wrote: > This would need to be GVR.Unknown if we're going to report a version for it. Can > this ever happen? If we are going to report GVR versions, which are too old to be used, then yes. https://codereview.chromium.org/2865463003/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:47: case VrCoreCompatibility::VR_CORE_COMPATIBILITY_VR_OUT_OF_DATE: ddorwin@ Should we also log that VrCore is too old even if we have a GVR version? At the moment kGvrTooOld is only logged if VrCore is installed and too old to retrieve a version.
https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:79: auto vr_core_info = MakeVrCoreInfo(env); nit: Only use auto when the type is apparent. I don't think it is in this case. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:204: jlong VrShellDelegate::MakeNativeVrCoreInfo(JNIEnv* env, I think you should move this function into (a new) vr_core_info.cc, and call it Init to match the pattern other places use. (Then expose this function through VrCoreInfo.java instead of VrShellDelegate) https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:217: std::unique_ptr<VrCoreInfo>(reinterpret_cast<VrCoreInfo*>( This is pretty confusing. I think you probably want to use base::WrapUnique on the VrCoreInfo*?
Thank you. https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java:14: public final Version gvrVersion; nit: Since this class is GVR-specific, "gvr" may be redundant. Or maybe there is a technical difference. https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909: long nativeVrCoreInfo = (vrCoreInfo.gvrVersion != null) nit: It is generally clearer to use positive logic. Yes, !null is the happy case, but is that more important than positive logic? It's a judgement call. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_core_info.h (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_core_info.h:22: class VrCoreInfo { struct and remove `public`? https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:20: if (has_logged_gvr_version_) { nit: "gvr" is probably irrelevant here, and you wouldn't want that if you extracted this into a cross-platform method. This might be runtime or vr_runtime. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:39: uint32_t encoded_gvr_version; Initialize even though it should be set. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:42: encoded_gvr_version = kVrNotSupported; nit: As above, "gvr" is irrelevant. Dropping is a simple choice here. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:53: } If you intend to fall through in the else case, please add an explicit comment. Otherwise, this looks like a bug. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:56: std::min(vr_core_info.gvr_version.major, 999) * 1000000 + nit: 1000 * 1000 can be easier to read and make the reason clearer.
https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreInfo.java:14: public final Version gvrVersion; On 2017/05/24 07:31:37, ddorwin wrote: > nit: Since this class is GVR-specific, "gvr" may be redundant. Or maybe there is > a technical difference. I wanted to communicate that this is the version of the GVR contained in VrCore and not the version of VrCore itself. https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909: long nativeVrCoreInfo = (vrCoreInfo.gvrVersion != null) On 2017/05/24 07:31:37, ddorwin wrote: > nit: It is generally clearer to use positive logic. Yes, !null is the happy > case, but is that more important than positive logic? It's a judgement call. Makes sense. Changed it. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_core_info.h (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_core_info.h:22: class VrCoreInfo { On 2017/05/24 07:31:37, ddorwin wrote: > struct and remove `public`? Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:20: if (has_logged_gvr_version_) { On 2017/05/24 07:31:38, ddorwin wrote: > nit: "gvr" is probably irrelevant here, and you wouldn't want that if you > extracted this into a cross-platform method. This might be runtime or > vr_runtime. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:39: uint32_t encoded_gvr_version; On 2017/05/24 07:31:37, ddorwin wrote: > Initialize even though it should be set. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:42: encoded_gvr_version = kVrNotSupported; On 2017/05/24 07:31:37, ddorwin wrote: > nit: As above, "gvr" is irrelevant. Dropping is a simple choice here. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:53: } On 2017/05/24 07:31:38, ddorwin wrote: > If you intend to fall through in the else case, please add an explicit comment. > Otherwise, this looks like a bug. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:56: std::min(vr_core_info.gvr_version.major, 999) * 1000000 + On 2017/05/24 07:31:38, ddorwin wrote: > nit: 1000 * 1000 can be easier to read and make the reason clearer. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right): https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:79: auto vr_core_info = MakeVrCoreInfo(env); On 2017/05/24 05:47:26, mthiesse wrote: > nit: Only use auto when the type is apparent. I don't think it is in this case. Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:204: jlong VrShellDelegate::MakeNativeVrCoreInfo(JNIEnv* env, On 2017/05/24 05:47:26, mthiesse wrote: > I think you should move this function into (a new) vr_core_info.cc, and call it > Init to match the pattern other places use. (Then expose this function through > VrCoreInfo.java instead of VrShellDelegate) Done. https://codereview.chromium.org/2865463003/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_delegate.cc:217: std::unique_ptr<VrCoreInfo>(reinterpret_cast<VrCoreInfo*>( On 2017/05/24 05:47:26, mthiesse wrote: > This is pretty confusing. I think you probably want to use base::WrapUnique on > the VrCoreInfo*? Done.
lgtm!
ddorwin@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow: Please review histograms and see my comments there. LGTM with two comments. https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:54: } else { I don't think you need to put it in an else block. It's just what happens. Also, you'd still have a problem that there is no comment outside the if-else. https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:78794: +<histogram name="VRRuntimeVersion.GVR" units="normalized version"> When we support other platforms, we would need the histogram name to be VRRuntimeVersion with suffixes of, for example, OpenVR. Thus, should the suffixes be "GVR.Cardboard"? I don't know whether that's possible, but I think that's what we want. And if it's not, how would we handle things not under ".GVR"?
https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_metrics_util.cc:54: } else { On 2017/05/25 05:14:12, ddorwin wrote: > I don't think you need to put it in an else block. It's just what happens. Also, > you'd still have a problem that there is no comment outside the if-else. Done.
Thanks. https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:55: // We fall through to We generally avoid "we" in comments, especially in the first case. So just "Fall through..." Also, I don't think you need to state the value we are falling through to - this is obvious from the code, prone to rot, and makes the comment longer and more difficult to read. https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:56: // VrCoreCompatibility::VR_CORE_COMPATIBILITY_VR_READY since we can log This should be indented with the above text. (I hope clang format doesn't break that.)
On 2017/05/25 05:14:12, ddorwin wrote: > rkaplow: Please review histograms and see my comments there. > > LGTM with two comments. > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_metrics_util.cc:54: } else { > I don't think you need to put it in an else block. It's just what happens. Also, > you'd still have a problem that there is no comment outside the if-else. > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:78794: +<histogram > name="VRRuntimeVersion.GVR" units="normalized version"> > When we support other platforms, we would need the histogram name to be > VRRuntimeVersion with suffixes of, for example, OpenVR. Thus, should the > suffixes be "GVR.Cardboard"? I don't know whether that's possible, but I think > that's what we want. And if it's not, how would we handle things not under > ".GVR"? How many possible platforms are we talking about? You can actually do multilevel suffixing for this purpose. Where platform is a suffix (only GVR as the entry) - and then you can add more as need be.
https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:66: UMA_HISTOGRAM_SPARSE_SLOWLY(histogram_name.c_str(), encoded_version); since this is using macros this should actually be "VRRuntimeVersion.GVR.Cardboard" - i.e. move switch to here https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:72: UMA_HISTOGRAM_ENUMERATION("VRViewerType", add xml for this? https://codereview.chromium.org/2865463003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2865463003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78794: +<histogram name="VRRuntimeVersion.GVR" units="normalized version"> what is the space of possible versions here btw?
On 2017/05/25 20:57:19, rkaplow wrote: > On 2017/05/25 05:14:12, ddorwin wrote: > > rkaplow: Please review histograms and see my comments there. > > > > LGTM with two comments. > > > > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > > File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): > > > > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > > chrome/browser/android/vr_shell/vr_metrics_util.cc:54: } else { > > I don't think you need to put it in an else block. It's just what happens. > Also, > > you'd still have a problem that there is no comment outside the if-else. > > > > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:78794: +<histogram > > name="VRRuntimeVersion.GVR" units="normalized version"> > > When we support other platforms, we would need the histogram name to be > > VRRuntimeVersion with suffixes of, for example, OpenVR. Thus, should the > > suffixes be "GVR.Cardboard"? I don't know whether that's possible, but I think > > that's what we want. And if it's not, how would we handle things not under > > ".GVR"? > > How many possible platforms are we talking about? You can actually do multilevel > suffixing for this purpose. Where platform is a suffix (only GVR as the entry) - > and then you can add more as need be. At the moment it's only GVR. But we may support OpenVR and Oculus as well and any other future VR platform. Correct me if I'm wrong, does this mean we could do <histogram name="VRRuntimeVersion" ...> ... </histogram> <histogram_suffixes name="VRRuntimeVersionPlatform" separator="."> <suffix name="GVR" label="..."/> <suffix name="OpenVR" label="..."/> <suffix name="Oculus" label="..."/> <affected-histogram name="VRRuntimeVersion"/> </histogram_suffixes> <histogram_suffixes name="VRRuntimeVersionGVRHeadset" separator="."> <suffix name="Cardboard" label="..."/> <suffix name="Daydream" label="..."/> <suffix name="Unknown" label="..."/> <affected-histogram name="VRRuntimeVersion.GVR"/> </histogram_suffixes>
Thank you for the review! https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:55: // We fall through to On 2017/05/25 16:42:26, ddorwin wrote: > We generally avoid "we" in comments, especially in the first case. So just "Fall > through..." Also, I don't think you need to state the value we are falling > through to - this is obvious from the code, prone to rot, and makes the comment > longer and more difficult to read. Done. https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:56: // VrCoreCompatibility::VR_CORE_COMPATIBILITY_VR_READY since we can log On 2017/05/25 16:42:26, ddorwin wrote: > This should be indented with the above text. (I hope clang format doesn't break > that.) Clang-format moved it into the next line. However, per the other comment the long value name is now gone anyway. https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:66: UMA_HISTOGRAM_SPARSE_SLOWLY(histogram_name.c_str(), encoded_version); On 2017/05/25 20:57:27, rkaplow wrote: > since this is using macros this should actually be > "VRRuntimeVersion.GVR.Cardboard" - i.e. move switch to here Done. https://codereview.chromium.org/2865463003/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_metrics_util.cc:72: UMA_HISTOGRAM_ENUMERATION("VRViewerType", There already is an entry for this: https://cs.chromium.org/chromium/src/tools/metrics/histograms/histograms.xml?... https://codereview.chromium.org/2865463003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2865463003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78794: +<histogram name="VRRuntimeVersion.GVR" units="normalized version"> On 2017/05/25 20:57:27, rkaplow wrote: > what is the space of possible versions here btw? At the moment there are 9 publicly released GVR versions we can log. But they release new versions frequently. So, it's hard to predict. The versions are between 1.0 to 1.60 (in encoded form 1000000 to 1060000).
On 2017/05/25 20:57:19, rkaplow wrote: > On 2017/05/25 05:14:12, ddorwin wrote: > > rkaplow: Please review histograms and see my comments there. > > > > LGTM with two comments. > > > > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > > File chrome/browser/android/vr_shell/vr_metrics_util.cc (right): > > > > > https://codereview.chromium.org/2865463003/diff/80001/chrome/browser/android/... > > chrome/browser/android/vr_shell/vr_metrics_util.cc:54: } else { > > I don't think you need to put it in an else block. It's just what happens. > Also, > > you'd still have a problem that there is no comment outside the if-else. > > > > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2865463003/diff/80001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:78794: +<histogram > > name="VRRuntimeVersion.GVR" units="normalized version"> > > When we support other platforms, we would need the histogram name to be > > VRRuntimeVersion with suffixes of, for example, OpenVR. Thus, should the > > suffixes be "GVR.Cardboard"? I don't know whether that's possible, but I think > > that's what we want. And if it's not, how would we handle things not under > > ".GVR"? > > How many possible platforms are we talking about? You can actually do multilevel > suffixing for this purpose. Where platform is a suffix (only GVR as the entry) - > and then you can add more as need be. Not many - probably just a couple more for the foreseeable future. This is it for Android/GVR; there aren't many runtimes on desktop, and I don't know that we'd care about the headset type as much (Cardboard and Daydream are very different). As an example, the next one would likely be "VRRuntimeVersion.OpenVR".
lgtm the suffix schema you mentioned if you need to do multi-level suffixing looks right
lgtm
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@google.com, amp@chromium.org, mthiesse@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2865463003/#ps160001 (title: "Added multilevel suffixing")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@google.com, amp@chromium.org, mthiesse@chromium.org, rkaplow@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2865463003/#ps180001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@google.com, amp@chromium.org, mthiesse@chromium.org, rkaplow@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2865463003/#ps200001 (title: "Renamed gvr_version to gvr_sdk_version")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tiborg@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@chromium.org: Please review changes in chrome/android/BUILD.gn chrome/browser/android/chrome_jni_registrar.cc Hi Ted, Would you mind taking a look at chrome/android/BUILD.gn and chrome/browser/android/chrome_jni_registrar.cc? Thank you! Cheers, Tibor
On 2017/06/01 15:55:21, tiborg wrote: > mailto:tedchoc@chromium.org: Please review changes in > chrome/android/BUILD.gn > chrome/browser/android/chrome_jni_registrar.cc > > Hi Ted, > > Would you mind taking a look at chrome/android/BUILD.gn and > chrome/browser/android/chrome_jni_registrar.cc? > > Thank you! > > Cheers, > Tibor lgtm
The CQ bit was checked by tiborg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@google.com, amp@chromium.org, tedchoc@chromium.org, mthiesse@chromium.org, rkaplow@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2865463003/#ps220001 (title: "Fixed tests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496419374016760,
"parent_rev": "f02dae5603f8310b91a35472b1d61d8ef7c13ab8", "commit_rev":
"92c0c2fb929a969c0ca9b7626e302360ece09f83"}
Message was sent while issue was closed.
Description was changed from ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 ========== to ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 Review-Url: https://codereview.chromium.org/2865463003 Cr-Commit-Position: refs/heads/master@{#476705} Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30...
Message was sent while issue was closed.
On 2017/06/02 17:29:28, commit-bot: I haz the power wrote: > Committed patchset #12 (id:220001) as > https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... This patch broke https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde..., will revert it.
Message was sent while issue was closed.
Description was changed from ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 Review-Url: https://codereview.chromium.org/2865463003 Cr-Commit-Position: refs/heads/master@{#476705} Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... ========== to ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 Review-Url: https://codereview.chromium.org/2865463003 Cr-Commit-Position: refs/heads/master@{#476705} Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... ==========
The CQ bit was checked by tiborg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@google.com, amp@chromium.org, tedchoc@chromium.org, rkaplow@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2865463003/#ps260001 (title: "Fixed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496761963700300,
"parent_rev": "fd9d71980637bf89994f61476acd7760da72612d", "commit_rev":
"7c35bee6935777ecc39bc419325703fb391d0159"}
Message was sent while issue was closed.
Description was changed from ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 Review-Url: https://codereview.chromium.org/2865463003 Cr-Commit-Position: refs/heads/master@{#476705} Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... ========== to ========== Track GVR version for each headset type using UMA. - A data point is only collected at most once between Chrome restarts. - The data point is either collected once the user navigates to a page that uses WebVR API or enters VrShell. - The GVR version is retrieved from Java and uploaded in native. By doing so, we can also track if older, non-supported versions of GVR were installed at the time the user tried to use a VR feature. - The GVR version is encoded as (major * 10^6 + minor * 10^3 + patch) and reported as a UMA_HISTOGRAM_SPARSE_SLOWLY for the headset types CARDBOARD, DAYDREAM and UNKNOWN. BUG=706436 Review-Url: https://codereview.chromium.org/2865463003 Cr-Original-Commit-Position: refs/heads/master@{#476705} Committed: https://chromium.googlesource.com/chromium/src/+/92c0c2fb929a969c0ca9b7626e30... Review-Url: https://codereview.chromium.org/2865463003 Cr-Commit-Position: refs/heads/master@{#477308} Committed: https://chromium.googlesource.com/chromium/src/+/7c35bee6935777ecc39bc4193257... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/7c35bee6935777ecc39bc4193257... |
