<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&#64;ericsson.com" title="Adam Bergkvist &lt;adam.bergkvist&#64;ericsson.com&gt;"> <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">&gt; Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=280093&amp;action=diff" name="attach_280093" title="Proposed patch">attachment 280093</a> <a href="attachment.cgi?id=280093&amp;action=edit" title="Proposed patch">[details]</a></span>
&gt; Proposed patch</span >

Thanks for reviewing. See comments inline.

<span class="quote">&gt; View in context:
&gt; <a href="https://bugs.webkit.org/attachment.cgi?id=280093&amp;action=review">https://bugs.webkit.org/attachment.cgi?id=280093&amp;action=review</a>
&gt; 
&gt; &gt; Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:265
&gt; &gt; +    RefPtr&lt;RealtimeMediaSource&gt; remoteSource = m_mediaEndpoint-&gt;createMutedRemoteSource(transceiverMid, sourceType);
&gt; &gt; +    RefPtr&lt;MediaStreamTrackPrivate&gt; remoteTrackPrivate = MediaStreamTrackPrivate::create(WTFMove(remoteSource), trackId);
&gt; &gt; +    Ref&lt;MediaStreamTrack&gt; remoteTrack = MediaStreamTrack::create(*m_client-&gt;scriptExecutionContext(), *remoteTrackPrivate);
&gt; 
&gt; Types should all be auto.
&gt; 
&gt; &gt; Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:164
&gt; &gt; +    RefPtr&lt;RTCRtpSender&gt; sender = RTCRtpSender::create(WTFMove(track), Vector&lt;String&gt;(), *this);
&gt; &gt; +    RefPtr&lt;RTCRtpReceiver&gt; receiver = m_backend-&gt;createReceiver(transceiverMid, trackKind, trackId);
&gt; &gt; +    Ref&lt;RTCRtpTransceiver&gt; transceiver = RTCRtpTransceiver::create(WTFMove(sender), WTFMove(receiver));
&gt; 
&gt; Types should just be auto.
&gt; 
&gt; &gt; Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:187
&gt; &gt; +    RefPtr&lt;RTCRtpSender&gt; sender = RTCRtpSender::create(kind, Vector&lt;String&gt;(), *this);
&gt; &gt; +    RefPtr&lt;RTCRtpReceiver&gt; receiver = m_backend-&gt;createReceiver(transceiverMid, kind, trackId);
&gt; &gt; +    Ref&lt;RTCRtpTransceiver&gt; transceiver = RTCRtpTransceiver::create(WTFMove(sender), WTFMove(receiver));
&gt; 
&gt; 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">&gt; &gt; Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:69
&gt; &gt; +    [StrictTypeChecking, RaisesException] RTCRtpTransceiver addTransceiver(MediaStreamTrack track, optional Dictionary init);
&gt; &gt; +    [StrictTypeChecking, RaisesException] RTCRtpTransceiver addTransceiver(DOMString kind, optional Dictionary init);
&gt; 
&gt; New code should never use Dictionary. These dictionaries should defined.
&gt; 
&gt; dictionary RTCRtpTransceiverInit {
&gt;     boolean send = true;
&gt;     boolean receive = true;
&gt;     sequence&lt;MediaStream&gt; streams;
&gt;     sequence&lt;RTCRtpEncodingParameters&gt; sendEncodings;
&gt; };
&gt; 
&gt; If the bindings generator can’t handle this dictionary, we should fix it so
&gt; it can handle it. Every single time we instead use the deprecated
&gt; 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">&gt; &gt; Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:98
&gt; &gt; +const String&amp; RTCRtpTransceiver::directionString() const
&gt; &gt; +{
&gt; &gt; +    switch (m_direction) {
&gt; &gt; +    case Direction::Sendrecv: return sendrecvString();
&gt; &gt; +    case Direction::Sendonly: return sendonlyString();
&gt; &gt; +    case Direction::Recvonly: return recvonlyString();
&gt; &gt; +    case Direction::Inactive: return inactiveString();
&gt; &gt; +    }
&gt; &gt; +
&gt; &gt; +    ASSERT_NOT_REACHED();
&gt; &gt; +    return emptyString();
&gt; &gt; +}
&gt; &gt; +
&gt; &gt; +static bool parseDirectionString(const String&amp; string, RTCRtpTransceiver::Direction&amp; outDirection)
&gt; &gt; +{
&gt; &gt; +    if (string == sendrecvString())
&gt; &gt; +        outDirection = RTCRtpTransceiver::Direction::Sendrecv;
&gt; &gt; +    else if (string == sendonlyString())
&gt; &gt; +        outDirection = RTCRtpTransceiver::Direction::Sendonly;
&gt; &gt; +    else if (string == recvonlyString())
&gt; &gt; +        outDirection = RTCRtpTransceiver::Direction::Recvonly;
&gt; &gt; +    else if (string == inactiveString())
&gt; &gt; +        outDirection = RTCRtpTransceiver::Direction::Inactive;
&gt; &gt; +    else
&gt; &gt; +        return false;
&gt; &gt; +
&gt; &gt; +    return true;
&gt; &gt; +}
&gt; 
&gt; Since bindings create functions like this for all enumerations, we should
&gt; 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">&gt; &gt; Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:110
&gt; &gt; +bool RTCRtpTransceiver::configureWithDictionary(const Dictionary&amp; dictionary)
&gt; &gt; +{
&gt; &gt; +    String direction;
&gt; &gt; +    if (dictionary.get(&quot;direction&quot;, direction)) {
&gt; &gt; +        if (!parseDirectionString(direction, m_direction))
&gt; &gt; +            return false;
&gt; &gt; +    }
&gt; &gt; +
&gt; &gt; +    // FIMXE: fix streams
&gt; &gt; +    return true;
&gt; &gt; +}
&gt; 
&gt; Since bindings create functions like this for dictionaries, we should avoid
&gt; writing this code ourselves if possible. What makes this necessary? The
&gt; bindings should convert dictionaries into structs and we should pass those
&gt; around.</span >

