Clean up RTCDataChannel
Created attachment 304601 [details] Patch
Created attachment 304602 [details] Patch
Comment on attachment 304602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304602&action=review > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:61 > + [ImplementedAs=maxPacketLifeTime] readonly attribute unsigned short maxRetransmitTime; Add the legacy runtime flag here, or maybe remove it!
Created attachment 305175 [details] patch patch missing tests
Attachment 305175 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/mediastream/RTCDataChannelEvent.h:43: The parameter name "isTrusted" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 305177 [details] patch for ews
Attachment 305177 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/mediastream/RTCDataChannelEvent.h:43: The parameter name "isTrusted" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 305636 [details] Patch
Comment on attachment 305636 [details] Patch Attachment 305636 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3429430 New failing tests: imported/w3c/web-platform-tests/webrtc/RTCDataChannelEvent-constructor.html webrtc/datachannel/basic.html
Created attachment 305654 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 305670 [details] Fixing test failures
Created attachment 305702 [details] Rebasing patch
Comment on attachment 305702 [details] Rebasing patch View in context: https://bugs.webkit.org/attachment.cgi?id=305702&action=review > Source/WebCore/Modules/mediastream/RTCDataChannelEvent.h:39 > + RefPtr<RTCDataChannel> channel; Since channel is required, we should be able to use a Ref<> instead of a RefPtr<>. This would require a change in the binding generator.
Comment on attachment 305702 [details] Rebasing patch View in context: https://bugs.webkit.org/attachment.cgi?id=305702&action=review > Source/WebCore/ChangeLog:12 > + Doing some additional cleaning refactoring. Link to the spec > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:30 > NoInterfaceObject, This is not supposed to be NoInterfaceObject. > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:41 > May be nice to add a FIXME comment about the attributes we do not support yet, e.g. readonly attribute RTCPriorityType priority; attribute unsigned long bufferedAmountLowThreshold; attribute EventHandler onbufferedamountlow; > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:42 > [SetterMayThrowException] attribute DOMString binaryType; send() should take a USVString, not a DOMString. >> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.h:39 >> + RefPtr<RTCDataChannel> channel; > > Since channel is required, we should be able to use a Ref<> instead of a RefPtr<>. > This would require a change in the binding generator. Yes, this is a known limitation. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:41 > + unsigned short? maxPacketLifeTime; This is not nullable in the spec? https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelinit > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:42 > + unsigned short? maxRetransmits; This is not nullable in the spec? https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelinit > LayoutTests/webrtc/datachannel/datachannel-event.html:22 > + assert_equals(event.channel, channel, "channel passed in constructor should be used"); You should also check event.type, event.isTrusted, event.bubbles, ...
Created attachment 305862 [details] Patch
Thanks for the review. I updated the patch accordingly and added support for some missing attributes. (In reply to Chris Dumez from comment #14) > Comment on attachment 305702 [details] > Rebasing patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305702&action=review > > > Source/WebCore/ChangeLog:12 > > + Doing some additional cleaning refactoring. > > Link to the spec > > > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:30 > > NoInterfaceObject, > > This is not supposed to be NoInterfaceObject. > > > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:41 > > > > May be nice to add a FIXME comment about the attributes we do not support > yet, e.g. > readonly attribute RTCPriorityType priority; > attribute unsigned long bufferedAmountLowThreshold; > attribute EventHandler onbufferedamountlow; > > > Source/WebCore/Modules/mediastream/RTCDataChannel.idl:42 > > [SetterMayThrowException] attribute DOMString binaryType; > > send() should take a USVString, not a DOMString. > > >> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.h:39 > >> + RefPtr<RTCDataChannel> channel; > > > > Since channel is required, we should be able to use a Ref<> instead of a RefPtr<>. > > This would require a change in the binding generator. > > Yes, this is a known limitation. > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:41 > > + unsigned short? maxPacketLifeTime; > > This is not nullable in the spec? > https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelinit > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:42 > > + unsigned short? maxRetransmits; > > This is not nullable in the spec? > https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelinit > > > LayoutTests/webrtc/datachannel/datachannel-event.html:22 > > + assert_equals(event.channel, channel, "channel passed in constructor should be used"); > > You should also check event.type, event.isTrusted, event.bubbles, ...
Comment on attachment 305862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305862&action=review > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:219 > +void RTCDataChannel::bufferedAmountIsDecreasing(size_t amount) Could you explain what you updated the function to take a parameter? It now causes us to call bufferedAmount() at the call site, even if m_stopped is true, which requires us to deal with m_stopped being true in bufferedAmount(). > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:108 > + protectedClient->bufferedAmountIsDecreasing(static_cast<size_t>(amount)); Why do we need the cast? I thought buffered_amount() returned a size_t already? > LayoutTests/webrtc/datachannel/bufferedAmountLowThreshold-expected.txt:4 > +Harness Error (FAIL), message = InvalidStateError (DOM Exception 11): The object is in an invalid state. Is this expected? > LayoutTests/webrtc/datachannel/datachannel-event.html:23 > + assert_equals(event.type, "RTCDataChannelEvent"); I believe this should be "test", not "RTCDataChannelEvent" > LayoutTests/webrtc/datachannel/datachannel-event.html:24 > + assert_equals(event.isTrusted, true, "trusted"); This seems wrong. How can an event created by JS be trusted?
Comment on attachment 305862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305862&action=review >> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:219 >> +void RTCDataChannel::bufferedAmountIsDecreasing(size_t amount) > > Could you explain what you updated the function to take a parameter? It now causes us to call bufferedAmount() at the call site, even if m_stopped is true, which requires us to deal with m_stopped being true in bufferedAmount(). Right, I will update the change log. libwebrtc data channel lives in the network thread. Also, bufferedAmountIsDecreasing is called asynchronously which could make the bufferedAmount value change between the time it is scheduled and the time it is called. >> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:108 >> + protectedClient->bufferedAmountIsDecreasing(static_cast<size_t>(amount)); > > Why do we need the cast? I thought buffered_amount() returned a size_t already? buffered_amount() is returning a uint64_t. >> LayoutTests/webrtc/datachannel/bufferedAmountLowThreshold-expected.txt:4 >> +Harness Error (FAIL), message = InvalidStateError (DOM Exception 11): The object is in an invalid state. > > Is this expected? Yes, due to send raising an exception when being closed. I will do a try/catch to make it look better. We should fix the implementation to not throw though. >> LayoutTests/webrtc/datachannel/datachannel-event.html:23 >> + assert_equals(event.type, "RTCDataChannelEvent"); > > I believe this should be "test", not "RTCDataChannelEvent" right. >> LayoutTests/webrtc/datachannel/datachannel-event.html:24 >> + assert_equals(event.isTrusted, true, "trusted"); > > This seems wrong. How can an event created by JS be trusted? right.
Created attachment 305865 [details] Patch
Comment on attachment 305865 [details] Patch r=me
Comment on attachment 305865 [details] Patch Rejecting attachment 305865 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 305865, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3442462
Created attachment 305901 [details] Patch for landing
Comment on attachment 305901 [details] Patch for landing Clearing flags on attachment: 305901 Committed r214627: <http://trac.webkit.org/changeset/214627>
All reviewed patches have been landed. Closing bug.