Bug 124369 – Modifying RTCIceCandidate object construction to match the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

According to the spec the RTCIceCandidateInit parameter in RTCSessionDescription constructor is optional, which must not be nullable, and, if passed, must be a valid Dictionary.
If the keys are not present, the string object that stores them in the RTCIceCandidate class, must be null in those cases.
Also, if a key is present and its value is not valid an exception must be raised.

[Thiago de Barros Lacerda]

Created attachment 216958 [details]
Patch

[Eric Carlson]

Comment on attachment 216958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216958&action=review

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:67
> +    ok = dictionary.get("sdpMLineIndex", tempLineIndex);
> +    // Then we try to convert it to a number and check if it was successful.
> +    unsigned short sdpMLineIndex = tempLineIndex.toUIntStrict(&intConversionOk);
> +    if (ok && !intConversionOk) {

Nit: there isn't any reason to attempt the conversion to int if the key doesn't exist.

[Thiago de Barros Lacerda]

(In reply to comment #2)
> (From update of attachment 216958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216958&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:67
> > +    ok = dictionary.get("sdpMLineIndex", tempLineIndex);
> > +    // Then we try to convert it to a number and check if it was successful.
> > +    unsigned short sdpMLineIndex = tempLineIndex.toUIntStrict(&intConversionOk);
> > +    if (ok && !intConversionOk) {
> 
> Nit: there isn't any reason to attempt the conversion to int if the key doesn't exist.

True story. Could you please also send me the xcode files patch?

[Thiago de Barros Lacerda]

Created attachment 216990 [details]
Patch

[Philippe Normand]

Created attachment 217018 [details]
xcode changes

[Eric Carlson]

(In reply to comment #5)
> Created an attachment (id=217018) [details]
> xcode changes

Thanks Philippe!

[Thiago de Barros Lacerda]

Created attachment 217065 [details]
Patch

[Eric Carlson]

Comment on attachment 217065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217065&action=review

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:48
>      bool ok = dictionary.get("candidate", candidate);

Nit: in a follow-up patch (or the next time you are in this file) it would be good to use statics for this and the other string constants in this file to avoid allocating a String for each every time this is called.

[Thiago de Barros Lacerda]

(In reply to comment #8)
> (From update of attachment 217065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217065&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:48
> >      bool ok = dictionary.get("candidate", candidate);
> 
> Nit: in a follow-up patch (or the next time you are in this file) it would be good to use statics for this and the other string constants in this file to avoid allocating a String for each every time this is called.

OK :)

[WebKit Commit Bot]

Comment on attachment 217065 [details]
Patch

Clearing flags on attachment: 217065

Committed r159349: <http://trac.webkit.org/changeset/159349>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.