<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#c3">Comment # 3</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>Comment on <span class=""><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
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>
<span class="quote">> LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:19
> + debug("This test can not be run without the testRunner");
> + finishJSTest();</span >
Nit: add this might as well return to avoid a bunch of script errors
<span class="quote">> LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:76
> + var mline;
> +
> + if (mline = match(line, regexp.mline)) {</span >
Nit: this local variable isn't used.
<span class="quote">> LayoutTests/fast/mediastream/RTCPeerConnection-inspect-offer.html:82
> + var msidsemantic;</span >
Ditto.
<span class="quote">> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:61
> + static Ref<WrappedSessionDescriptionPromise> create(SessionDescriptionPromise&& promise)
> + {</span >
Nit: this brace should be on the previous line.
<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);</span >
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?
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:384
> + if (!callScript("generate", configurationToJSON(configuration), sdpString))
> + return Result::InternalError;</span >
Nit: It could be helpful to log this error.
<span class="quote">> 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;</span >
Ditto.
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:411
> + if (!callScript("generateCandidateLine", iceCandidateToJSON(candidate), candidateLine))
> + return Result::InternalError;</span >
Ditto.
<span class="quote">> 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;</span >
Ditto.
<span class="quote">> 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;</span >
Ditto.
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:482
> + if (!result.isString())
> + return false;</span >
Ditto.</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>