Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add subprotocols constructor argument to WebTransport #598

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Apr 8, 2024

Fixes #536. Right now this seems only defined for HTTP/3, so that's what I went with. Is this what we want?


Preview | Diff

@jan-ivar
Copy link
Member Author

jan-ivar commented Apr 9, 2024

Meeting:

  • overall reaction seems positive
  • s/protocols/subprotocols/
  • Make sure to preserve order in header field list.

@jan-ivar jan-ivar changed the title Add protocols constructor argument to WebTransport Add subprotocols constructor argument to WebTransport Apr 11, 2024
@@ -703,6 +708,11 @@ A {{WebTransport}} object has the following internal slots.
<td class="non-normative">The number of concurrently open
[=bidirectional=] streams the application anticipates the server creating, or null.
</tr>
<tr>
<td><dfn>`[[Protocol]]`</dfn>
<td class="non-normative">A string indicating the subprotocol selected by the server,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a reference for "subprotocol"? I know that it's a websocket concept, so maybe that can be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In WebSocket, the options-member is protocols, but refers to each string as a "subprotocol name" that has to "match the requirements for elements that comprise the value of Sec-WebSocket-Protocol fields as defined by The WebSocket protocol. [WSP]"

image

WebSockets is mostly what I modeled this PR on. We could s/subprotocol/subprotocol name/ maybe if that helps?

I could also break out "subprotocol name" as a definition if that helps?

index.bs Outdated Show resolved Hide resolved
1. Let |protocols| be {{WebTransport/constructor(url, options)/options}}'s
{{WebTransportOptions/protocols}}
1. If any of the values in |protocols| occur more than once, fail to match
the requirements for elements that comprise the value of `WebTransport-Subprotocol`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop the "sub" and use WebTransport-Protocol instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to be going the other way naming-wise judging from other feedback I've received.

But here specifically it needs to match ietf-wg-webtrans/draft-ietf-webtrans-http3#144 which is already merged.

index.bs Outdated Show resolved Hide resolved
[Section 4.1](https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-overview-06#section-4.1-2.2.1),
with using |origin|, [=ASCII serialization of an origin|serialized=] and [=isomorphic encoded=],
as the [:Origin:] header of the request.
When establishing a session, the client MUST NOT provide any [=credentials=].
The resulting underlying transport stream is referred to as the session's <dfn>CONNECT stream</dfn>.
Additionally, if the [=underlying connection=] is using HTTP/3 and the |protocols| array is non-empty,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that you mentioned only specifying this for HTTP/3 as it's only defined in that IETF doc, but I'm not sure if making this HTTP/3 specific is what we want. We should probably file an issue to get it added to the HTTP/2 or overview doc as well, and keep the language here as protocol-agnostic as possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this should be the goal. @nidhijaju or @vasilvv can you open this issue? But in the meantime, can we iterate here by merging what we have, and come back and fix this when it's available in the overview doc?

jan-ivar and others added 2 commits April 23, 2024 14:50
Co-authored-by: Nidhi Jaju <41948741+nidhijaju@users.noreply.github.com>
@jan-ivar
Copy link
Member Author

Meeting:

  • @nidhijaju to open issue on HTTP/2 and overview doc
  • then update this PR to reference overview doc and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconsider whether we need sub-protocol negotiation
3 participants