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

Specify "writeDatagrams" in more detail #220

Merged
merged 7 commits into from Mar 30, 2021
Merged

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Mar 8, 2021

  • Define sendDatagrams that can be called at any time.
  • Specify timestamp and expiration duration handling in writeDatagrams.

Related to #179 and #187. This change adds internal slots but doesn't change the public interface.


Preview | Diff

@yutakahirano
Copy link
Contributor Author

@jan-ivar @aboba @ricea PTAL

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

@yutakahirano
Copy link
Contributor Author

@jan-ivar , can you tell me the meaning of the label "Editors can integrate"?

@jan-ivar
Copy link
Member

@yutakahirano Sorry, I didn't see your message sooner!

It's a "please merge the PR at your convenience (after addressing any nits)" label.

It means the PR is good to go, but some detail or nit prevented us from merging it.

In this case there were conflicts that needed resolving. Let me see if I can resolve them.

@jan-ivar
Copy link
Member

Actually, I also promised to look over this one. Let me do that quickly as well.

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

The back-pressure parts LGTM! Some nits, and threading concerns, but I'm happy to open a new issue on the latter.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 392 to 394
The user agent SHOULD run [=sendDatagrams=] for any {{WebTransport}} object whose
{{[[WebTransportState]]}} is `"connected"` as soon as reasonably possbile whenever
the algorithm can make progress.
Copy link
Member

Choose a reason for hiding this comment

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

Doing this on the JS thread seems wrong. I have some concerns about threading in general here, but I'll open a new issue so we can iterate and merge this (with my other nits).

index.bs Outdated Show resolved Hide resolved
@jan-ivar jan-ivar merged commit f76cf8f into main Mar 30, 2021
github-actions bot added a commit that referenced this pull request Mar 30, 2021
Specify "writeDatagrams" in more detail

SHA: f76cf8f
Reason: push, by @jan-ivar

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yutakahirano yutakahirano deleted the yhirano/write-algorithm branch June 14, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants