Add "required" specifiers within dictionaries #569
Conversation
There was a problem hiding this comment.
I am happy with all the required metrics in this PR becoming required, and as such feel free to merge the PR, but per comment below I think it would make sense to expand required to include any metrics where, if implemented, the value "undefined" has no meaning.
webrtc-stats.html
Outdated
| @@ -750,7 +750,7 @@ <h3> | |||
| </h3> | |||
| <div> | |||
| <pre class="idl">dictionary RTCRtpStreamStats : RTCStats { | |||
| unsigned long ssrc; | |||
| required unsigned long ssrc; | |||
| DOMString kind; | |||
There was a problem hiding this comment.
Why is kind not required? It's not valid to have a stream that has an undefined kind. (Even if you null the track, the sender can't change kind, it just silences it)
There was a problem hiding this comment.
Good point. I was considering whether or not we needed an affordance for kinds that weren't supported yet, but I think that would not be an RTP stream. Let's make the kind required.
(Adding a new kind is a huge undertaking in our current document structure anyway.)
| unsigned long contributorSsrc; | ||
| DOMString inboundRtpStreamId; | ||
| required unsigned long contributorSsrc; | ||
| required DOMString inboundRtpStreamId; | ||
| unsigned long packetsContributedTo; |
There was a problem hiding this comment.
Here's an example of a counter that might not be very important, but it always has a defined value if implemented (unlike framesDecoded which is only valid for video and thus has a sensible definition of undefined: the audio stream case).
Opinion: if the only meaning of "undefined" for a metric is "it hasn't been implemented" then I think is should be required, otherwise it is like saying "you can implemented this as undefined by not implementing it"... that sounds self-contradictory.
There was a problem hiding this comment.
Seems like a reasonable argument. However, it applies to all counters.
The counterargument is for the case where an implementor sees value in knowing what ssrcs exist as CSRCs, but doesn't care about the packet value (or finds it too hard to implement correctly) - that implementation is either conformant or non-conformant to this spec; if we make the counter mandatory, it's non-conformant even in the absence of a conformance statement anywhere else.
I think I'm arguing myself into leaving it non-required.
There was a problem hiding this comment.
OK. Am I correct in thinking that this implies that required means "the subset of mandatory to implement stats that can't be undefined if implemented"?
alvestrand
force-pushed
the
required-fields-304
branch
from
1197b40
to
1f1ea9f
Compare
Fixes #304