[webkit-reviews] review granted: [Bug 129484] [WebRTC] Updating createOffer and createAnswer methods to match WebRTC editor's draft of 01/27/2014 : [Attachment 225468] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 28 11:26:45 PST 2014


Eric Carlson <eric.carlson at apple.com> has granted Thiago de Barros Lacerda
<thiago.lacerda at openbossa.org>'s request for review:
Bug 129484: [WebRTC] Updating createOffer and createAnswer methods to match
WebRTC editor's draft of 01/27/2014
https://bugs.webkit.org/show_bug.cgi?id=129484

Attachment 225468: Patch
https://bugs.webkit.org/attachment.cgi?id=225468&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225468&action=review


> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:41
> +	   // According to the spec, the error is going to be defined yet, so
let's use TYPE_MISMATCH_ERR for now.
> +	   ec = TYPE_MISMATCH_ERR;

This should probably have a FIXME with a bug number to help us remember to
change this once the error is defined.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:57
> +    if (validateRequestIdentity(requestIdentity))
> +	   m_requestIdentity = requestIdentity;
> +
> +    return true;

An invalid request identity does not result in an error?

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:64
> +bool RTCOfferAnswerOptions::validateRequestIdentity(const String& value)
const
> +{
> +    return value == "yes" || value == "no" || value == "ifconfigured";
> +}
> +

Unless this is going to be used outside of this class, it doesn't have to be a
member function.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:70
> +	   // According to the spec, the error is going to be defined yet, so
let's use TYPE_MISMATCH_ERR for now.
> +	   ec = TYPE_MISMATCH_ERR;

Ditto about adding a FIXME.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:85
> +    String offerToReceiveVideoStr, offerToReceiveAudioStr;

Nit: these declarations should be on separate lines.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:92
> +    if (options.get("offerToReceiveVideo", offerToReceiveVideoStr)) {
> +	   m_offerToReceiveVideo =
offerToReceiveVideoStr.toInt64Strict(&numberConversionSuccess);
> +	   if (!numberConversionSuccess)
> +	       return false;
> +    } else
> +	   return false;

Nit: I would reverse the order of the comparisons, return false if
"offerToReceiveVideo" isn't specified first, to avoid having to indent the
number conversion.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:99
> +    if (options.get("offerToReceiveAudio", offerToReceiveAudioStr)) {
> +	   m_offerToReceiveAudio =
offerToReceiveAudioStr.toInt64Strict(&numberConversionSuccess);
> +	   if (!numberConversionSuccess)
> +	       return false;
> +    } else
> +	   return false;

Ditto.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:102
> +    bool iceRestart;

Nit: this can be declared just before it is used.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:206
> +void
RTCPeerConnection::createOffer(PassRefPtr<RTCSessionDescriptionCallback>
successCallback, PassRefPtr<RTCPeerConnectionErrorCallback> errorCallback,
const Dictionary& offerOptions, ExceptionCode& ec)

It doesn't have to be done in this patch, but we should update all of the
MEDIA_STREAM code to use Ref<> and PassRef<> instead of RefPtr<> and
PassRefPtr<>.

> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67
> +void
RTCPeerConnectionHandlerMock::createAnswer(PassRefPtr<RTCSessionDescriptionRequ
est> request, PassRefPtr<RTCOfferAnswerOptions> constraints)

This will break the -Werror build because "constraints" is not used.


More information about the webkit-reviews mailing list