[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 #5 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(In reply to comment #3)
> Comment on attachment 280093 [details]
> Proposed patch

Thanks for reviewing. See comments inline.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280093&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:265
> > +    RefPtr<RealtimeMediaSource> remoteSource = m_mediaEndpoint->createMutedRemoteSource(transceiverMid, sourceType);
> > +    RefPtr<MediaStreamTrackPrivate> remoteTrackPrivate = MediaStreamTrackPrivate::create(WTFMove(remoteSource), trackId);
> > +    Ref<MediaStreamTrack> remoteTrack = MediaStreamTrack::create(*m_client->scriptExecutionContext(), *remoteTrackPrivate);
> 
> Types should all be auto.
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:164
> > +    RefPtr<RTCRtpSender> sender = RTCRtpSender::create(WTFMove(track), Vector<String>(), *this);
> > +    RefPtr<RTCRtpReceiver> receiver = m_backend->createReceiver(transceiverMid, trackKind, trackId);
> > +    Ref<RTCRtpTransceiver> transceiver = RTCRtpTransceiver::create(WTFMove(sender), WTFMove(receiver));
> 
> Types should just be auto.
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:187
> > +    RefPtr<RTCRtpSender> sender = RTCRtpSender::create(kind, Vector<String>(), *this);
> > +    RefPtr<RTCRtpReceiver> receiver = m_backend->createReceiver(transceiverMid, kind, trackId);
> > +    Ref<RTCRtpTransceiver> transceiver = RTCRtpTransceiver::create(WTFMove(sender), WTFMove(receiver));
> 
> Types should just be auto.

Fixed all above. I've been thinking about when to use auto. I guess it's OK when there's obvious redundant information; like the above cases :).

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:69
> > +    [StrictTypeChecking, RaisesException] RTCRtpTransceiver addTransceiver(MediaStreamTrack track, optional Dictionary init);
> > +    [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

> > Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:98
> > +const String& RTCRtpTransceiver::directionString() const
> > +{
> > +    switch (m_direction) {
> > +    case Direction::Sendrecv: return sendrecvString();
> > +    case Direction::Sendonly: return sendonlyString();
> > +    case Direction::Recvonly: return recvonlyString();
> > +    case Direction::Inactive: return inactiveString();
> > +    }
> > +
> > +    ASSERT_NOT_REACHED();
> > +    return emptyString();
> > +}
> > +
> > +static bool parseDirectionString(const String& string, RTCRtpTransceiver::Direction& outDirection)
> > +{
> > +    if (string == sendrecvString())
> > +        outDirection = RTCRtpTransceiver::Direction::Sendrecv;
> > +    else if (string == sendonlyString())
> > +        outDirection = RTCRtpTransceiver::Direction::Sendonly;
> > +    else if (string == recvonlyString())
> > +        outDirection = RTCRtpTransceiver::Direction::Recvonly;
> > +    else if (string == inactiveString())
> > +        outDirection = RTCRtpTransceiver::Direction::Inactive;
> > +    else
> > +        return false;
> > +
> > +    return true;
> > +}
> 
> 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?

> > Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:110
> > +bool RTCRtpTransceiver::configureWithDictionary(const Dictionary& dictionary)
> > +{
> > +    String direction;
> > +    if (dictionary.get("direction", direction)) {
> > +        if (!parseDirectionString(direction, m_direction))
> > +            return false;
> > +    }
> > +
> > +    // FIMXE: fix streams
> > +    return true;
> > +}
> 
> Since bindings create functions like this for dictionaries, we should avoid
> writing this code ourselves if possible. What makes this necessary? The
> bindings should convert dictionaries into structs and we should pass those
> around.

Replaced by defined dictionary in the idl.

> > Source/WebCore/platform/mediastream/MediaEndpoint.h:82
> > +    virtual RefPtr<RealtimeMediaSource> createMutedRemoteSource(const String& mid, RealtimeMediaSource::Type) = 0;
> 
> Should return Ref, not RefPtr. Caller can’t handle nullptr without crashing.

True. Fixed.

> > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:52
> >  RealtimeMediaSource::RealtimeMediaSource(const String& id, Type type, const String& name)
> > -    : m_id(id)
> > +    : m_muted(false)
> > +    , m_id(id)
> >      , m_type(type)
> >      , m_name(name)
> >      , m_stopped(false)
> > -    , m_muted(false)
> >      , m_readonly(false)
> >      , m_remote(false)
> >      , m_fitnessScore(0)
> 
> Most data members should be initialized in the header, not in the
> constructor, except for the ones where we are initializing from passed in
> values.

Fixed. Refactored the rest in this init list as well.

> > Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:43
> >  RefPtr<MockRealtimeAudioSource> MockRealtimeAudioSource::create()
> 
> Return type should be Ref, not RefPtr.
> 
> > Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:48
> > +RefPtr<MockRealtimeAudioSource> MockRealtimeAudioSource::createMuted(const String& name)
> 
> Return type should be Ref, not RefPtr.
> 
> > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:52
> >  RefPtr<MockRealtimeVideoSource> MockRealtimeVideoSource::create()
> 
> Return type should be Ref, not RefPtr.
> 
> > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:57
> > +RefPtr<MockRealtimeVideoSource> MockRealtimeVideoSource::createMuted(const String& name)
> 
> Return type should be Ref, not RefPtr.
> 
> > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
> > +    RefPtr<MockRealtimeVideoSource> source = adoptRef(new MockRealtimeVideoSource(name));
> 
> Should be Ref, not RefPtr.
> 
> > Source/WebCore/platform/mock/MockRealtimeVideoSource.h:50
> >      static RefPtr<MockRealtimeVideoSource> create();
> > +    static RefPtr<MockRealtimeVideoSource> createMuted(const String& name);
> 
> Return type should be Ref, not RefPtr.

Fixed all s/RefPtr/Ref/ above.

-- 
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/a6820e69/attachment-0001.html>