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

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 272580
  --> https://bugs.webkit.org/attachment.cgi?id=272580
Proposed patch

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

> LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:76
> +                    var mline;
> +
> +                    if (mline = match(line, regexp.mline)) {

Nit: this local variable isn't used.

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

Ditto.

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:61
> +    static Ref<WrappedSessionDescriptionPromise> create(SessionDescriptionPromise&& promise)
> +    {

Nit: this brace should be on the previous line.

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

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

> 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/68c0c961/attachment.html>