Nidhi Jaju (Gerrit)

unread,

Apr 13, 2026, 2:36:02 AMApr 13

to krishna dheeraj Pannala, android-bu...@system.gserviceaccount.com, Mayur Patil, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Mayur Patil and krishna dheeraj Pannala

Nidhi Jaju added 5 comments

File third_party/blink/renderer/modules/webtransport/web_transport.cc

Line 849, Patchset 3 (Latest): DCHECK(options);

Nidhi Jaju . unresolved

Should this be a `CHECK`? Same in other places.

Line 881, Patchset 3 (Latest): // strong hold during the async gap. nullptr is safe — Persistent<T>

Nidhi Jaju . unresolved

```suggestion
  // strong reference during the async gap. nullptr is safe — Persistent<T>
```

Line 881, Patchset 3 (Latest): // strong hold during the async gap. nullptr is safe — Persistent<T>

Nidhi Jaju . unresolved

What does this mean? Can you update the comment to clarify? Same in other places.

Line 1725, Patchset 3 (Latest): send_stream->setSendOrder(send_order);

Nidhi Jaju . unresolved

If `setSendGroup` was to throw, should we check for that before `setSendOrder`? Same for the bidirectional code.

Line 1721, Patchset 3 (Latest):

    if (!try_catch.HasCaught()) {
      if (send_group) {
        send_stream->setSendGroup(send_group, PassThroughException(isolate));
      }
      send_stream->setSendOrder(send_order);
    }

Nidhi Jaju . unresolved

Since this is the same for both unidirectional and bidirectional streams, can we factor this out into a helper function on `WebTransportSendStream`? Maybe `ApplySendOptions()`?

Open in Gerrit

Related details

Attention is currently required from:

Mayur Patil
krishna dheeraj Pannala

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

krishna dheeraj Pannala (Gerrit)

unread,

Apr 13, 2026, 6:52:09 AM (14 days ago) Apr 13

to Nidhi Jaju, android-bu...@system.gserviceaccount.com, Mayur Patil, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Mayur Patil and Nidhi Jaju

krishna dheeraj Pannala added 5 comments

File third_party/blink/renderer/modules/webtransport/web_transport.cc

Line 849, Patchset 3: DCHECK(options);

Nidhi Jaju . resolved

Should this be a `CHECK`? Same in other places.

krishna dheeraj Pannala

Done. Changed to `CHECK(options)` in all three places: ` createUnidirectionalStream()`, `createBidirectionalStream()`, and `ExtractSendStreamOptions()`.

Line 881, Patchset 3: // strong hold during the async gap. nullptr is safe — Persistent<T>

Nidhi Jaju . resolved

```suggestion
  // strong reference during the async gap. nullptr is safe — Persistent<T>
```

krishna dheeraj Pannala

Addressed together with Comment 2, rewrote the entire comment block for clarity.

Line 881, Patchset 3: // strong hold during the async gap. nullptr is safe — Persistent<T>

Nidhi Jaju . resolved

What does this mean? Can you update the comment to clarify? Same in other places.

krishna dheeraj Pannala

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.

Line 1725, Patchset 3: send_stream->setSendOrder(send_order);

Nidhi Jaju . resolved

If `setSendGroup` was to throw, should we check for that before `setSendOrder`? Same for the bidirectional code.

krishna dheeraj Pannala

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

Line 1721, Patchset 3: if (!try_catch.HasCaught()) {


      if (send_group) {
        send_stream->setSendGroup(send_group, PassThroughException(isolate));
      }
      send_stream->setSendOrder(send_order);
    }

Nidhi Jaju . resolved

Since this is the same for both unidirectional and bidirectional streams, can we factor this out into a helper function on `WebTransportSendStream`? Maybe `ApplySendOptions()`?

krishna dheeraj Pannala

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.

Open in Gerrit

Related details

Attention is currently required from:

Mayur Patil
Nidhi Jaju

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nidhi Jaju (Gerrit)

unread,

Apr 13, 2026, 7:42:34 AM (14 days ago) Apr 13

to krishna dheeraj Pannala, Adam Rice, android-bu...@system.gserviceaccount.com, Mayur Patil, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Adam Rice, Mayur Patil and krishna dheeraj Pannala

Nidhi Jaju voted and added 1 comment

Votes added by Nidhi Jaju

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Nidhi Jaju . resolved

lgtm, let me add @ri...@chromium.org as a third_party/blink/renderer/modules/webtransport/ owner

Open in Gerrit

Related details

Attention is currently required from:

Adam Rice

Mayur Patil
krishna dheeraj Pannala

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Adam Rice (Gerrit)

unread,

Apr 15, 2026, 5:51:01 AM (12 days ago) Apr 15

to krishna dheeraj Pannala, Nidhi Jaju, android-bu...@system.gserviceaccount.com, Mayur Patil, chromiu...@luci-project-accounts.iam.gserviceaccount.com, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Mayur Patil and krishna dheeraj Pannala

