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

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(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.

> > LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:76
> > +                    var mline;
> > +
> > +                    if (mline = match(line, regexp.mline)) {
> 
> Nit: this local variable isn't used.

Fixed.

> > LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:82
> > +                        var msidsemantic;
> 
> Ditto.

Fixed.

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:61
> > +    static Ref<WrappedSessionDescriptionPromise> create(SessionDescriptionPromise&& promise)
> > +    {
> 
> Nit: this brace should be on the previous line.

The style checker complains in that case.

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

> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:384
> > +    if (!callScript("generate", configurationToJSON(configuration), sdpString))
> > +        return Result::InternalError;
> 
> Nit: It could be helpful to log this error.

The error is currently propagated to the caller and logged there (e.g. MediaEndpointPeerConnection:161). We could log it here as well. 

> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:401
> > +    if (!callScript("parse", sdp, scriptOutput))
> > +        return Result::InternalError;
> > +
> > +    if (scriptOutput == "ParseError")
> > +        return Result::ParseError;
> > +
> > +    RefPtr<MediaEndpointSessionConfiguration> configuration = configurationFromJSON(scriptOutput);
> > +    if (!configuration)
> > +        return Result::InternalError;
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:411
> > +    if (!callScript("generateCandidateLine", iceCandidateToJSON(candidate), candidateLine))
> > +        return Result::InternalError;
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:428
> > +    if (!callScript("parseCandidateLine", candidateLine, scriptOutput))
> > +        return Result::InternalError;
> > +
> > +    if (scriptOutput == "ParseError")
> > +        return Result::ParseError;
> > +
> > +    RefPtr<IceCandidate> candidate = iceCandidateFromJSON(scriptOutput);
> > +    if (!candidate)
> > +        return Result::InternalError;
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:469
> > +        if (exec->hadException()) {
> > +            exec->clearException();
> > +            return false;
> > +        }
> > +    }
> > +
> > +    JSC::JSValue functionValue = globalObject->get(exec, JSC::Identifier::fromString(exec, functionName));
> > +    if (!functionValue.isFunction())
> > +        return false;
> > +
> > +    JSC::JSObject* function = functionValue.toObject(exec);
> > +    JSC::CallData callData;
> > +    JSC::CallType callType = function->methodTable()->getCallData(function, callData);
> > +    if (callType == JSC::CallTypeNone)
> > +        return false;
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:482
> > +    if (!result.isString())
> > +        return false;
> 
> Ditto.

-- 
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/20160302/c6a95907/attachment-0001.html>