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

[bugzilla-daemon at webkit.org]

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

--- Comment #8 from Jer Noble <jer.noble 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

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

-- 
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/20160304/254f33dd/attachment.html>