Created attachment 168898 [details] Patch

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 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 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 on attachment 168898 [details] Patch Clearing flags on attachment: 168898 Committed r131494: <http://trac.webkit.org/changeset/131494>

All reviewed patches have been landed. Closing bug.

(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.