Bug 99080

Created attachment 168241 [details] Patch

If the patch is on the large side I can split it, but I wanted to give the full picture first.

Created attachment 168244 [details] Patch

Comment on attachment 168244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168244&action=review This patch could use another iteration, but my comments are mostly just naming nits. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:42 > +PassRefPtr<RTCDataChannel> RTCDataChannel::create(ScriptExecutionContext* context, RTCPeerConnectionHandler* handler, String label, bool reliable, ExceptionCode& ec) String -> const String& > Source/WebCore/Modules/mediastream/RTCDataChannelEvent.idl:30 > + readonly attribute RTCDataChannel channel; Bad indent > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:413 > +PassRefPtr<RTCDataChannel> RTCPeerConnection::createDataChannel(String label, const Dictionary& dataChannelDict, ExceptionCode& ec) dataChannelDict -> options? > Source/WebCore/platform/mediastream/RTCDataChannelDescriptor.h:38 > + class Owner { Typically we'd call this a client rather than an owner. Also, we'd usually declare it as a top-level class "RTCDataChannelClient" It's confusing for a RefCounted object to have an "owner", which is why we tend not to use that term.

Comment on attachment 168244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168244&action=review >> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:42 >> +PassRefPtr<RTCDataChannel> RTCDataChannel::create(ScriptExecutionContext* context, RTCPeerConnectionHandler* handler, String label, bool reliable, ExceptionCode& ec) > > String -> const String& Fixed. >> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.idl:30 >> + readonly attribute RTCDataChannel channel; > > Bad indent Fixed. >> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:413 >> +PassRefPtr<RTCDataChannel> RTCPeerConnection::createDataChannel(String label, const Dictionary& dataChannelDict, ExceptionCode& ec) > > dataChannelDict -> options? Fixed. dataChannelDict is used in the specification but options is much better. >> Source/WebCore/platform/mediastream/RTCDataChannelDescriptor.h:38 >> + class Owner { > > Typically we'd call this a client rather than an owner. > > Also, we'd usually declare it as a top-level class "RTCDataChannelClient" > > It's confusing for a RefCounted object to have an "owner", which is why we tend not to use that term. Agree with that, I just reused the name from MediaStreamDescriptor. Do you want me to rename owner to client there as well in an followup patch?

Created attachment 168386 [details] Patch

Comment on attachment 168386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168386&action=review This looks great. We just need to make sure it still compiles w.r.t. the IDL parser change last night. > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:28 > +interface [ > + Conditional=MEDIA_STREAM, > + EventTarget > +] RTCDataChannel { We're working on making WebKitIDL more like WebIDL, so we might need to move these attributes before the keyword "interface"

> I just reused the name from MediaStreamDescriptor. Do you want me to rename owner to client there as well in an followup patch? Yeah, that's a good idea. Sorry for missing that earlier.

Created attachment 168639 [details] Patch

(In reply to comment #7) > (From update of attachment 168386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168386&action=review > > This looks great. We just need to make sure it still compiles w.r.t. the IDL parser change last night. > > > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:28 > > +interface [ > > + Conditional=MEDIA_STREAM, > > + EventTarget > > +] RTCDataChannel { > > We're working on making WebKitIDL more like WebIDL, so we might need to move these attributes before the keyword "interface" Fixed this, and only this, in the latest patch.

Comment on attachment 168639 [details] Patch Great.

(In reply to comment #11) > (From update of attachment 168639 [details]) > Great. Thanks :) I tend to worry that something else have sneaked into the patch and therefore wait for the buildbots before flipping the CQ+ flag.

Comment on attachment 168639 [details] Patch Rejecting attachment 168639 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14292799

Comment on attachment 168639 [details] Patch Clearing flags on attachment: 168639 Committed r131372: <http://trac.webkit.org/changeset/131372>