Created attachment 272580 [details] Proposed patch

Comment on attachment 272580 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=272580&action=review > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:476 > + printf("SDPProcessor::callScript(): %s() threw\n", functionName.ascii().data()); This should be a LOG_ERROR.

Comment on attachment 272580 [details] 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.

Created attachment 272685 [details] Updated patch

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

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

(In reply to comment #6) > This seems fine to me, but this is a large & complex patch so I have asked > someone else to take a look as well. Thanks. That seems reasonable.

Comment on attachment 272580 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=272580&action=review > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:74 > + candidateObject->setString(ASCIILiteral("type"), candidate.type()); > + candidateObject->setString(ASCIILiteral("foundation"), candidate.foundation()); > + candidateObject->setInteger(ASCIILiteral("componentId"), candidate.componentId()); > + candidateObject->setString(ASCIILiteral("transport"), candidate.transport()); > + candidateObject->setInteger(ASCIILiteral("priority"), candidate.priority()); > + candidateObject->setString(ASCIILiteral("address"), candidate.address()); > + candidateObject->setInteger(ASCIILiteral("port"), candidate.port()); > + if (!candidate.tcpType().isEmpty()) > + candidateObject->setString(ASCIILiteral("tcpType"), candidate.tcpType()); > + if (candidate.type().convertToASCIIUppercase() != "HOST") { > + candidateObject->setString(ASCIILiteral("relatedAddress"), candidate.relatedAddress()); > + candidateObject->setInteger(ASCIILiteral("relatedPort"), candidate.relatedPort()); > + } It seems like a shame to have to create so many temporary String objects just for the sake of passing through to the InspectorObject here (and later in createCandidate). Could you just use some static methods returning NeverDestroyed<String> objects? E.g.: static const String& typeString() { static NeverDestroyed<const String> type { ASCIILiteral("type") }; return type; } > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:86 > + if (candidateObject.getString(ASCIILiteral("type"), stringValue)) > + candidate->setType(stringValue); Ditto. > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:153 > + if (mdescObject->getString(ASCIILiteral("type"), stringValue)) > + mdesc->setType(stringValue); Ditto. > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:299 > + for (const RefPtr<PeerMediaDescription>& mdesc : configuration.mediaDescriptions()) { "mdesc" is not a very, ahem, descriptive name. "description"? > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:300 > + RefPtr<InspectorObject> mdescObject = InspectorObject::create(); Same for "mdescObject". > Source/WebCore/platform/mediastream/gtk/SDPProcessorScriptResourceGtk.cpp:45 > +String scriptString() > +{ > + return String(sdpJavaScript); > +} This should probably return a const String&, and store it locally in a "static NeverDestroyed<const String>".

Created attachment 273169 [details] Candidate for landing

Created attachment 273176 [details] Candidate for landing (rebased) Rebased and updated to match some new style rules.

(In reply to comment #8) > Comment on attachment 272580 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272580&action=review Thanks for taking time to review. Find answers inline. > > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:74 > > + candidateObject->setString(ASCIILiteral("type"), candidate.type()); > > + candidateObject->setString(ASCIILiteral("foundation"), candidate.foundation()); > > + candidateObject->setInteger(ASCIILiteral("componentId"), candidate.componentId()); > > + candidateObject->setString(ASCIILiteral("transport"), candidate.transport()); > > + candidateObject->setInteger(ASCIILiteral("priority"), candidate.priority()); > > + candidateObject->setString(ASCIILiteral("address"), candidate.address()); > > + candidateObject->setInteger(ASCIILiteral("port"), candidate.port()); > > + if (!candidate.tcpType().isEmpty()) > > + candidateObject->setString(ASCIILiteral("tcpType"), candidate.tcpType()); > > + if (candidate.type().convertToASCIIUppercase() != "HOST") { > > + candidateObject->setString(ASCIILiteral("relatedAddress"), candidate.relatedAddress()); > > + candidateObject->setInteger(ASCIILiteral("relatedPort"), candidate.relatedPort()); > > + } > > It seems like a shame to have to create so many temporary String objects > just for the sake of passing through to the InspectorObject here (and later > in createCandidate). Could you just use some static methods returning > NeverDestroyed<String> objects? E.g.: > > static const String& typeString() > { > static NeverDestroyed<const String> type { ASCIILiteral("type") }; > return type; > } Yes. I created simple a macro to define a bunch of these functions. > > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:86 > > + if (candidateObject.getString(ASCIILiteral("type"), stringValue)) > > + candidate->setType(stringValue); > > Ditto. > > > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:153 > > + if (mdescObject->getString(ASCIILiteral("type"), stringValue)) > > + mdesc->setType(stringValue); > > Ditto. > > > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:299 > > + for (const RefPtr<PeerMediaDescription>& mdesc : configuration.mediaDescriptions()) { > > "mdesc" is not a very, ahem, descriptive name. "description"? Fixed. Went with mediaDescription. > > Source/WebCore/Modules/mediastream/SDPProcessor.cpp:300 > > + RefPtr<InspectorObject> mdescObject = InspectorObject::create(); > > Same for "mdescObject". Fixed. Went with mediaDescriptionObject. > > Source/WebCore/platform/mediastream/gtk/SDPProcessorScriptResourceGtk.cpp:45 > > +String scriptString() > > +{ > > + return String(sdpJavaScript); > > +} > > This should probably return a const String&, and store it locally in a > "static NeverDestroyed<const String>". Indeed. Fixed.

Created attachment 273177 [details] Candidate for landing (rebased)

Comment on attachment 273177 [details] Candidate for landing (rebased) Clearing flags on attachment: 273177 Committed r197702: <http://trac.webkit.org/changeset/197702>