Bug 99435

Summary: MediaStream API: Add the chromium API for RTCDataChannel
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebKit APIAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cabanier, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch none

Description Tommy Widenflycht 2012-10-16 01:44:05 PDT
Add the chromium API for RTCDataChannel, as well as DRT mock changes and tests.
Comment 1 Tommy Widenflycht 2012-10-16 02:30:51 PDT
Created attachment 168898 [details]
Patch
Comment 2 Rik Cabanier 2012-10-16 08:58:43 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-10-16 12:15:16 PDT
Comment on attachment 168898 [details]
Patch

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

> Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:71
> +    // RTCDataChannel
> +    virtual bool openDataChannel(const WebRTCDataChannel&) { return false; }
> +    virtual bool sendStringData(const WebRTCDataChannel&, const WebString&) { return false; }
> +    virtual bool sendRawData(const WebRTCDataChannel&, const char*, size_t) { return false; }
> +    virtual void closeDataChannel(const WebRTCDataChannel&) { }

The relationship between the PeerConnectionHandler and the DataChannel are slightly strange.  The data channel seems to represent the WebKit side of the channel and you're using PeerConnectionHandler as the embedder endpoint.  I would have expected the DataChannel to have more a of a send/didReceive sort of API, but this approach seems ok too.
Comment 4 WebKit Review Bot 2012-10-16 12:47:45 PDT
Comment on attachment 168898 [details]
Patch

Rejecting attachment 168898 [details] from commit-queue.

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

Last 500 characters of output:
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Rename some AudioNodes

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14391275
Comment 5 WebKit Review Bot 2012-10-16 13:41:39 PDT
Comment on attachment 168898 [details]
Patch

Clearing flags on attachment: 168898

Committed r131494: <http://trac.webkit.org/changeset/131494>
Comment 6 WebKit Review Bot 2012-10-16 13:41:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Tommy Widenflycht 2012-10-17 01:58:52 PDT
(In reply to comment #3)
> (From update of attachment 168898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168898&action=review
> 
> > Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:71
> > +    // RTCDataChannel
> > +    virtual bool openDataChannel(const WebRTCDataChannel&) { return false; }
> > +    virtual bool sendStringData(const WebRTCDataChannel&, const WebString&) { return false; }
> > +    virtual bool sendRawData(const WebRTCDataChannel&, const char*, size_t) { return false; }
> > +    virtual void closeDataChannel(const WebRTCDataChannel&) { }
> 
> The relationship between the PeerConnectionHandler and the DataChannel are slightly strange.  The data channel seems to represent the WebKit side of the channel and you're using PeerConnectionHandler as the embedder endpoint.  I would have expected the DataChannel to have more a of a send/didReceive sort of API, but this approach seems ok too.

It's a very good comment, I implemented it like this for two reasons:

1)  Data channels are not stand-alone entities like websockets are, they are "only" a view into part of the data stream between two PeerConnections. Hence it makes some sense of extending the PeerConnection handler interface.

2) The current descriptor pattern that we are using doesn't really work for communication from JS to the UA, but the other way is fine.