<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Implement MediaEndpointPeerConnection::createOffer()"
href="https://bugs.webkit.org/show_bug.cgi?id=154867#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Implement MediaEndpointPeerConnection::createOffer()"
href="https://bugs.webkit.org/show_bug.cgi?id=154867">bug 154867</a>
from <span class="vcard"><a class="email" href="mailto:eric.carlson@apple.com" title="Eric Carlson <eric.carlson@apple.com>"> <span class="fn">Eric Carlson</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=154867#c5">comment #5</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=154867#c3">comment #3</a>)
> > Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=272580&action=diff" name="attach_272580" title="Proposed patch">attachment 272580</a> <a href="attachment.cgi?id=272580&action=edit" title="Proposed patch">[details]</a></span>
> > Proposed patch
>
> Thanks for reviewing. Find answers inline.
>
> > View in context:
> > <a href="https://bugs.webkit.org/attachment.cgi?id=272580&action=review">https://bugs.webkit.org/attachment.cgi?id=272580&action=review</a>
> >
> > > LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:19
> > > + debug("This test can not be run without the testRunner");
> > > + finishJSTest();
> >
> > Nit: add this might as well return to avoid a bunch of script errors
>
> That's pretty much the behavior right now since the getUserMedia() request
> will never resolve. We could wrap everything in a function and return as
> well.
> </span >
Indeed, I didn't look close enough!
<span class="quote">> > > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:158
> > > + mediaDescription->setMediaStreamId(sender->mediaStreamIds()[0]);
> > > + mediaDescription->setMediaStreamTrackId(track->id());
> > > + mediaDescription->setType(track->kind());
> > > + mediaDescription->setPort(9);
> > > + mediaDescription->setAddress("0.0.0.0");
> > > + mediaDescription->setMode("sendrecv");
> > > + mediaDescription->setPayloads(track->kind() == "audio" ? m_defaultAudioPayloads : m_defaultVideoPayloads);
> > > + mediaDescription->setRtcpMux(true);
> > > + mediaDescription->setDtlsSetup("actpass");
> > > + mediaDescription->setDtlsFingerprintHashFunction(m_dtlsFingerprintFunction);
> > > + mediaDescription->setDtlsFingerprint(m_dtlsFingerprint);
> > > + mediaDescription->setCname(m_cname);
> > > + mediaDescription->addSsrc(cryptographicallyRandomNumber());
> > > + mediaDescription->setIceUfrag(m_iceUfrag);
> > > + mediaDescription->setIcePassword(m_icePassword);
> >
> > Nit: This is a lot of setup Might it make sense to add a
> > PeerMediaDescription constructor that takes a MediaStreamTrack (or maybe
> > RtpSender)? If "0.0.0.0" the default address, can you set it in the
> > constructor?
>
> My intention with the setters was to avoid having expressions like
> 'cryptographicallyRandomNumber()' in the middle of a long constructor
> argument list since it's not obvious that it's an SSRC. But as it turned
> out, most of the values set are stored in variables with fairly descriptive
> names. :)
>
> I made all constant initializations here defaults when a
> PeerMediaDescription is constructed as you suggested with address. That got
> rid of five setter-calls. Is that enough or should we go for a constructor?
> </span >
This seems fine, thanks.
This seems fine to me, but this is a large & complex patch so I have asked someone else to take a look as well.</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>