<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#c5">Comment # 5</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:adam.bergkvist@ericsson.com" title="Adam Bergkvist <adam.bergkvist@ericsson.com>"> <span class="fn">Adam Bergkvist</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=158189#c3">comment #3</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><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</span >
Thanks for reviewing. See comments inline.
<span class="quote">> 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>
>
> > 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.</span >
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 :).
<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);
>
> 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 >
Using dictionary from latest published editor's draft [1]. Not all members are supported yet. Should I add a FIXME+bug# for that?
[1] <a href="https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtcrtptransceiverinit">https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtcrtptransceiverinit</a>
<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;
> > +}
>
> Since bindings create functions like this for all enumerations, we should
> avoid writing this code ourselves if possible. What makes this necessary?</span >
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?
<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;
> > +}
>
> 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 >
Replaced by defined dictionary in the idl.
<span class="quote">> > 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.</span >
True. Fixed.
<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)
>
> 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 >
Fixed. Refactored the rest in this init list as well.
<span class="quote">> > 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.</span >
Fixed all s/RefPtr/Ref/ above.</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>