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
Thiago de Barros Lacerda
2013-11-14 09:59:30 PST
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. |