<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189">bug 158189</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=280093&action=diff" name="attach_280093" title="Proposed patch">attachment 280093</a> <a href="attachment.cgi?id=280093&action=edit" title="Proposed patch">[details]</a></span>
Proposed patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280093&action=review">https://bugs.webkit.org/attachment.cgi?id=280093&action=review</a>
<span class="quote">> 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);</span >
Types should all be auto.
<span class="quote">> 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));</span >
Types should just be auto.
<span class="quote">> 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));</span >
Types should just be auto.
<span class="quote">> 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);</span >
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!
<span class="quote">> 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;
> +}</span >
Since bindings create functions like this for all enumerations, we should avoid writing this code ourselves if possible. What makes this necessary?
<span class="quote">> 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;
> +}</span >
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.
<span class="quote">> Source/WebCore/platform/mediastream/MediaEndpoint.h:82
> + virtual RefPtr<RealtimeMediaSource> createMutedRemoteSource(const String& mid, RealtimeMediaSource::Type) = 0;</span >
Should return Ref, not RefPtr. Caller can’t handle nullptr without crashing.
<span class="quote">> 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)</span >
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.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:43
> RefPtr<MockRealtimeAudioSource> MockRealtimeAudioSource::create()</span >
Return type should be Ref, not RefPtr.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:48
> +RefPtr<MockRealtimeAudioSource> MockRealtimeAudioSource::createMuted(const String& name)</span >
Return type should be Ref, not RefPtr.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:52
> RefPtr<MockRealtimeVideoSource> MockRealtimeVideoSource::create()</span >
Return type should be Ref, not RefPtr.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:57
> +RefPtr<MockRealtimeVideoSource> MockRealtimeVideoSource::createMuted(const String& name)</span >
Return type should be Ref, not RefPtr.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
> + RefPtr<MockRealtimeVideoSource> source = adoptRef(new MockRealtimeVideoSource(name));</span >
Should be Ref, not RefPtr.
<span class="quote">> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:50
> static RefPtr<MockRealtimeVideoSource> create();
> + static RefPtr<MockRealtimeVideoSource> createMuted(const String& name);</span >
Return type should be Ref, not RefPtr.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>