Bug 169732

Summary: Clean up RTCDataChannel
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebKit Misc.Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex, buildbot, cdumez, commit-queue, eric.carlson, rniwa, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 169731    
Bug Blocks: 169662    
Attachments:
Description Flags
Patch
none
Patch
none
patch
none
patch for ews
none
Patch
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Fixing test failures
none
Rebasing patch
none
Patch
none
Patch
none
Patch for landing none

Description Jon Lee 2017-03-15 20:32:07 PDT
Clean up RTCDataChannel
Comment 1 Jon Lee 2017-03-15 20:37:39 PDT
Created attachment 304601 [details]
Patch
Comment 2 Jon Lee 2017-03-15 20:43:40 PDT
Created attachment 304602 [details]
Patch
Comment 3 youenn fablet 2017-03-17 08:27:05 PDT
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!
Comment 4 Jon Lee 2017-03-23 01:06:08 PDT
Created attachment 305175 [details]
patch

patch missing tests
Comment 5 Build Bot 2017-03-23 01:09:42 PDT
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.
Comment 6 Jon Lee 2017-03-23 01:22:31 PDT
Created attachment 305177 [details]
patch for ews
Comment 7 Build Bot 2017-03-23 01:25:04 PDT
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.
Comment 8 youenn fablet 2017-03-28 13:31:37 PDT
Created attachment 305636 [details]
Patch
Comment 9 Build Bot 2017-03-28 15:41:32 PDT
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
Comment 10 Build Bot 2017-03-28 15:41:35 PDT
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
Comment 11 youenn fablet 2017-03-28 16:46:08 PDT
Created attachment 305670 [details]
Fixing test failures
Comment 12 youenn fablet 2017-03-28 21:07:23 PDT
Created attachment 305702 [details]
Rebasing patch
Comment 13 youenn fablet 2017-03-28 21:08:28 PDT
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 14 Chris Dumez 2017-03-29 09:34:00 PDT
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 15 youenn fablet 2017-03-30 08:25:12 PDT
Created attachment 305862 [details]
Patch
Comment 16 youenn fablet 2017-03-30 08:26:03 PDT
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 17 Chris Dumez 2017-03-30 08:51:21 PDT
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 18 youenn fablet 2017-03-30 09:37:11 PDT
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.
Comment 19 youenn fablet 2017-03-30 09:37:45 PDT
Created attachment 305865 [details]
Patch
Comment 20 Chris Dumez 2017-03-30 12:48:43 PDT
Comment on attachment 305865 [details]
Patch

r=me
Comment 21 WebKit Commit Bot 2017-03-30 13:19:58 PDT
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
Comment 22 youenn fablet 2017-03-30 14:10:55 PDT
Created attachment 305901 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2017-03-30 14:42:23 PDT
Comment on attachment 305901 [details]
Patch for landing

Clearing flags on attachment: 305901

Committed r214627: <http://trac.webkit.org/changeset/214627>
Comment 24 WebKit Commit Bot 2017-03-30 14:42:25 PDT
All reviewed patches have been landed.  Closing bug.