Bug 124369 - Modifying RTCIceCandidate object construction to match the spec
Summary: Modifying RTCIceCandidate object construction to match the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords:
Depends on:
Blocks: 124288
  Show dependency treegraph
 
Reported: 2013-11-14 09:59 PST by Thiago de Barros Lacerda
Modified: 2013-11-15 11:32 PST (History)
13 users (show)

See Also:


Attachments
Patch (18.21 KB, patch)
2013-11-14 10:32 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (18.18 KB, patch)
2013-11-14 15:49 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
xcode changes (3.56 KB, patch)
2013-11-14 23:46 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (21.91 KB, patch)
2013-11-15 10:54 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 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.
Comment 1 Thiago de Barros Lacerda 2013-11-14 10:32:49 PST
Created attachment 216958 [details]
Patch
Comment 2 Eric Carlson 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.
Comment 3 Thiago de Barros Lacerda 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?
Comment 4 Thiago de Barros Lacerda 2013-11-14 15:49:43 PST
Created attachment 216990 [details]
Patch
Comment 5 Philippe Normand 2013-11-14 23:46:40 PST
Created attachment 217018 [details]
xcode changes
Comment 6 Eric Carlson 2013-11-15 10:02:27 PST
(In reply to comment #5)
> Created an attachment (id=217018) [details]
> xcode changes

Thanks Philippe!
Comment 7 Thiago de Barros Lacerda 2013-11-15 10:54:40 PST
Created attachment 217065 [details]
Patch
Comment 8 Eric Carlson 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.
Comment 9 Thiago de Barros Lacerda 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 :)
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2013-11-15 11:32:06 PST
All reviewed patches have been landed.  Closing bug.