RTCOfferAnswerOptions and RTCOfferOptions objects were being passed to platform class, causing a layer violation.
Created attachment 227675 [details] Patch
Comment on attachment 227675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227675&action=review > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:78 > + RefPtr<RTCOfferOptionsPrivate> optionsPrivate = RTCOfferOptionsPrivate::create(); > + m_private = optionsPrivate; Nit: is the local variable necessary? > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:54 > -private: > - String m_requestIdentity; > + RefPtr<RTCOfferAnswerOptionsPrivate> m_private; This should remain private. > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:64 > + int64_t offerToReceiveVideo() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->offerToReceiveVideo(); } > + int64_t offerToReceiveAudio() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->offerToReceiveAudio(); } > + bool voiceActivityDetection() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->voiceActivityDetection(); } > + bool iceRestart() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->iceRestart(); } Are these static_casts necessary? If so, please put the accessor privateOfferOptions() in the base class and use it. > Source/WebCore/platform/mediastream/RTCOfferAnswerOptionsPrivate.h:53 > + return; This should be indented.
Comment on attachment 227675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227675&action=review >> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:78 >> + m_private = optionsPrivate; > > Nit: is the local variable necessary? It is just to avoid an static_cast to access RTCOfferOptionsPrivate methods (since m_private is an instance of the base class RTCOfferAnswerOptionsPrivate). >> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:64 >> + bool iceRestart() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->iceRestart(); } > > Are these static_casts necessary? If so, please put the accessor privateOfferOptions() in the base class and use it. I don't think that the base class should no about anyone that derives from it. Putting privateOfferOptions in the base class seems like to add extra information on the base class that it should not know. And yes, these casts are necessary, m_private is declared as the base class. >> Source/WebCore/platform/mediastream/RTCOfferAnswerOptionsPrivate.h:53 >> + return; > > This should be indented. oops
Comment on attachment 227675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227675&action=review >>> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:64 >>> + bool iceRestart() const { return static_cast<RTCOfferOptionsPrivate*>(m_private.get())->iceRestart(); } >> >> Are these static_casts necessary? If so, please put the accessor privateOfferOptions() in the base class and use it. > > I don't think that the base class should no about anyone that derives from it. Putting privateOfferOptions in the base class seems like to add extra information on the base class that it should not know. > And yes, these casts are necessary, m_private is declared as the base class. Then I suggest having an privateOfferAnswer() in the base class, use that in privateOfferOptions() here, and use privateOfferOptions in these methods.
Created attachment 227889 [details] Requested changes
Comment on attachment 227889 [details] Requested changes Clearing flags on attachment: 227889 Committed r166325: <http://trac.webkit.org/changeset/166325>