device/gamepad: Add UMA and DVLOG for USBDeviceOpen result [chromium/src : main]
0 views
Skip to first unread message
Rob Pitkin (Gerrit)
Jan 12, 2026, 8:30:39 PM (3 days ago) Jan 12
to Alvin Ji, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Matt Reynolds
Rob Pitkin added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Jan 12, 2026, 8:58:34 PM (3 days ago) Jan 12
to Rob Pitkin, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Matt Reynolds and Rob Pitkin
Alvin Ji added 2 comments![]( "Open in Gerrit")
File device/gamepad/xbox_controller_mac.mm
Line 332, Patchset 1 (Latest):
DVLOG(1) << "XboxControllerMac: called USBDeviceClose";Alvin Ji . unresolved
Should this be enclosed in if block of "device_ && device_is_open_"?
Line 459, Patchset 1 (Latest):
RecordUsbDeviceOpenMac(result);Alvin Ji . unresolved
We probably need to consider this further because the "kIOReturnExclusiveAccess" and "KERN_SUCCESS" case will return directly.
Related details
Attention is currently required from:
- Matt Reynolds
- Rob Pitkin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rob Pitkin (Gerrit)
Jan 13, 2026, 12:43:57 PM (2 days ago) Jan 13
to Alvin Ji, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Matt Reynolds
Rob Pitkin added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File device/gamepad/xbox_controller_mac.mm
Should this be enclosed in if block of "device_ && device_is_open_"?
Rob Pitkin
Yes! Forgot brackets, this is done now, thanks for catching that!
We probably need to consider this further because the "kIOReturnExclusiveAccess" and "KERN_SUCCESS" case will return directly.
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Jan 13, 2026, 2:18:28 PM (2 days ago) Jan 13
to Rob Pitkin, Alvin Ji, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Rob Pitkin
Matt Reynolds added 1 comment![]( "Open in Gerrit")
File device/gamepad/xbox_controller_mac.mm
Line 446, Patchset 2 (Latest):
GamepadUsbDeviceOpenMacResult result;Matt Reynolds . unresolved
GamepadUsbDeviceOpenMacResult and XboxControllerMac::OpenDeviceResult represent the same information, let's remove GamepadUsbDeviceOpenMacResult and use the existing enum for metrics.
Related details
Attention is currently required from:
- Alvin Ji
- Rob Pitkin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rob Pitkin (Gerrit)
Jan 13, 2026, 3:36:26 PM (2 days ago) Jan 13
to Alvin Ji, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Matt Reynolds
Rob Pitkin added 2 comments![]( "Open in Gerrit")
Patchset-level comments
Rob Pitkin . resolved
Thanks for the review!
File device/gamepad/xbox_controller_mac.mm
GamepadUsbDeviceOpenMacResult and XboxControllerMac::OpenDeviceResult represent the same information, let's remove GamepadUsbDeviceOpenMacResult and use the existing enum for metrics.
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Jan 14, 2026, 5:49:03 PM (yesterday) Jan 14
to Rob Pitkin, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji, Matt Reynolds and Rob Pitkin
Alvin Ji voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alvin Ji
| Commit-Queue | +1 |
1 comment
File tools/metrics/histograms/metadata/others/enums.xml
Line 96, Patchset 6 (Latest):
<int value="1" label="Exclusive Access Error"/>
<int value="2" label="Other Error"/>Alvin Ji . unresolved
I think we should update this to align with the order in `OpenDeviceResult`
Related details
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
- Rob Pitkin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Jan 14, 2026, 6:00:31 PM (yesterday) Jan 14
to Rob Pitkin, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Matt Reynolds and Rob Pitkin
Alvin Ji added 2 comments![]( "Open in Gerrit")
File device/gamepad/gamepad_uma.h
Line 31, Patchset 6 (Latest):
void RecordUsbDeviceOpenMac(XboxControllerMac::OpenDeviceResult result);Alvin Ji . unresolved
ditto
Line 13, Patchset 6 (Latest):
#include "device/gamepad/xbox_controller_mac.h"Alvin Ji . unresolved
Please add build macro to include this file only for Mac and not for other platforms.
Related details
Attention is currently required from:
- Matt Reynolds
- Rob Pitkin
Rob Pitkin (Gerrit)
2:12 PM (7 hours ago) 2:12 PM
to Alvin Ji, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Matt Reynolds
Rob Pitkin added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File device/gamepad/gamepad_uma.h
Line 31, Patchset 6:
void RecordUsbDeviceOpenMac(XboxControllerMac::OpenDeviceResult result);Alvin Ji . resolved
Rob Pitkinditto
Done
Line 13, Patchset 6:
#include "device/gamepad/xbox_controller_mac.h"Alvin Ji . unresolved
Please add build macro to include this file only for Mac and not for other platforms.
Rob Pitkin
Took a pass at this, I used `IS_APPLE`, but let me know if `IS_MAC` is a better choice.
Related details
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rob Pitkin (Gerrit)
2:19 PM (6 hours ago) 2:19 PM
to Alvin Ji, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Matt Reynolds
Rob Pitkin added 1 comment![]( "Open in Gerrit")
File tools/metrics/histograms/metadata/others/enums.xml
Line 96, Patchset 6:
<int value="1" label="Exclusive Access Error"/>
<int value="2" label="Other Error"/>Alvin Ji . resolved
I think we should update this to align with the order in `OpenDeviceResult`
Rob Pitkin
Just saw this comment too, great catch! Thanks!
Related details
Attention is currently required from:
- Alvin Ji
- Matt Reynolds
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
6:31 PM (2 hours ago) 6:31 PM
to Rob Pitkin, Matt Reynolds, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Matt Reynolds and Rob Pitkin
Alvin Ji voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alvin Ji
| Code-Review | +1 |
2 comments
Patchset-level comments
File device/gamepad/gamepad_uma.h
Line 13, Patchset 6:
#include "device/gamepad/xbox_controller_mac.h"Alvin Ji . resolved
Rob PitkinPlease add build macro to include this file only for Mac and not for other platforms.
Took a pass at this, I used `IS_APPLE`, but let me know if `IS_MAC` is a better choice.
Attention is currently required from:
- Matt Reynolds
- Rob Pitkin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
7:57 PM (1 hour ago) 7:57 PM
to Rob Pitkin, Alvin Ji, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, mac-r...@chromium.org, mattreyno...@chromium.org
Attention needed from Rob Pitkin
Matt Reynolds added 2 comments![]( "Open in Gerrit")
File device/gamepad/gamepad_uma.h
Line 35, Patchset 8 (Latest):
void RecordUsbDeviceOpenMac(XboxControllerMac::OpenDeviceResult result);Matt Reynolds . unresolved
Please move this method into xbox_controller_mac.cc (define it in the unnamed namespace at the top of the file after kSupportedDeviceIds).
gamepad_uma.h is intended for metrics that need to be recorded across multiple data fetchers. For metrics that are only used in one file it's simpler to keep the Record* method in that file.
File device/gamepad/xbox_controller_mac.mm
Line 332, Patchset 8 (Latest):
DVLOG(1) << "XboxControllerMac: called USBDeviceClose";Matt Reynolds . unresolved
Let's use VLOG for these messages since it might be useful to see them in release builds. VLOG has some performance impact even when logging is disabled, but it shouldn't be an issue here since opening/closing a gamepad is infrequent.
Related details
Attention is currently required from:
- Rob Pitkin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages