WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
Summary
[WebRTC] Updating createOffer and createAnswer methods to match WebRTC editor...
Thiago de Barros Lacerda
Reported
2014-02-28 08:15:41 PST
According to the spec, createOffer and createAnswer will no longer have MediaConstraints as an argument, instead they will have RTCOfferOptions and RTCOfferAnswerOptions, respectively.
Attachments
Patch
(34.77 KB, patch)
2014-02-28 08:22 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Requested Changes
(34.81 KB, patch)
2014-03-06 06:20 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Thiago de Barros Lacerda
Comment 1
2014-02-28 08:22:39 PST
Created
attachment 225468
[details]
Patch
Eric Carlson
Comment 2
2014-02-28 11:23:51 PST
Comment on
attachment 225468
[details]
Patch 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<RTCSessionDescriptionRequest> request, PassRefPtr<RTCOfferAnswerOptions> constraints)
This will break the -Werror build because "constraints" is not used.
Thiago de Barros Lacerda
Comment 3
2014-03-06 06:20:01 PST
Created
attachment 225984
[details]
Requested Changes
Thiago de Barros Lacerda
Comment 4
2014-03-06 06:25:32 PST
Comment on
attachment 225468
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=225468&action=review
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:41 >> + 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.
OK
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:57 >> + return true; > > An invalid request identity does not result in an error?
Spec don't says anything about it and requestIdentity has a default value ("ifconfigured"), so I thought that would be better to just ignore an invalid value and stay with the default one.
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:64 >> + > > Unless this is going to be used outside of this class, it doesn't have to be a member function.
OK, I'll put it as static
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:70 >> + ec = TYPE_MISMATCH_ERR; > > Ditto about adding a FIXME.
OK
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:85 >> + String offerToReceiveVideoStr, offerToReceiveAudioStr; > > Nit: these declarations should be on separate lines.
OK
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:92 >> + 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.
OK
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:99 >> + return false; > > Ditto.
OK
>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:102 >> + bool iceRestart; > > Nit: this can be declared just before it is used.
OK
>> 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<>.
I can do that later
>> Source/WebCore/platform/mock/RTCPeerConnectionHandlerMock.cpp:67 >> +void RTCPeerConnectionHandlerMock::createAnswer(PassRefPtr<RTCSessionDescriptionRequest> request, PassRefPtr<RTCOfferAnswerOptions> constraints) > > This will break the -Werror build because "constraints" is not used.
oops.
WebKit Commit Bot
Comment 5
2014-03-06 15:35:40 PST
Comment on
attachment 225984
[details]
Requested Changes Clearing flags on attachment: 225984 Committed
r165226
: <
http://trac.webkit.org/changeset/165226
>
WebKit Commit Bot
Comment 6
2014-03-06 15:35:44 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.
Top of Page
Format For Printing
XML
Clone This Bug