Bug 104543 - MediaStream API: Change the data channel descriptor pattern to a handler pattern
Summary: MediaStream API: Change the data channel descriptor pattern to a handler pattern
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (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-12-10 05:58 PST by Tommy Widenflycht
Modified: 2012-12-12 02:50 PST (History)
13 users (show)

See Also:


Attachments
Patch (91.40 KB, patch)
2012-12-10 06:03 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (91.38 KB, patch)
2012-12-10 06:29 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (92.89 KB, patch)
2012-12-11 02:09 PST, Tommy Widenflycht
tkent: review+
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (92.68 KB, patch)
2012-12-12 02:15 PST, 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-12-10 05:58:14 PST
This will fix lifetime/memory/code structure issues in the chromium port.
Comment 1 Tommy Widenflycht 2012-12-10 06:03:48 PST
Created attachment 178529 [details]
Patch
Comment 2 Tommy Widenflycht 2012-12-10 06:06:13 PST
Reviewers: This patch is big but it contains quite a bit of deleted files. It is also straight-forward and not particularly complicated.
Comment 3 WebKit Review Bot 2012-12-10 06:08:38 PST
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 4 kov's GTK+ EWS bot 2012-12-10 06:14:44 PST
Comment on attachment 178529 [details]
Patch

Attachment 178529 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15219972
Comment 5 Tommy Widenflycht 2012-12-10 06:29:48 PST
Created attachment 178534 [details]
Patch
Comment 6 Darin Fisher (:fishd, Google) 2012-12-10 11:28:57 PST
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?
Comment 7 Tommy Widenflycht 2012-12-11 02:09:51 PST
Created attachment 178753 [details]
Patch
Comment 8 Tommy Widenflycht 2012-12-11 02:19:05 PST
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 9 Kent Tamura 2012-12-12 01:00:43 PST
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.
Comment 10 Tommy Widenflycht 2012-12-12 02:15:13 PST
Created attachment 179006 [details]
Patch for landing
Comment 11 Tommy Widenflycht 2012-12-12 02:16:31 PST
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 12 WebKit Review Bot 2012-12-12 02:43:41 PST
Comment on attachment 179006 [details]
Patch for landing

Clearing flags on attachment: 179006

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