Replaced by defined dictionary in the idl.

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

True. Fixed.

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

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

<span class="quote">&gt; &gt; Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:43
&gt; &gt;  RefPtr&lt;MockRealtimeAudioSource&gt; MockRealtimeAudioSource::create()
&gt; 
&gt; Return type should be Ref, not RefPtr.
&gt; 
&gt; &gt; Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:48
&gt; &gt; +RefPtr&lt;MockRealtimeAudioSource&gt; MockRealtimeAudioSource::createMuted(const String&amp; name)
&gt; 
&gt; Return type should be Ref, not RefPtr.
&gt; 
&gt; &gt; Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:52
&gt; &gt;  RefPtr&lt;MockRealtimeVideoSource&gt; MockRealtimeVideoSource::create()
&gt; 
&gt; Return type should be Ref, not RefPtr.
&gt; 
&gt; &gt; Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:57
&gt; &gt; +RefPtr&lt;MockRealtimeVideoSource&gt; MockRealtimeVideoSource::createMuted(const String&amp; name)
&gt; 
&gt; Return type should be Ref, not RefPtr.
&gt; 
&gt; &gt; Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
&gt; &gt; +    RefPtr&lt;MockRealtimeVideoSource&gt; source = adoptRef(new MockRealtimeVideoSource(name));
&gt; 
&gt; Should be Ref, not RefPtr.
&gt; 
&gt; &gt; Source/WebCore/platform/mock/MockRealtimeVideoSource.h:50
&gt; &gt;      static RefPtr&lt;MockRealtimeVideoSource&gt; create();
&gt; &gt; +    static RefPtr&lt;MockRealtimeVideoSource&gt; createMuted(const String&amp; name);
&gt; 
&gt; 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>