Bug 130689

Summary: Add platform implementation for RTCOfferAnswerOptions and RTCOfferOptions
Product: WebKit Reporter: Thiago de Barros Lacerda <thiago.lacerda>
Component: WebCore Misc.Assignee: Thiago de Barros Lacerda <thiago.lacerda>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, glenn, hta, jer.noble, philipj, sergio, tommyw
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124288    
Attachments:
Description Flags
Patch
eric.carlson: review+
Requested changes none

Description Thiago de Barros Lacerda 2014-03-24 11:44:25 PDT
RTCOfferAnswerOptions and RTCOfferOptions objects were being passed to platform class, causing a layer violation.
Comment 1 Thiago de Barros Lacerda 2014-03-24 12:02:30 PDT
Created attachment 227675 [details]
Patch
Comment 2 Eric Carlson 2014-03-24 14:58:44 PDT
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 3 Thiago de Barros Lacerda 2014-03-24 16:31:58 PDT
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 4 Eric Carlson 2014-03-25 07:42:11 PDT
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.
Comment 5 Thiago de Barros Lacerda 2014-03-26 15:02:41 PDT
Created attachment 227889 [details]
Requested changes
Comment 6 WebKit Commit Bot 2014-03-26 16:27:15 PDT
Comment on attachment 227889 [details]
Requested changes

Clearing flags on attachment: 227889

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