[Webkit-unassigned] [Bug 158189] WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=158189

--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 280093
  --> https://bugs.webkit.org/attachment.cgi?id=280093
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280093&action=review

Noticed a few more things while re-looking at this that we could refine later.

>>> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:69
>>> +    [StrictTypeChecking, RaisesException] RTCRtpTransceiver addTransceiver(DOMString kind, optional Dictionary init);
>> 
>> New code should never use Dictionary. These dictionaries should defined.
>> 
>> dictionary RTCRtpTransceiverInit {
>>     boolean send = true;
>>     boolean receive = true;
>>     sequence<MediaStream> streams;
>>     sequence<RTCRtpEncodingParameters> sendEncodings;
>> };
>> 
>> If the bindings generator can’t handle this dictionary, we should fix it so it can handle it. Every single time we instead use the deprecated Dictionary, we add code we will need to later delete!
> 
> Using dictionary from latest published editor's draft [1]. Not all members are supported yet. Should I add a FIXME+bug# for that?
> 
> [1] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtcrtptransceiverinit

I don’t think we need a FIXME about each piece that is missing as we implement something new like this. Eventually it might have some value. but during development it’s probably OK to just do things a bit at a time.

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:58
> +    if (track)
> +        setTrack(WTFMove(track));

Not sure the if here is valuable.

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:47
>      virtual ~RTCRtpSenderReceiverBase() { }

It’s probably more elegant to write "= default" here instead of writing out an empty body.

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:53
> +    RTCRtpSenderReceiverBase()
> +    { }

It’s probably more elegant to write "= default" here instead of writing out an empty body.

>>> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:98
>>> +}
>> 
>> Since bindings create functions like this for all enumerations, we should avoid writing this code ourselves if possible. What makes this necessary?
> 
> Removed parseDirectionString() as part of moving to properly defined dictionary in idl. directionString() is used when the direction value is added to a PeerMediaDescription (which represents direction as a string). I guess there's no nice way to use the generated version from the binding?

Yes, at the moment there is no way to convert an enumeration into the string from the bindings, but if this pattern keeps recurring, I suppose we can add a way.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160531/408f83b9/attachment-0001.html>