Bug 130689 – Add platform implementation for RTCOfferAnswerOptions and RTCOfferOptions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

RTCOfferAnswerOptions and RTCOfferOptions objects were being passed to platform class, causing a layer violation.

[Thiago de Barros Lacerda]

Created attachment 227675 [details]
Patch

[Eric Carlson]

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.

[Thiago de Barros Lacerda]

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

[Eric Carlson]

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.

[Thiago de Barros Lacerda]

Created attachment 227889 [details]
Requested changes

[WebKit Commit Bot]

Comment on attachment 227889 [details]
Requested changes

Clearing flags on attachment: 227889

Committed r166325: <http://trac.webkit.org/changeset/166325>