Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 2388163005: Add WebVR and Gamepad extension metrics (Closed)

Created:
4 years, 2 months ago by billorr
Modified:
4 years, 2 months ago
Reviewers:
haraken, ddorwin, bshe
CC:
chromium-reviews, blink-reviews, asvitkine+watch_chromium.org, amp
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 Committed: https://crrev.com/6805d8ab4296d9033602bad708da31e67e9fff6d Cr-Commit-Position: refs/heads/master@{#424522}

Patch Set 1 #

Total comments: 40

Patch Set 2 : Add gamepad extension metrics, and address most code review feedback before rebase. #

Patch Set 3 : Sync/rebase, histograms.xml update, and fix build errors #

Total comments: 2

Patch Set 4 : Update for remaining CR feedback, along with enum renames #

Total comments: 25

Patch Set 5 : sync/rebase #

Patch Set 6 : CR feedback #

Total comments: 12

Patch Set 7 : CR feedback #

Total comments: 9

Patch Set 8 : CR feedback #

Total comments: 2

Patch Set 9 : sync/rebase #

Total comments: 6

Patch Set 10 : fix typo #

Patch Set 11 : minor cr feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -16 lines) Patch
M chrome/browser/android/vr_shell/vr_shell.cc View 1 2 3 4 5 6 7 8 3 chunks +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/UseCounter.h View 1 2 3 4 5 6 7 8 1 chunk +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/gamepad/Gamepad.idl View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/gamepad/GamepadButton.idl View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/gamepad/GamepadPose.idl View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRController.cpp View 1 2 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +19 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.cpp View 1 2 3 4 5 6 7 8 9 10 11 chunks +25 lines, -4 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 4 chunks +48 lines, -0 lines 0 comments Download

Messages

Total messages: 48 (28 generated)
amp
https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_shell/vr_shell.cc File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_shell/vr_shell.cc#newcode156 chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid ...
4 years, 2 months ago (2016-10-04 22:39:25 UTC) #1
haraken
https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.h File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.h#newcode122 third_party/WebKit/Source/modules/vr/VRDisplay.h:122: enum class PresentationStatus { Can we avoid adding local ...
4 years, 2 months ago (2016-10-05 01:18:42 UTC) #3
ddorwin
https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_shell/vr_shell.cc File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_shell/vr_shell.cc#newcode156 chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid ...
4 years, 2 months ago (2016-10-05 17:48:21 UTC) #5
billorr
Postponing to get deprecated APIs taken care of first. I'll incorporate feedback and and update ...
4 years, 2 months ago (2016-10-05 20:45:55 UTC) #6
billorr
addressed all outstanding comments. I prefer to keep the enum for PresentationResult rather than flattening ...
4 years, 2 months ago (2016-10-06 20:54:53 UTC) #8
billorr
bshe@chromium.org: Please OWNER review changes in vr_shell.cc. haraken, please OWNER review the files other than ...
4 years, 2 months ago (2016-10-06 21:01:54 UTC) #10
ddorwin
https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/vr_shell/vr_shell.cc File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/vr_shell/vr_shell.cc#newcode198 chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, Chromium style for enums is MACRO_STYLE: ...
4 years, 2 months ago (2016-10-06 22:31:29 UTC) #17
billorr
https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/vr_shell/vr_shell.cc File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/vr_shell/vr_shell.cc#newcode198 chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, On 2016/10/06 22:31:28, ddorwin wrote: > ...
4 years, 2 months ago (2016-10-06 23:57:03 UTC) #18
ddorwin
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode154 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", It's interesting that "Web" is dropped from "WebVR" ...
4 years, 2 months ago (2016-10-07 00:41:03 UTC) #21
haraken
LGTM
4 years, 2 months ago (2016-10-07 00:45:35 UTC) #22
billorr
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode154 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", On 2016/10/07 00:41:03, ddorwin wrote: > VRPresentResult seems ...
4 years, 2 months ago (2016-10-07 16:41:55 UTC) #25
ddorwin
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode303 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: On 2016/10/07 16:41:54, billorr wrote: > On 2016/10/07 00:41:03, ...
4 years, 2 months ago (2016-10-10 21:06:58 UTC) #26
bshe
vr_shell lgtm with nits https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android/vr_shell/vr_shell.cc File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android/vr_shell/vr_shell.cc#newcode220 chrome/browser/android/vr_shell/vr_shell.cc:220: case gvr::ViewerType::GVR_VIEWER_TYPE_DAYDREAM: nit: 2 more ...
4 years, 2 months ago (2016-10-11 15:57:46 UTC) #27
billorr
Addressed all CR feedback https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode154 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", On 2016/10/07 16:41:55, billorr ...
4 years, 2 months ago (2016-10-11 17:07:13 UTC) #28
ddorwin
A few minor things. https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode164 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:164: ReportPresentationResult(PresentationResult::RequestedPreset); Present is misspelled. Also, ...
4 years, 2 months ago (2016-10-11 17:46:33 UTC) #35
billorr
https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode164 third_party/WebKit/Source/modules/vr/VRDisplay.cpp:164: ReportPresentationResult(PresentationResult::RequestedPreset); On 2016/10/11 17:46:33, ddorwin wrote: > Present is ...
4 years, 2 months ago (2016-10-11 18:08:17 UTC) #37
ddorwin
lgtm
4 years, 2 months ago (2016-10-11 18:11:22 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2388163005/200001
4 years, 2 months ago (2016-10-11 18:20:58 UTC) #44
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 2 months ago (2016-10-11 20:09:18 UTC) #46
commit-bot: I haz the power
4 years, 2 months ago (2016-10-11 20:11:48 UTC) #48
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/6805d8ab4296d9033602bad708da31e67e9fff6d
Cr-Commit-Position: refs/heads/master@{#424522}

Powered by Google App Engine
This is Rietveld 408576698