Bug 164680 – Refresh RTCDataChannel abstract infrastructure

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

[youenn fablet]

To prepare for RTCDataChannel implementation, let's refresh what is there.

[youenn fablet]

Created attachment 294614 [details]
Patch

[youenn fablet]

Created attachment 294618 [details]
Patch

[WebKit Commit Bot]

Attachment 294618 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/MediaEndpoint.cpp:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 294619 [details]
Patch

[WebKit Commit Bot]

Attachment 294619 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/MediaEndpoint.cpp:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 294619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294619&action=review

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:80
> +    std::unique_ptr<RTCDataChannelHandler> createDataChannelHandler(const String&, const RTCDataChannelInit&) final;

Can this be private? Does it ever get called non-polymorphically on this class rather than the base class?

> Source/WebCore/Modules/mediastream/RTCDataChannel.h:50
> +    static Ref<RTCDataChannel> create(ScriptExecutionContext*, std::unique_ptr<RTCDataChannelHandler>&&, String&&, RTCDataChannelInit&&);

Can this take a ScriptExecutionContext& instead?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:51
> +dictionary RTCDataChannelInit {
> +    boolean        ordered = true;
> +    unsigned short maxRetransmitTime;
> +    unsigned short maxRetransmits;
> +    USVString      protocol = "";
> +    boolean        negotiated = false;
> +    unsigned short id;
> +};

WebKit coding style calls for *not* lining up in columns like this

> Source/WebCore/platform/mediastream/MediaEndpoint.cpp:57
>      EmptyMediaEndpoint(MediaEndpointClient&) { }

I don’t understand why this constructor needs this ignored argument. I couldn’t fid the code that calls this; is it somehow shared or a template that works with more than one class?

Also, if we are keeping it like this, it should be marked explicit since it has exactly one argument but should not be used for type conversion.

[youenn fablet]

Created attachment 294711 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #6)
> Comment on attachment 294619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294619&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:80
> > +    std::unique_ptr<RTCDataChannelHandler> createDataChannelHandler(const String&, const RTCDataChannelInit&) final;
> 
> Can this be private? Does it ever get called non-polymorphically on this
> class rather than the base class?

Right

> > Source/WebCore/Modules/mediastream/RTCDataChannel.h:50
> > +    static Ref<RTCDataChannel> create(ScriptExecutionContext*, std::unique_ptr<RTCDataChannelHandler>&&, String&&, RTCDataChannelInit&&);
> 
> Can this take a ScriptExecutionContext& instead?

Changed the IDL to pass a ScriptExecutionContext& directly.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:51
> > +dictionary RTCDataChannelInit {
> > +    boolean        ordered = true;
> > +    unsigned short maxRetransmitTime;
> > +    unsigned short maxRetransmits;
> > +    USVString      protocol = "";
> > +    boolean        negotiated = false;
> > +    unsigned short id;
> > +};
> 
> WebKit coding style calls for *not* lining up in columns like this

OK

> > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:57
> >      EmptyMediaEndpoint(MediaEndpointClient&) { }
> 
> I don’t understand why this constructor needs this ignored argument. I
> couldn’t fid the code that calls this; is it somehow shared or a template
> that works with more than one class?
> 
> Also, if we are keeping it like this, it should be marked explicit since it
> has exactly one argument but should not be used for type conversion.

It's used by a function pointer who has this parameter.
But the function could just not pass it to the constructor.
Updated patch accordingly.

[WebKit Commit Bot]

Comment on attachment 294711 [details]
Patch for landing

Clearing flags on attachment: 294711

Committed r208694: <http://trac.webkit.org/changeset/208694>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.