-
Notifications
You must be signed in to change notification settings - Fork 50
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
Merge QuicTransport and Http3Transport interfaces into a single WebTransport interface #134
Conversation
@aboba @yutakahirano @ricea: please take a look. |
1. Let |transport| be the WebTransport on which `getStats` is invoked. | ||
1. Let |p| be a new promise. | ||
1. Return |p| and continue the following steps in background. | ||
1. Gather the stats from the underlying QUIC connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe from a security and privacy standpoint with an HTTP connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them are safe, others are not. I made it an error for now, and added a note saying we should revisit this and split them into "available only in dedicated" and "always available".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
<dfn dictionary>WebTransportOptions</dfn> is a dictionary of parameters | ||
that determine how WebTransport connection is established and used. | ||
|
||
: <dfn for="WebTransportOptions" dict-member>serverCertificateFingerprints</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be less ambiguous to use a non-normative description here and put the normative behaviour in the algorithm steps for the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, though I'd have to write more text about how connection establishment happens in general, so perhaps that would be better left for a follow-up CL.
@@ -484,169 +633,30 @@ dictionary QuicTransportStats { | |||
|
|||
The dictionary SHALL have the following attributes: | |||
|
|||
: <dfn for="QuicTransportStats" dict-member>timestamp</dfn> | |||
: <dfn for="WebTransportStats" dict-member>timestamp</dfn> | |||
:: The `timestamp` for when the stats are gathered, relative to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use DOMTimeStamp: https://heycam.github.io/webidl/#DOMTimeStamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any specific reason for it to use low-res version? https://w3c.github.io/webrtc-pc/#dom-rtcstats uses DOMHighResTimeStamp
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes, DOMHighResTimeStamp
is the right choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #129.