Add Browser.addMockCamera DevTools command [chromium/src : main]
Minju Kim (Gerrit)
Message from Minju Kim![]( "Open in Gerrit")
Set Ready For Review
Related details
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Thanks for the CL. Is there a design doc for the CDP changes or the overall feature?
Related details
- Ilya Nikolaevskiy
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Thanks for the CL. Is there a design doc for the CDP changes or the overall feature?
(unresolved)
Related details
- Ilya Nikolaevskiy
- Minju Kim
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. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 1 comment![]( "Open in Gerrit")
base::StrCat({"Browser.addMockCamera requires --",It doesn't seem to be the requirement of the tool. True, in tests we might want to disable real cameras. But that's a separate matter, not related to the tool itself.
Is there any reason I'm missing here?
Related details
- Minju Kim
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. |
Minju Kim (Gerrit)
Minju Kim voted and added 3 comments![]( "Open in Gerrit")
Votes added by Minju Kim
| Commit-Queue | +1 |
Alex RudenkoThanks for the CL. Is there a design doc for the CDP changes or the overall feature?
(unresolved)
There is no standalone design doc yet. I added an update to
crbug.com/489736656 describing the current architecture and planned CL split.
Would that be sufficient as the design context for this CL? I plan to prepare a
short design note before adding remove/reset/get commands and WebDriver session
lifecycle support. Please let me know if you would prefer the design note
before this CL lands.
base::StrCat({"Browser.addMockCamera requires --",It doesn't seem to be the requirement of the tool. True, in tests we might want to disable real cameras. But that's a separate matter, not related to the tool itself.
Is there any reason I'm missing here?
You're right. I removed the switch requirement from Browser.addMockCamera. PTAL.
Related details
- Alex Rudenko
- Ilya Nikolaevskiy
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 7 comments![]( "Open in Gerrit")
This is a follow-up to https://crrev.com/c/7991297, which added theIs this change motivated by any WebDriver spec? Could you link it in in the CL description plese?
mock_capture_device_controller_.reset();could we add a test verifying that the mock is cleaned up fully if the CDP session disconnects?
Response BrowserHandler::AddMockCamera(const std::string& device_id) {Let's make sure this is only available to browser targets (agent host type == browser). This currently seems to affect global state and we do not want to expose it to DevTools frontend or other page-scoped clients.
class MockCameraDevToolsProtocolTest : public DevToolsProtocolTest {does it need to be a browser test? could we convert it to an inspector-protocol test https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/http/tests/inspector-protocol/browser/?
// TODO(crbug.com/40781953): This test is flakey on macOS.why is it flaky on MacOS?
# Adds a mock camera for test automation.Does this command require any command args to work? Let's document them.
experimental command addMockCamerais the command scoped per page/frame or is it global? Can we make it frame-specific and more to the Page domain?
Related details
- Ilya Nikolaevskiy
- Minju Kim
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. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy voted Code-Review+1![]( "Open in Gerrit")
- Minju Kim
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. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Minju Kim
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. |
Minju Kim (Gerrit)
Minju Kim added 8 comments![]( "Open in Gerrit")
Thanks so much for review. Please take another look!
This is a follow-up to https://crrev.com/c/7991297, which added theIs this change motivated by any WebDriver spec? Could you link it in in the CL description plese?
Done
could we add a test verifying that the mock is cleaned up fully if the CDP session disconnects?
Done. I have added [add-mock-camera-device-list.js](https://chromium-review.googlesource.com/c/chromium/src/+/8213640/14/third_party/blink/web_tests/http/tests/inspector-protocol/browser/add-mock-camera-device-list.js), which creates a mock camera through a browser-target CDP session, disconnects that session, and waits until the camera is removed from enumerateDevices()
Response BrowserHandler::AddMockCamera(const std::string& device_id) {Let's make sure this is only available to browser targets (agent host type == browser). This currently seems to affect global state and we do not want to expose it to DevTools frontend or other page-scoped clients.
Done
class MockCameraDevToolsProtocolTest : public DevToolsProtocolTest {does it need to be a browser test? could we convert it to an inspector-protocol test https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/http/tests/inspector-protocol/browser/?
Done
// TODO(crbug.com/40781953): This test is flakey on macOS.why is it flaky on MacOS?
The open test [add-mock-camera-open.js](https://chromium-review.googlesource.com/c/chromium/src/+/8213640/14/third_party/blink/web_tests/http/tests/inspector-protocol/browser/add-mock-camera-open.js) exposes a macOS-specific buffer-handle mismatch. The capture client requests a GpuMemoryBuffer, while the shared-memory virtual device provides an UnsafeShmemRegion, causing BroadcastingReceiver::CloneBufferHandle() to crash. I couldn’t confirm that the existing flake issue covers this exact failure, so I’ll track it separately. Please check [here](https://chromium-review.googlesource.com/c/chromium/src/+/8213640/15/third_party/blink/web_tests/TestExpectations)
Does this command require any command args to work? Let's document them.
Done.
is the command scoped per page/frame or is it global? Can we make it frame-specific and more to the Page domain?
It’s shared across pages in the browser rather than tied to a specific page or frame. Since the video-capture service currently uses a single virtual-device registry, I kept it in the Browser domain. The device is cleaned up when the DevTools session that created it disconnects.
Related details
- Alex Rudenko
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. |