[Webkit-unassigned] [Bug 154867] WebRTC: Implement MediaEndpointPeerConnection::createOffer()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=154867

--- Comment #6 from Eric Carlson <eric.carlson at apple.com> ---
(In reply to comment #5)
> (In reply to comment #3)
> > Comment on attachment 272580 [details]
> > Proposed patch
> 
> Thanks for reviewing. Find answers inline.
>  
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=272580&action=review
> > 
> > > 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.
> 

Indeed, I didn't look close enough!

> > > 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?
> 

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.

-- 
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/20160303/bfa20e60/attachment-0001.html>