According to the spec, createOffer and createAnswer will no longer have MediaConstraints as an argument, instead they will have RTCOfferOptions and RTCOfferAnswerOptions, respectively.
Created attachment 225468 [details] Patch
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.
Created attachment 225984 [details] Requested Changes
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.
Comment on attachment 225984 [details] Requested Changes Clearing flags on attachment: 225984 Committed r165226: <http://trac.webkit.org/changeset/165226>
All reviewed patches have been landed. Closing bug.