Re: Why is RTCIceCandidate mutable

On 11/4/14, 5:59 PM, Martin Thomson wrote:
> Fixing https://www.w3.org/Bugs/Public/show_bug.cgi?id=26644
>
> It's strange to me that we should permit RTCIceCandidate being
> mutable.  Can anyone explain why this is so?

Probably an accident.

> Having it mutable opens all sorts of strange error cases: particularly
> those where candidates are passed to RTCPeerConnection and then
> mutated by script after basic sanity checks have been passed.

+1

> Having it mutable opens all sorts of strange error cases: particularly
> those where candidates are passed to RTCPeerConnection and then
> mutated by script after basic sanity checks have been passed.
>
> Here's what I think the definition should look like:
>
> [Constructor(RTCIceCandidateInit init)]
> interface RTCIceCandidate {
>    readonly attribute DOMString candidate;
>    readonly attribute DOMString? sdpMid;
>    readonly attribute unsigned short? sdpMLineIndex;
> };
>
> Note that the current definition also allows for the init dictionary
> to be omitted or empty.

This again. Dictionaries can always be empty, so requiring them seems 
futile. If we're going to require arguments, why not use required 
arguments? [1]

>    I would prefer that script only be allowed to
> create candidates that are basically valid.  Syntax checking might be
> deferred, but I see no reason to permit a missing or null candidate
> value.

I used to think so, but changed my mind when I discovered two things:

 1. It would require prose for each event to throw on missing argument,
    making it embarrassingly obvious that we're using the wrong construct.

 2. Mozilla's webidl compiler auto-generates all events for us,
    including the init-dictionary pattern, and it naturally generates
    nullable attributes since that's internally consistent, absent a
    context-specific reason for it to be otherwise.


So I think we should give in and make them all nullable, and/or use 
required arguments when that's what we mean. After all, we can't add 
required arguments later, now can we.

> I originally thought that this might be to support the terminal (or
> null) candidate on the icecandidate event.  But that relies on the
> candidate value in RTCIceCandidateEvent being null.

Seems right. Odd that it's not nullable. [2]

> I plan to include these changes in my pull request for 26644.

Fwiw I've been told that editors prefer separate PRs for nits and syntax 
fixes. In my experience this also boosts their chance of landing 
considerably compared to being chained to any of my actual proposals ;-)

.: Jan-Ivar :.

[1] http://lists.w3.org/Archives/Public/public-webrtc/2013Dec/0049.html
[2] http://w3c.github.io/webrtc-pc/#rtcpeerconnectioniceevent