Throw specific gUM error when the selected mic has been removed [chromium/src : main]
Henrik Andreasson (Gerrit)
Henrik Andreasson removed Dale Curtis from this change![]( "Open in Gerrit")
- Dale Curtis
Related details
- Guido Urdaneta
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted and added 1 comment![]( "Open in Gerrit")
Votes added by Guido Urdaneta
| Code-Review | +1 |
1 comment
Related details
- Henrik Andreasson
- Olga Sharonova
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tove Petersson (Gerrit)
Tove Petersson added 4 comments![]( "Open in Gerrit")
kFailedDeviceRemoved = 5,
kMaxValue = kFailedDeviceRemoved,Since kMaxValue was updated and this enum is used for UMA (as seen in audio_helper_chromeos_impl.cc), the corresponding enum in the histograms enums.xml file needs to be updated with the new value.
STREAM_OPEN_DEVICE_REMOVED_ERROR = 6,The LINT here points to tools/metrics/histograms/metadata/media/enums.xml. Since you added STREAM_OPEN_DEVICE_REMOVED_ERROR = 6, you must also update the ReferenceOpenOutcome enum in that XML file to include the new value.
DEVICE_REMOVED = 55,MediaStreamRequestResult is used in histograms (see tools/metrics/histograms/enums.xml). You should add DEVICE_REMOVED with value 55 to the MediaStreamRequestResult enum in enums.xml to ensure UMA logs are correctly decoded.
message = "Audio capture device was removed";The commit message says the error message will be Device was removed, but here it is set to Audio capture device was removed. While this is more specific, you may want to ensure the consistency with the description or other error messages.
Related details
- Henrik Andreasson
- Olga Sharonova
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. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 5 comments![]( "Open in Gerrit")
kFailedDeviceRemoved = 5,
kMaxValue = kFailedDeviceRemoved,Since kMaxValue was updated and this enum is used for UMA (as seen in audio_helper_chromeos_impl.cc), the corresponding enum in the histograms enums.xml file needs to be updated with the new value.
Correct but I now realize that we actually don't have to add this here and now since only the native layer on Windows has been changed.
Instead, I have reverted the changes to the `OpenOutcomeChromeOs` enum in the header file. Because `kFailedDeviceRemoved` is only ever generated by the Windows WASAPI implementation, it is unreachable on ChromeOS. I simply added the case to the .cc file with a `NOTREACHED()` assertion to satisfy the compiler's switch checking without needing any XML changes.
The LINT here points to tools/metrics/histograms/metadata/media/enums.xml. Since you added STREAM_OPEN_DEVICE_REMOVED_ERROR = 6, you must also update the ReferenceOpenOutcome enum in that XML file to include the new value.
I was planning to do XML stuff in a follow-up but I agree that it makes sense to address this now. Will update the XML and rename the UMA by adding a 2 at the end.
MediaStreamRequestResult is used in histograms (see tools/metrics/histograms/enums.xml). You should add DEVICE_REMOVED with value 55 to the MediaStreamRequestResult enum in enums.xml to ensure UMA logs are correctly decoded.
Will fix. Will also rename the UMAs that are using this enum.
The commit message says the error message will be Device was removed, but here it is set to Audio capture device was removed. While this is more specific, you may want to ensure the consistency with the description or other error messages.
I followed the existing pattern that Tony added for "Device in use". The shorter message ("Device in use") is used for the gUM exception and the longer for WebRTC logs "Audio capture device already in use". I do the same here and I think it makes sense since a user who calls gUM and sees "Device was removed" will understand what happened. The logs should be more explicit since they are more indirect and taken out of context.
Related details
- Olga Sharonova
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
Asking yuw...@chromium.org for OK on changes in `remoting/host/chromeos/ audio_helper_chromeos_impl.h/cc`
Related details
- Guido Urdaneta
- Olga Sharonova
- Tove Petersson
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
Asking blun...@chromium.org for OK on `media/mojo/common/ input_error_code_converter.cc`
Related details
- Colin Blundell
- Guido Urdaneta
- Olga Sharonova
- Tove Petersson
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
- Colin Blundell
- Guido Urdaneta
- Johannes Kron
- Olga Sharonova
- Tove Petersson
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: kin...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Colin Blundell
- Guido Urdaneta
- Johannes Kron
- Kinuko Yasuda
- Olga Sharonova
- Tove Petersson
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted Code-Review+1![]( "Open in Gerrit")
- Colin Blundell
- Henrik Andreasson
- Johannes Kron
- Kinuko Yasuda
- Olga Sharonova
- Tove Petersson
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Colin BlundellAsking blun...@chromium.org for OK on `media/mojo/common/ input_error_code_converter.cc`
LGTM
Thanks! Note: I didn't review anything outside of the requested file