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.
Created attachment 216958 [details] Patch
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.
(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?
Created attachment 216990 [details] Patch
Created attachment 217018 [details] xcode changes
(In reply to comment #5) > Created an attachment (id=217018) [details] > xcode changes Thanks Philippe!
Created attachment 217065 [details] Patch
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.
(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 :)
Comment on attachment 217065 [details] Patch Clearing flags on attachment: 217065 Committed r159349: <http://trac.webkit.org/changeset/159349>
All reviewed patches have been landed. Closing bug.