[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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #280145|review?                     |review+
              Flags|                            |

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

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

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:195
> +    transceiver->setDirection(static_cast<RTCRtpTransceiver::Direction>(init.direction));

This typecast should not be needed. It’s currently only needed because we are defining the enum twice (see my other comments).

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:67
>      Vector<RefPtr<RTCRtpSender>> getSenders() const override { return m_senderSet; }
>      Vector<RefPtr<RTCRtpReceiver>> getReceivers() const { return m_receiverSet; }

It’s expensive to have functions that return an existing Vector by copying it. We should not do that unless we have to.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:78
> +    enum class RtpTransceiverDirection {
> +        Sendrecv,
> +        Sendonly,
> +        Recvonly,
> +        Inactive
> +    };

Instead of re-defining this here, we should write this:

    using RtpTransceiverDirection = RTCRtpTransceiver::Direction;

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:82
> +    struct RtpTransceiverInit {
> +        RtpTransceiverDirection direction;
> +    };

Eventually, we probably also want to define this in RTCRtpTransceiver and use "using" to pull it in here with the appropriate name.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:125
> +enum RTCRtpTransceiverDirection {
> +    "sendrecv",
> +    "sendonly",
> +    "recvonly",
> +    "inactive"
> +};

Eventually we will make the bindings script smarter so we don’t have to repeat this in multiple IDL files. In the mean time we might want to comment so anyone modifying one knows they should modify the other.

> Source/WebCore/Modules/mediastream/RTCRtpSender.h:60
> +    bool isStopped() const { return m_client == nullptr; }

WebKit coding style says we write this !m_client rather than m_client == nullptr.

> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:80
> +    return emptyString();

Maybe return inactiveString() here instead of emptyString()?

> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.h:52
> +    enum class Direction {
> +        Sendrecv,
> +        Sendonly,
> +        Recvonly,
> +        Inactive
> +    };

I find short enumerations like this really nice when writing out all on one line.

> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:50
> +    Ref<MockRealtimeAudioSource> source = adoptRef(*new MockRealtimeAudioSource(name));

I think auto works better here than writing the type a second time. I’d also omit the empty line below.

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
> +    Ref<MockRealtimeVideoSource> source = adoptRef(*new MockRealtimeVideoSource(name));

I think auto works better here than writing the type a second time. I’d also omit the empty line below.

-- 
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/9c81c842/attachment.html>