Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867093005:
Add UMA metrics to track user action sequence in VR (Closed)
DescriptionAdd UMA metrics to track user action sequence in VR
This change adds user actions for entering/exiting Vr, and starting/stopping VR presentation.
BUG=717150
Review-Url: https://codereview.chromium.org/2867093005
Cr-Commit-Position: refs/heads/master@{#471483}
Committed: https://chromium.googlesource.com/chromium/src/+/8277e32f6d5d92edc839fb7c3ac7496a0f3d59d8
Patch Set 1 #
Total comments: 1
Patch Set 2 : uncomment assert #
Total comments: 2
Messages
Total messages: 24 (11 generated)
billorr@chromium.org changed reviewers: + bajones@chromium.org, jwd@chromium.org, mthiesse@chromium.org
bajones@, please review blink webvr mthiesse@, please review VrShellDelegate.java jwd@, please review actions.xml
billorr@google.com changed reviewers: + billorr@google.com
https://codereview.chromium.org/2867093005/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/2867093005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:483: // assert !mPaused; I will uncomment this. I hitt the assert at one point, and commented it out.
The CQ bit was checked by billorr@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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:225: if (first_present && Why duplicate the logic for when we should (usually) successfully present? Why not record the action when we call display_->RequestPresent to know that the page successfully requested presentation. If you want to also capture failures, why not record an action unconditionally when the page calls RequestPresent, and another action when successful?
https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:225: if (first_present && On 2017/05/10 14:58:13, mthiesse wrote: > Why duplicate the logic for when we should (usually) successfully present? > > Why not record the action when we call display_->RequestPresent to know that the > page successfully requested presentation. > > If you want to also capture failures, why not record an action unconditionally > when the page calls RequestPresent, and another action when successful? I'm trying to filter calls of "requestPresent" to only user actions - which include putting the device in a headset and clicking to initiate presentation. I also filter to the first present because otherwise we are already presenting. This is almost identitical to the logic above (could be reduced to just "if(first_present)"), but this appears more clear and future proof if the logic changes in this function. I put it here because it is after first_present is defined. This could be moved up to the top of the function to get all calls, including those that will fail, or moved further down to be more likely to indicate success at presenting. I don't have a strong preference for either of those, and I think realistically they should give very similar results. https://chromium.googlesource.com/chromium/src/+/72a5c913974f7c92e7dc393bf870... Guidance is to have one event triggered per user action. This is following that if we consider clicking "enter vr" and putting the phone in the headset as two separate actions.
https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2867093005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:225: if (first_present && On 2017/05/10 14:58:13, mthiesse wrote: > Why duplicate the logic for when we should (usually) successfully present? > > Why not record the action when we call display_->RequestPresent to know that the > page successfully requested presentation. > > If you want to also capture failures, why not record an action unconditionally > when the page calls RequestPresent, and another action when successful? I'm trying to filter calls of "requestPresent" to only user actions - which include putting the device in a headset and clicking to initiate presentation. I also filter to the first present because otherwise we are already presenting. This is almost identitical to the logic above (could be reduced to just "if(first_present)"), but this appears more clear and future proof if the logic changes in this function. I put it here because it is after first_present is defined. This could be moved up to the top of the function to get all calls, including those that will fail, or moved further down to be more likely to indicate success at presenting. I don't have a strong preference for either of those, and I think realistically they should give very similar results. https://chromium.googlesource.com/chromium/src/+/72a5c913974f7c92e7dc393bf870... Guidance is to have one event triggered per user action. This is following that if we consider clicking "enter vr" and putting the phone in the headset as two separate actions.
metrics LGTM with comment.
lgtm jwd did you forget to post your comment?
On 2017/05/10 19:22:55, jwd wrote: > metrics LGTM with comment. jwd, did you have a comment, or is this good to commit?
The CQ bit was checked by billorr@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, FWIW
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by billorr@google.com
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": 20001, "attempt_start_ts": 1494621579364320,
"parent_rev": "e87608851db608de9bde8ef458bf34c48b42405e", "commit_rev":
"8277e32f6d5d92edc839fb7c3ac7496a0f3d59d8"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA metrics to track user action sequence in VR This change adds user actions for entering/exiting Vr, and starting/stopping VR presentation. BUG=717150 ========== to ========== Add UMA metrics to track user action sequence in VR This change adds user actions for entering/exiting Vr, and starting/stopping VR presentation. BUG=717150 Review-Url: https://codereview.chromium.org/2867093005 Cr-Commit-Position: refs/heads/master@{#471483} Committed: https://chromium.googlesource.com/chromium/src/+/8277e32f6d5d92edc839fb7c3ac7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/8277e32f6d5d92edc839fb7c3ac7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
