Bug 104543

Created attachment 178529 [details] Patch

Reviewers: This patch is big but it contains quite a bit of deleted files. It is also straight-forward and not particularly complicated.

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 178529 [details] Patch Attachment 178529 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15219972

Created attachment 178534 [details] Patch

Comment on attachment 178534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178534&action=review > Source/Platform/chromium/public/WebRTCDataChannelHandler.h:39 > + virtual void setClient(WebRTCDataChannelHandlerClient*) = 0; Q: do you need to be able to modify the client at runtime? how about just passing it to the createDataChannel method? > Source/Platform/chromium/public/WebRTCDataChannelHandler.h:42 > + virtual bool reliable() = 0; nit: this method looks like it is asking a question, so perhaps it should be named as such? isReliable? > Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:46 > + virtual void dataArrived(const WebString&) const = 0; nit: call this didReceiveData just like WebURLLoaderClient? > Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:47 > + virtual void dataArrived(const char*, size_t) const = 0; Q: why are there two versions of this function? should the first be called didReceiveStringData and the second didReceiveRawData? I'm re-using the terms from WebRTCDataChannelHandler. > Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:48 > + virtual void error() const = 0; nit: didReceiveError?

Created attachment 178753 [details] Patch

Comment on attachment 178534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178534&action=review >> Source/Platform/chromium/public/WebRTCDataChannelHandler.h:39 >> + virtual void setClient(WebRTCDataChannelHandlerClient*) = 0; > > Q: do you need to be able to modify the client at runtime? how about just passing it to the createDataChannel method? No, the client will nor change once set. This is needed when the remote side initiates the DataChannel setup; the local UA detects that and creates an WebRTCDataChannelHandler and sends to through the WebRTCPeerConnectionHandlerClient. The WebCore code then needs to set the client. This way it is symmetrical. >> Source/Platform/chromium/public/WebRTCDataChannelHandler.h:42 >> + virtual bool reliable() = 0; > > nit: this method looks like it is asking a question, so perhaps it should be named as such? isReliable? Done. >> Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:46 >> + virtual void dataArrived(const WebString&) const = 0; > > nit: call this didReceiveData just like WebURLLoaderClient? see next comment >> Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:47 >> + virtual void dataArrived(const char*, size_t) const = 0; > > Q: why are there two versions of this function? should the first be called didReceiveStringData and the second didReceiveRawData? I'm re-using the terms from WebRTCDataChannelHandler. Changed them to didReceiveStringData and didReceiveRawData as suggested. >> Source/Platform/chromium/public/WebRTCDataChannelHandlerClient.h:48 >> + virtual void error() const = 0; > > nit: didReceiveError? Receive is not the correct word so I renamed it to didDetectError.

Comment on attachment 178753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178753&action=review This seems to address Darin's comment. Rubber-stamped. > Source/WebCore/GNUmakefile.list.am:5665 > + Source/WebCore/platform/mediastream/RTCDataChannelHandler.h \ > + Source/WebCore/platform/mediastream/RTCDataChannelHandlerClient.h \ > + Source/WebCore/platform/mediastream/RTCIceCandidateDescriptor.cpp \ These lines should start with TAB to be consistent with other lines.

Created attachment 179006 [details] Patch for landing

Comment on attachment 178753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178753&action=review >> Source/WebCore/GNUmakefile.list.am:5665 >> + Source/WebCore/platform/mediastream/RTCIceCandidateDescriptor.cpp \ > > These lines should start with TAB to be consistent with other lines. Fixed.

Comment on attachment 179006 [details] Patch for landing Clearing flags on attachment: 179006 Committed r137441: <http://trac.webkit.org/changeset/137441>