<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#c8">Comment # 8</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:jer.noble@apple.com" title="Jer Noble <jer.noble@apple.com>"> <span class="fn">Jer Noble</span></a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><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">> 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());
> + }</span >
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;
}
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:86
> + if (candidateObject.getString(ASCIILiteral("type"), stringValue))
> + candidate->setType(stringValue);</span >
Ditto.
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:153
> + if (mdescObject->getString(ASCIILiteral("type"), stringValue))
> + mdesc->setType(stringValue);</span >
Ditto.
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:299
> + for (const RefPtr<PeerMediaDescription>& mdesc : configuration.mediaDescriptions()) {</span >
"mdesc" is not a very, ahem, descriptive name. "description"?
<span class="quote">> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:300
> + RefPtr<InspectorObject> mdescObject = InspectorObject::create();</span >
Same for "mdescObject".
<span class="quote">> Source/WebCore/platform/mediastream/gtk/SDPProcessorScriptResourceGtk.cpp:45
> +String scriptString()
> +{
> + return String(sdpJavaScript);
> +}</span >
This should probably return a const String&, and store it locally in a "static NeverDestroyed<const String>".</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>