Bug 124369

Summary: Modifying RTCIceCandidate object construction to match the spec
Product: WebKit Reporter: Thiago de Barros Lacerda <thiago.lacerda>
Component: WebCore Misc.Assignee: Thiago de Barros Lacerda <thiago.lacerda>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, hta, jer.noble, kondapallykalyan, pnormand, rakuco, syoichi, tommyw
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124288    
Attachments:
Description Flags
Patch
none
Patch
none
xcode changes
none
Patch none

Thiago de Barros Lacerda
Reported 2013-11-14 09:59:30 PST
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.
Attachments
Patch (18.21 KB, patch)
2013-11-14 10:32 PST, Thiago de Barros Lacerda 		no flags
DetailsFormatted DiffDiff
Patch (18.18 KB, patch)
2013-11-14 15:49 PST, Thiago de Barros Lacerda 		no flags
DetailsFormatted DiffDiff
xcode changes (3.56 KB, patch)
2013-11-14 23:46 PST, Philippe Normand 		no flags
DetailsFormatted DiffDiff
Patch (21.91 KB, patch)
2013-11-15 10:54 PST, Thiago de Barros Lacerda 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Thiago de Barros Lacerda
Comment 1 2013-11-14 10:32:49 PST
Created attachment 216958 [details] Patch
Eric Carlson
Comment 2 2013-11-14 15:24:10 PST
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
Comment 3 2013-11-14 15:49:13 PST
(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
Comment 4 2013-11-14 15:49:43 PST
Created attachment 216990 [details] Patch
Philippe Normand
Comment 5 2013-11-14 23:46:40 PST
Created attachment 217018 [details] xcode changes
Eric Carlson
Comment 6 2013-11-15 10:02:27 PST
(In reply to comment #5) > Created an attachment (id=217018) [details] > xcode changes Thanks Philippe!
Thiago de Barros Lacerda
Comment 7 2013-11-15 10:54:40 PST
Created attachment 217065 [details] Patch
Eric Carlson
Comment 8 2013-11-15 11:05:54 PST
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
Comment 9 2013-11-15 11:07:21 PST
(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 10 2013-11-15 11:32:03 PST
Comment on attachment 217065 [details] Patch Clearing flags on attachment: 217065 Committed r159349: <http://trac.webkit.org/changeset/159349>
WebKit Commit Bot
Comment 11 2013-11-15 11:32:06 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.