[Direct Sockets] Mark members of OpenInfo dictionaries as required. [chromium/src : main]
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
required DOMString localAddress;You need to fix [this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/direct_sockets/tcp_socket.cc;l=152-155?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20CreateFromAcceptedConnection) if you want to proceed with this change.
Related details
- Paulina Gacek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
required DOMString remoteAddress;Same here: bound UDP socket won't feature `remoteAddress` and `remotePort`.
Realistically the only required thingies here are `readable` and `writable` 😊
Paulina Gacek (Gerrit)
Paulina Gacek added 2 comments![]( "Open in Gerrit")
Same here: bound UDP socket won't feature `remoteAddress` and `remotePort`.
Realistically the only required thingies here are `readable` and `writable` 😊
Done
You need to fix [this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/direct_sockets/tcp_socket.cc;l=152-155?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20CreateFromAcceptedConnection) if you want to proceed with this change.
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Paulina Gacek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
required DOMString localAddress;Paulina GacekYou need to fix [this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/direct_sockets/tcp_socket.cc;l=152-155?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20CreateFromAcceptedConnection) if you want to proceed with this change.
Done
We should probably fix that.
Reilly Grant (Gerrit)
Reilly Grant voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Paulina Gacek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
ReadableStream readable;I don't know if this is supported by WebIDL but to make the Typescript definitions even more useful these streams should be annotated with the chunk type.
Related details
- Paulina Gacek
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
ReadableStream readable;I don't know if this is supported by WebIDL but to make the Typescript definitions even more useful these streams should be annotated with the chunk type.
To my knowledge standard WebIDL does not currently support annotating the chunk type.
However, the WebIDL specification itself does not define any meaning for the content of comments, so I could do something like this:
```
// @ts-chunk-type: Uint8Array
required ReadableStream readable;
// @ts-chunk-type: Uint8Array
required WritableStream writable;
```
And then adjust the converter to generate appropriate chunks. What do you think?
Related details
- Reilly Grant
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. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
ReadableStream readable;Paulina GacekI don't know if this is supported by WebIDL but to make the Typescript definitions even more useful these streams should be annotated with the chunk type.
To my knowledge standard WebIDL does not currently support annotating the chunk type.
However, the WebIDL specification itself does not define any meaning for the content of comments, so I could do something like this:
```
// @ts-chunk-type: Uint8Array
required ReadableStream readable;// @ts-chunk-type: Uint8Array
required WritableStream writable;
```
And then adjust the converter to generate appropriate chunks. What do you think?
Seems reasonable. Let's open a discussion on http://github.com/whatwg/webidl just in case someone's thought about this already.
Related details
- Paulina Gacek
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
ReadableStream readable;Paulina GacekI don't know if this is supported by WebIDL but to make the Typescript definitions even more useful these streams should be annotated with the chunk type.
Reilly GrantTo my knowledge standard WebIDL does not currently support annotating the chunk type.
However, the WebIDL specification itself does not define any meaning for the content of comments, so I could do something like this:
```
// @ts-chunk-type: Uint8Array
required ReadableStream readable;// @ts-chunk-type: Uint8Array
required WritableStream writable;
```
And then adjust the converter to generate appropriate chunks. What do you think?
Seems reasonable. Let's open a discussion on http://github.com/whatwg/webidl just in case someone's thought about this already.
That's a great idea, thank you!
As noted in this discussion (https://github.com/whatwg/webidl/issues/951), the standard doesn't support generic streams because there is no runtime type enforcement. However, I agree that a build-time annotation would be very useful for our internal TypeScript tooling, so I have filed a tracking bug to start that discussion: crbug.com/470275962.
For this specific CL, to avoid blocking on architectural changes, I will stick to the current Chromium convention of using comments. This matches existing patterns in the codebase:
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paulina Gacek (Gerrit)
Paulina Gacek voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
2 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/renderer/modules/direct_sockets/socket_connection.idl
Insertions: 3, Deletions: 0.
@@ -1,5 +1,7 @@
dictionary SocketOpenInfo {
+ // A readable stream of Uint8Array chunks (for TCPSocket) or UDPMessage chunks (for UDPSocket).
required ReadableStream readable;
+ // A writable stream of BufferSource chunks (for TCPSocket) or UDPMessage chunks (for UDPSocket).
required WritableStream writable;
DOMString remoteAddress;
@@ -16,6 +18,7 @@
};
dictionary TCPServerSocketOpenInfo {
+ // A readable stream of TCPSocket objects.
required ReadableStream readable;
DOMString localAddress;
```
Change information
[Direct Sockets] Mark members of OpenInfo dictionaries as required.
This CL updates the WebIDL definitions for TCPSocketOpenInfo and TCPServerSocketOpenInfo to mark these members as required, aligning the IDL with the implementation logic and preventing invalid states where an "open" socket lacks necessary fields.
- M third_party/blink/renderer/modules/direct_sockets/socket_connection.idl
Code-Review: +1 by Andrew Rayskiy, +1 by Reilly Grant
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |