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

Mon May 30 21:07:58 PDT 2016

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

--- Comment #3 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

> 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.

> 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!

> 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?

> 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.

> 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.

> 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.

> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:43
>  RefPtr<MockRealtimeAudioSource> MockRealtimeAudioSource::create()