Bug 99080 – MediaStream API: Implement RTCDataChannel

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Introduce the WebCore implementation of RTCDataChannel according to <http://dev.w3.org/2011/webrtc/editor/archives/20120920/webrtc.html#idl-def-DataChannel>.

[Tommy Widenflycht]

Created attachment 168241 [details]
Patch

[Tommy Widenflycht]

If the patch is on the large side I can split it, but I wanted to give the full picture first.

[Tommy Widenflycht]

Created attachment 168244 [details]
Patch

[Adam Barth]

Comment on attachment 168244 [details]
Patch

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

This patch could use another iteration, but my comments are mostly just naming nits.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:42
> +PassRefPtr<RTCDataChannel> RTCDataChannel::create(ScriptExecutionContext* context, RTCPeerConnectionHandler* handler, String label, bool reliable, ExceptionCode& ec)

String -> const String&

> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.idl:30
> +       readonly attribute RTCDataChannel channel;

Bad indent

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:413
> +PassRefPtr<RTCDataChannel> RTCPeerConnection::createDataChannel(String label, const Dictionary& dataChannelDict, ExceptionCode& ec)

dataChannelDict -> options?

> Source/WebCore/platform/mediastream/RTCDataChannelDescriptor.h:38
> +    class Owner {

Typically we'd call this a client rather than an owner.

Also, we'd usually declare it as a top-level class "RTCDataChannelClient"

It's confusing for a RefCounted object to have an "owner", which is why we tend not to use that term.

[Tommy Widenflycht]

Comment on attachment 168244 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:42
>> +PassRefPtr<RTCDataChannel> RTCDataChannel::create(ScriptExecutionContext* context, RTCPeerConnectionHandler* handler, String label, bool reliable, ExceptionCode& ec)
> 
> String -> const String&

Fixed.

>> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.idl:30
>> +       readonly attribute RTCDataChannel channel;
> 
> Bad indent

Fixed.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:413
>> +PassRefPtr<RTCDataChannel> RTCPeerConnection::createDataChannel(String label, const Dictionary& dataChannelDict, ExceptionCode& ec)
> 
> dataChannelDict -> options?

Fixed. dataChannelDict is used in the specification but options is much better.

>> Source/WebCore/platform/mediastream/RTCDataChannelDescriptor.h:38
>> +    class Owner {
> 
> Typically we'd call this a client rather than an owner.
> 
> Also, we'd usually declare it as a top-level class "RTCDataChannelClient"
> 
> It's confusing for a RefCounted object to have an "owner", which is why we tend not to use that term.

Agree with that, I just reused the name from MediaStreamDescriptor. Do you want me to rename owner to client there as well in an followup patch?

[Tommy Widenflycht]

Created attachment 168386 [details]
Patch

[Adam Barth]

Comment on attachment 168386 [details]
Patch

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

This looks great.  We just need to make sure it still compiles w.r.t. the IDL parser change last night.

> Source/WebCore/Modules/mediastream/RTCDataChannel.idl:28
> +interface [
> +    Conditional=MEDIA_STREAM,
> +    EventTarget
> +] RTCDataChannel {

We're working on making WebKitIDL more like WebIDL, so we might need to move these attributes before the keyword "interface"

[Adam Barth]

> I just reused the name from MediaStreamDescriptor. Do you want me to rename owner to client there as well in an followup patch?

Yeah, that's a good idea.  Sorry for missing that earlier.

[Tommy Widenflycht]

Created attachment 168639 [details]
Patch

[Tommy Widenflycht]

(In reply to comment #7)
> (From update of attachment 168386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168386&action=review
> 
> This looks great.  We just need to make sure it still compiles w.r.t. the IDL parser change last night.
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:28
> > +interface [
> > +    Conditional=MEDIA_STREAM,
> > +    EventTarget
> > +] RTCDataChannel {
> 
> We're working on making WebKitIDL more like WebIDL, so we might need to move these attributes before the keyword "interface"

Fixed this, and only this, in the latest patch.

[Adam Barth]

Comment on attachment 168639 [details]
Patch

Great.

[Tommy Widenflycht]

(In reply to comment #11)
> (From update of attachment 168639 [details])
> Great.

Thanks :)
I tend to worry that something else have sneaked into the patch and therefore wait for the buildbots before flipping the CQ+ flag.

[WebKit Review Bot]

Comment on attachment 168639 [details]
Patch

Rejecting attachment 168639 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14292799

[WebKit Review Bot]

Comment on attachment 168639 [details]
Patch

Clearing flags on attachment: 168639

Committed r131372: <http://trac.webkit.org/changeset/131372>