Adam Rice voted and added 3 comments

Votes added by Adam Rice

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Adam Rice . resolved

lgtm with nits

File third_party/blink/renderer/modules/webtransport/web_transport.h

Line 197, Patchset 4 (Latest): bool ExtractSendStreamOptions(WebTransportSendStreamOptions* options,

Adam Rice . unresolved

The style guide generally discourages out-params: see https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

I suggest defining a nested class and returning an optional value, something like
```
struct SendStreamOptions {
  STACK_ALLOCATED();
 public:
  WebTransportSendGroup* send_group = nullptr;
  int64_t send_order = 0;
};
std::optional<SendStreamOptions> ExtractSendStreamOptions(
  const WebTransportSendStreamOptions*,
  ExceptionState&);
```

File third_party/blink/renderer/modules/webtransport/web_transport_test.cc

Line 2710, Patchset 4 (Latest):// Regression: sendOrder=0 must not be silently dropped (was a previous bug

Adam Rice . unresolved

I don't understand this comment. This test has exactly the same expectations as CreateUnidirectionalStreamWithEmptyOptions, below, so would behave the same whether the
```
  options->setSendOrder(0);
```
line was there or not. Or is the purpose of the test to prove that calling that changes nothing?

Open in Gerrit

Related details

Attention is currently required from:

Mayur Patil
krishna dheeraj Pannala

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

krishna dheeraj Pannala (Gerrit)

unread,

Apr 15, 2026, 7:32:13 AM (12 days ago) Apr 15

to Adam Rice, Nidhi Jaju, android-bu...@system.gserviceaccount.com, Mayur Patil, chromiu...@luci-project-accounts.iam.gserviceaccount.com, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Adam Rice, Mayur Patil and Nidhi Jaju

krishna dheeraj Pannala added 2 comments

File third_party/blink/renderer/modules/webtransport/web_transport.h

Line 197, Patchset 4: bool ExtractSendStreamOptions(WebTransportSendStreamOptions* options,

Adam Rice . resolved

The style guide generally discourages out-params: see https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

I suggest defining a nested class and returning an optional value, something like
```
struct SendStreamOptions {
  STACK_ALLOCATED();
 public:
  WebTransportSendGroup* send_group = nullptr;
  int64_t send_order = 0;
};
std::optional<SendStreamOptions> ExtractSendStreamOptions(
  const WebTransportSendStreamOptions*,
  ExceptionState&);
```

krishna dheeraj Pannala

Done. Refactored to return `std::optional<SendStreamOptions>` as suggested. Defined a `STACK_ALLOCATED()` nested struct with `send_group` and `send_order` fields, and updated both call sites to use the returned value

File third_party/blink/renderer/modules/webtransport/web_transport_test.cc

Line 2710, Patchset 4:// Regression: sendOrder=0 must not be silently dropped (was a previous bug

Adam Rice . resolved

I don't understand this comment. This test has exactly the same expectations as CreateUnidirectionalStreamWithEmptyOptions, below, so would behave the same whether the
```
  options->setSendOrder(0);
```
line was there or not. Or is the purpose of the test to prove that calling that changes nothing?

krishna dheeraj Pannala

You're right, in the current implementation `sendOrder` defaults to 0 in the IDL, so this test is indistinguishable from `CreateUnidirectionalStreamWithEmptyOptions`. The comment references a bug from a previous CL where there was an `if (send_order != 0)` guard that would skip the explicit zero, but that guard no longer exists. Removed the test.

Open in Gerrit

Related details

Attention is currently required from:

Adam Rice
Mayur Patil
Nidhi Jaju

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

suresh potti (Gerrit)

unread,

Apr 15, 2026, 11:55:29 AM (12 days ago) Apr 15

to krishna dheeraj Pannala, Adam Rice, Nidhi Jaju, android-bu...@system.gserviceaccount.com, Mayur Patil, chromiu...@luci-project-accounts.iam.gserviceaccount.com, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Adam Rice, Mayur Patil, Nidhi Jaju and krishna dheeraj Pannala

suresh potti voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Adam Rice
Mayur Patil
Nidhi Jaju

krishna dheeraj Pannala

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nidhi Jaju (Gerrit)

unread,

Apr 15, 2026, 10:50:12 PM (11 days ago) Apr 15

to krishna dheeraj Pannala, suresh potti, Adam Rice, android-bu...@system.gserviceaccount.com, Mayur Patil, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Adam Rice, Mayur Patil and krishna dheeraj Pannala

Nidhi Jaju voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Adam Rice
Mayur Patil
krishna dheeraj Pannala

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

krishna dheeraj Pannala (Gerrit)

unread,

Apr 15, 2026, 10:51:18 PM (11 days ago) Apr 15

to Nidhi Jaju, suresh potti, Adam Rice, android-bu...@system.gserviceaccount.com, Mayur Patil, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Attention needed from Adam Rice and Mayur Patil

krishna dheeraj Pannala voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Adam Rice
Mayur Patil

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Apr 15, 2026, 11:04:45 PM (11 days ago) Apr 15

to krishna dheeraj Pannala, Nidhi Jaju, suresh potti, Adam Rice, android-bu...@system.gserviceaccount.com, Mayur Patil, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[Webtransport]Accept options in UnidirectionalStream/BidirectionalStream

Add WebTransportSendStreamOptions (inheriting WebTransportSendOptions)
so that createUnidirectionalStream() and createBidirectionalStream() can
accept sendGroup and sendOrder at stream creation time, per the W3C
WebTransport spec.

This CL:
- Adds WebTransportSendOptions and WebTransportSendStreamOptions IDL
  dictionaries. Both sendGroup and sendOrder members are gated behind
  RuntimeEnabled=WebTransportSendGroup.
- Updates createUnidirectionalStream() and createBidirectionalStream()
  IDL signatures to accept optional WebTransportSendStreamOptions.
- Adds ExtractSendStreamOptions() helper to validate options (rejects
  cross-transport sendGroup) and wire them through the async Mojo
  callback to the newly created stream.
- Captures sendGroup via WrapPersistent in the BindOnce callback to
  ensure the group survives the async gap; the transport's registry
  uses WeakMember so unreferenced groups can be collected.
- Applies sendGroup/sendOrder inside the v8::TryCatch scope so any
  exception from setSendGroup() properly rejects the promise.
- Adds 10 new unit tests covering: sendGroup+sendOrder at creation,
  sendOrder-only, sendOrder=0, empty options, explicit null group,
  cross-transport rejection (uni + bidi), and flag-off behavior.

Setters are local-only stubs — Mojo plumbing for network-side initial
priority will be added in a follow-up CL.

Bug: 487117768

Change-Id: Ie627b66d29b9d47b790a463e68eb51ef5d2b53ca

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7743083

Reviewed-by: Nidhi Jaju <nidh...@chromium.org>

Commit-Queue: krishna dheeraj Pannala <kpan...@microsoft.com>

Reviewed-by: suresh potti <sures...@microsoft.com>

Cr-Commit-Position: refs/heads/main@{#1615606}

Files:

M third_party/blink/renderer/bindings/generated_in_modules.gni
M third_party/blink/renderer/bindings/idl_in_modules.gni
M third_party/blink/renderer/modules/webtransport/bidirectional_stream_test.cc
M third_party/blink/renderer/modules/webtransport/web_transport.cc
M third_party/blink/renderer/modules/webtransport/web_transport.h
M third_party/blink/renderer/modules/webtransport/web_transport.idl
A third_party/blink/renderer/modules/webtransport/web_transport_send_options.idl
M third_party/blink/renderer/modules/webtransport/web_transport_send_stream.cc
M third_party/blink/renderer/modules/webtransport/web_transport_send_stream.h
A third_party/blink/renderer/modules/webtransport/web_transport_send_stream_options.idl
M third_party/blink/renderer/modules/webtransport/web_transport_test.cc
D third_party/blink/web_tests/external/wpt/webtransport/sendorder.https.any-expected.txt
D third_party/blink/web_tests/external/wpt/webtransport/sendorder.https.any.serviceworker-expected.txt
D third_party/blink/web_tests/external/wpt/webtransport/sendorder.https.any.sharedworker-expected.txt
D third_party/blink/web_tests/external/wpt/webtransport/sendorder.https.any.worker-expected.txt

Change size: L

Delta: 15 files changed, 456 insertions(+), 61 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Nidhi Jaju, +1 by suresh potti

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Adam Rice (Gerrit)

unread,

Apr 17, 2026, 6:25:14 AM (10 days ago) Apr 17

to Chromium LUCI CQ, krishna dheeraj Pannala, Nidhi Jaju, suresh potti, android-bu...@system.gserviceaccount.com, Mayur Patil, chromium...@chromium.org, Kentaro Hara, Raphael Kubo da Costa, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org

Adam Rice voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[Webtransport]Accept options in UnidirectionalStream/BidirectionalStream [chromium/src : main]

Nidhi Jaju (Gerrit)

Nidhi Jaju added 5 comments

Related details

krishna dheeraj Pannala (Gerrit)

krishna dheeraj Pannala added 5 comments

Related details

Nidhi Jaju (Gerrit)

Nidhi Jaju voted and added 1 comment

Votes added by Nidhi Jaju

1 comment

Related details

Adam Rice (Gerrit)

Adam Rice voted and added 3 comments

Votes added by Adam Rice

3 comments

Related details

krishna dheeraj Pannala (Gerrit)

krishna dheeraj Pannala added 2 comments

Related details

suresh potti (Gerrit)

suresh potti voted Code-Review+1

Related details

Nidhi Jaju (Gerrit)

Nidhi Jaju voted Code-Review+1

Related details

krishna dheeraj Pannala (Gerrit)

krishna dheeraj Pannala voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Adam Rice (Gerrit)

Adam Rice voted Code-Review+1

Related details