Re: Issue 198: Philipp Hancke's Review Comments

removing any issue where my answer is "+1", "+1000", "LGTM", "thank you" 
etc to keep this short.

> 7) In Section 13.1, change:
>
> "For RTCDtlsTransport.getStats(), check whether RTCDtlsTransport.start() has been called; if not, throw an InvalidStateError exception. For RTCIceTransport.getStats(), check whether RTCIceTransport.start() has been called; if not, or if RTCIceTransport.stop() has been called, throw an InvalidStateError exception. For RTCRtpSender.getStats(), check whether RTCRtpSender.send(parameters) has been called; if not, throw an InvalidStateError exception. For RTCRtpReceiver.getStats(), check whether RTCRtpReceiver.receive(parameters) has been called; if not, throw an InvalidStateError exception."
>
> To:
>
> "For RTCDtlsTransport.getStats(), check whether RTCDtlsTransport.state is "closed"; if so, throw an InvalidStateError exception. For RTCIceTransport.getStats(), check whether RTCIceTransport.state is "closed"; if so, or if RTCIceTransport.stop() has been called, throw an InvalidStateError exception. For RTCRtpSender.getStats(), check whether RTCRtpSender.send(parameters) has been called; if not, throw an InvalidStateError exception. For RTCRtpReceiver.getStats(), check whether RTCRtpReceiver.receive(parameters) has been called; if not, throw an InvalidStateError exception."

see next email (not today).

> 8) This won't affect the ability to emulate WebRTC 1.0 over ORTC, only vice-versa.

see next email, too


> 15) I found it surprising that urls can be a string. But it also can in PC
>
> [BA] Yes. Kept it the same in ORTC for consistency with 1.0.

I suppose a shim could be smart enough to do the right thing so I am not 
sure consistency is such an issue. But i'll probably argue over this in 
the peerconnection spec.
	
> 17) page 12, section 3.11
>
> WebIDL
> how does this deal with the extensibility defined in RFC 5245 --
> https://tools.ietf.org/html/rfc5245#section-15.1 (extension-att-name etc)
>
> [BA] It is possible to add variables to the dictionary. so it is possible to add new attributes for extension-att-name/extension-att-value. Do you think there is something missing in particular that needs to be added?

the generation attribute from 
http://xmpp.org/extensions/xep-0176.html#protocol-syntax is pretty common.


> "udp",
> see case question above (19). Jingle and SDP and consequently chrome and firefox disagree here
>
> [BA] We do use "udp"/"tcp" as well as "RTP"/"RTCP".  Do you think that is a problem?

No. Chrome and Firefox are dealing ok with this currently.

> 29) Cleaned up the syntax; hopefully it is correct now.

In the 5-21-2015 version, The indent of mySignaller.myOfferTracks (ex 7) 
is now an odd mix of 3, 1 and 2.
Looks correct to me though, just nitpicking on the indent :-)

> 30) Are there other places where you think logging would be useful?

no, so i'd just inline this for brevity.

> 34) Seems odd to have the function definition at the top.

Doesn't jshint barf because you use a function before defining it?
JS style question.

Received on Thursday, 4 June 2015 04:39:37 UTC