Bug 99435 - MediaStream API: Add the chromium API for RTCDataChannel
Summary: MediaStream API: Add the chromium API for RTCDataChannel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-10-16 01:44 PDT by Tommy Widenflycht
Modified: 2012-10-17 01:58 PDT (History)
9 users (show)

See Also:


Attachments
Patch (32.12 KB, patch)
2012-10-16 02:30 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.