Bug 104543 – MediaStream API: Change the data channel descriptor pattern to a handler pattern

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

This will fix lifetime/memory/code structure issues in the chromium port.

[Tommy Widenflycht]

Created attachment 178529 [details]
Patch

[Tommy Widenflycht]

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

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[kov's GTK+ EWS bot]

Comment on attachment 178529 [details]
Patch

Attachment 178529 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15219972

[Tommy Widenflycht]

Created attachment 178534 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Tommy Widenflycht]

Created attachment 178753 [details]
Patch

[Tommy Widenflycht]

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.

[Kent Tamura]

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.

[Tommy Widenflycht]

Created attachment 179006 [details]
Patch for landing

[Tommy Widenflycht]

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.

[WebKit Review Bot]

Comment on attachment 179006 [details]
Patch for landing

Clearing flags on attachment: 179006

Committed r137441: <http://trac.webkit.org/changeset/137441>