Devtools protocol for DirectSocket #3 [chromium/src : main]
Vlad Krot (Gerrit)
Vlad Krot voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andrey Kosyakov
- Dmitry Gozman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 10 comments![]( "Open in Gerrit")
GetFrontend()->directUDPSocketChunkReceived(Let's make sure actual chunk sent/received messages are opt-in (ditto for TCP) -- just add an experimental `optional boolean reportDirectSocketTraffic` to `Network.enable`.
std::unique_ptr<protocol::Network::DirectUDPMessage> probe_message =Let's avoid non-trivial conversions that are performed solely for instrumentation in case no devtools are present -- you probably don't need extra overhead for the majority of users here. As a minimum, we should pass data to the probe as span and convert to binary in the agent. Or you can avoid other conversions as well by making it conditional on `CoreProbeSink::HasInspectorNetworkAgents()`, [like this](https://source.chromium.org/search?q=-f:gen%2F%20hasinspector&sq=&ss=chromium%2Fchromium%2Fsrc).
The latter is not without downsides, as it incorporates some knowledge of how probes are routed into your code, whereas it really belongs to `core_probes.json5`, but given these are mostly single-purpose probes this would be fine, and you already are passing protocol domain specific types there.
Ideally we just want to pass a bunch of simple types to probes and let agents build protocol messages, so there's little overhead in case no instrumentation is enabled.
probe::DirectUDPSocketChunkError(*GetScriptState(), inspector_id_, message);There are just local API errors that are appropriately reported to user as JS exceptions (which in turn are [also reported via Runtime domain in v8](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/node_modules/devtools-protocol/pdl/js_protocol.pdl;l=1768?)) -- is there additional value in reporting this via a domain-specific event? We don't usually do this for other APIs.
const connectedSendLoop = (async () => {`connectedSendLoop` unused?
for (let index = 0; index < data.length; index++) {Ahem...
await connectedReadLoop;Let's ditch this and just inline the function.
' options:' + JSON.stringify(createdEvent.params.options, null, 3))Just `testRunner.log(createdEvent.params.options, "options: ")`, please, it's usually preferred to JSON.stringify() since it knows how to stabilize output.
url = url.match(/\/[^\/]+$/);nit: `const match = url.match(...)` (avoid redefining input parameters)
return url;So what exactly are we returning here?
Related details
- Dmitry Gozman
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 10 comments![]( "Open in Gerrit")
GetFrontend()->directUDPSocketChunkReceived(Let's make sure actual chunk sent/received messages are opt-in (ditto for TCP) -- just add an experimental `optional boolean reportDirectSocketTraffic` to `Network.enable`.
How will it be different from just checking enabled variable in InspectorNetworkAgent?
When reportDirectSocketTraffic will be true?
std::unique_ptr<protocol::Network::DirectUDPMessage> probe_message =Let's avoid non-trivial conversions that are performed solely for instrumentation in case no devtools are present -- you probably don't need extra overhead for the majority of users here. As a minimum, we should pass data to the probe as span and convert to binary in the agent. Or you can avoid other conversions as well by making it conditional on `CoreProbeSink::HasInspectorNetworkAgents()`, [like this](https://source.chromium.org/search?q=-f:gen%2F%20hasinspector&sq=&ss=chromium%2Fchromium%2Fsrc).
The latter is not without downsides, as it incorporates some knowledge of how probes are routed into your code, whereas it really belongs to `core_probes.json5`, but given these are mostly single-purpose probes this would be fine, and you already are passing protocol domain specific types there.
Ideally we just want to pass a bunch of simple types to probes and let agents build protocol messages, so there's little overhead in case no instrumentation is enabled.
Done
probe::DirectUDPSocketChunkError(*GetScriptState(), inspector_id_, message);There are just local API errors that are appropriately reported to user as JS exceptions (which in turn are [also reported via Runtime domain in v8](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/node_modules/devtools-protocol/pdl/js_protocol.pdl;l=1768?)) -- is there additional value in reporting this via a domain-specific event? We don't usually do this for other APIs.
Yes, they are also reported as js exceptions. Good catch, removing it.
let bytesEchoed = 0;Vlad Krotunused?
Yes, thanks for spotting.
const connectedSendLoop = (async () => {Vlad Krot`connectedSendLoop` unused?
Done
for (let index = 0; index < data.length; index++) {Vlad KrotAhem...
Done
Let's ditch this and just inline the function.
Done
' options:' + JSON.stringify(createdEvent.params.options, null, 3))Just `testRunner.log(createdEvent.params.options, "options: ")`, please, it's usually preferred to JSON.stringify() since it knows how to stabilize output.
Done
nit: `const match = url.match(...)` (avoid redefining input parameters)
Done
So what exactly are we returning here?
I think it was a mistake. Renamed the variable and return original at the end.
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
GetFrontend()->directUDPSocketChunkReceived(Vlad KrotLet's make sure actual chunk sent/received messages are opt-in (ditto for TCP) -- just add an experimental `optional boolean reportDirectSocketTraffic` to `Network.enable`.
How will it be different from just checking enabled variable in InspectorNetworkAgent?
When reportDirectSocketTraffic will be true?
We just want an explicit opt-in for clients (devtools front-end is not the only one). Most other clients won't care about the traffic and would probably not like the overhead. So this would be an additional parameter to `Network.enable` CDP command, and the traffic would only be reported to clients that expressed their interest by setting it to true.
Related details
- Dmitry Gozman
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
GetFrontend()->directUDPSocketChunkReceived(Vlad KrotLet's make sure actual chunk sent/received messages are opt-in (ditto for TCP) -- just add an experimental `optional boolean reportDirectSocketTraffic` to `Network.enable`.
Andrey KosyakovHow will it be different from just checking enabled variable in InspectorNetworkAgent?
When reportDirectSocketTraffic will be true?
We just want an explicit opt-in for clients (devtools front-end is not the only one). Most other clients won't care about the traffic and would probably not like the overhead. So this would be an additional parameter to `Network.enable` CDP command, and the traffic would only be reported to clients that expressed their interest by setting it to true.
- Andrey Kosyakov
- Dmitry Gozman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
2 comments
optional boolean reportDirectSocketTraffic`experimental`, please!
Related details
- Dmitry Gozman
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
optional boolean reportDirectSocketTrafficVlad Krot`experimental`, please!
Done
Related details
- Dmitry Gozman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Dmitry Gozman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/public/devtools_protocol/browser_protocol.pdl
Insertions: 1, Deletions: 1.
@@ -7004,7 +7004,7 @@
# Longest post body size (in bytes) that would be included in requestWillBeSent notification
optional integer maxPostDataSize
# Whether DirectSocket chunk send/receive events should be reported.
- optional boolean reportDirectSocketTraffic
+ experimental optional boolean reportDirectSocketTraffic
# Returns all browser cookies. Depending on the backend support, will return detailed cookie
# information in the `cookies` field.
```
Change information
Devtools protocol for DirectSocket #3
Send direct socket UDP events to Chrome Devtools Protocol. Later, the UI
will be added to devtools-frontend repository.
The previous 2 CL's:
https://chromium-review.googlesource.com/c/chromium/src/+/6308994
https://chromium-review.googlesource.com/c/chromium/src/+/6434437
The previous CL for tests:
https://chromium-review.googlesource.com/c/chromium/src/+/6458767
- M content/browser/devtools/protocol/network_handler.cc
- M content/browser/devtools/protocol/network_handler.h
- M third_party/blink/public/devtools_protocol/browser_protocol.pdl
- M third_party/blink/renderer/core/inspector/inspector_network_agent.cc
- M third_party/blink/renderer/core/inspector/inspector_network_agent.h
- M third_party/blink/renderer/core/probe/core_probes.h
- M third_party/blink/renderer/core/probe/core_probes.json5
- M third_party/blink/renderer/core/probe/core_probes.pidl
- M third_party/blink/renderer/modules/direct_sockets/socket.cc
- M third_party/blink/renderer/modules/direct_sockets/socket.h
- M third_party/blink/renderer/modules/direct_sockets/tcp_socket.cc
- M third_party/blink/renderer/modules/direct_sockets/tcp_writable_stream_wrapper.cc
- M third_party/blink/renderer/modules/direct_sockets/tcp_writable_stream_wrapper.h
- M third_party/blink/renderer/modules/direct_sockets/udp_readable_stream_wrapper.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_readable_stream_wrapper.h
- M third_party/blink/renderer/modules/direct_sockets/udp_readable_stream_wrapper_unittest.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.h
- M third_party/blink/renderer/modules/direct_sockets/udp_writable_stream_wrapper.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_writable_stream_wrapper.h
- M third_party/blink/renderer/modules/direct_sockets/udp_writable_stream_wrapper_unittest.cc
- R third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/socket-default.php
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/tcp-socket-success.php
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/udp-socket-success.php
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-abort.js
- D third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-chunk-error-expected.txt
- D third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-chunk-error.js
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-success-expected.txt
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-success.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/udp-socket-abort-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/udp-socket-abort.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/udp-socket-success-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/udp-socket-success.js
Code-Review: +1 by Andrey Kosyakov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |