[Webtransport]Accept options in UnidirectionalStream/BidirectionalStream [chromium/src : main]
Nidhi Jaju (Gerrit)
Nidhi Jaju added 5 comments![]( "Open in Gerrit")
DCHECK(options);Should this be a `CHECK`? Same in other places.
// strong hold during the async gap. nullptr is safe — Persistent<T>```suggestion
// strong reference during the async gap. nullptr is safe — Persistent<T>
```
// strong hold during the async gap. nullptr is safe — Persistent<T>What does this mean? Can you update the comment to clarify? Same in other places.
send_stream->setSendOrder(send_order);If `setSendGroup` was to throw, should we check for that before `setSendOrder`? Same for the bidirectional code.
if (!try_catch.HasCaught()) {
if (send_group) {
send_stream->setSendGroup(send_group, PassThroughException(isolate));
}
send_stream->setSendOrder(send_order);
}Since this is the same for both unidirectional and bidirectional streams, can we factor this out into a helper function on `WebTransportSendStream`? Maybe `ApplySendOptions()`?
Related details
- Mayur Patil
- krishna dheeraj Pannala
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. |
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala added 5 comments![]( "Open in Gerrit")
Should this be a `CHECK`? Same in other places.
Done. Changed to `CHECK(options)` in all three places: ` createUnidirectionalStream()`, `createBidirectionalStream()`, and `ExtractSendStreamOptions()`.
// strong hold during the async gap. nullptr is safe — Persistent<T>```suggestion
// strong reference during the async gap. nullptr is safe — Persistent<T>
```
Addressed together with Comment 2, rewrote the entire comment block for clarity.
// strong hold during the async gap. nullptr is safe — Persistent<T>What does this mean? Can you update the comment to clarify? Same in other places.
Done. Rewrote the comment to be explicit about what's happening:
> Capture send_group via WrapPersistent so it survives the asynchronous
Mojo round-trip (between `CreateStream()` and the callback). The
transport's ` send_groups_` registry uses `WeakMember`, so without this
strong reference the group could be garbage-collected if JS drops all
references before the callback fires. `nullptr` is safe, `Persistent<T>`
accepts null.
If `setSendGroup` was to throw, should we check for that before `setSendOrder`? Same for the bidirectional code.
Fixed `ApplySendStreamOptions()` now calls `setSendGroup()` first, checks `exception_state.HadException()`, and returns early before calling `setSendOrder()` if it threw. Applied to both unidirectional and bidirectional paths
if (!try_catch.HasCaught()) {
if (send_group) {
send_stream->setSendGroup(send_group, PassThroughException(isolate));
}
send_stream->setSendOrder(send_order);
}Since this is the same for both unidirectional and bidirectional streams, can we factor this out into a helper function on `WebTransportSendStream`? Maybe `ApplySendOptions()`?
Done. Added `WebTransportSendStream::ApplySendStreamOptions(send_group, send_order, exception_state)` that handles the sequencing correctly (setSendGroup → check exception → setSendOrder). Both `OnCreateSendStreamResponse` and `OnCreateBidirectionalStreamResponse` now use this single helper.
Related details
- Mayur Patil
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
1 comment
lgtm, let me add @ri...@chromium.org as a third_party/blink/renderer/modules/webtransport/ owner
Related details
- Adam Rice
- Mayur Patil
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |