Bug 169732 – Clean up RTCDataChannel

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Clean up RTCDataChannel

[Jon Lee]

Created attachment 304601 [details]
Patch

[Jon Lee]

Created attachment 304602 [details]
Patch

[youenn fablet]

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!

[Jon Lee]

Created attachment 305175 [details]
patch

patch missing tests

[Build Bot]

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.

[Jon Lee]

Created attachment 305177 [details]
patch for ews

[Build Bot]

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.

[youenn fablet]

Created attachment 305636 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 305670 [details]
Fixing test failures

[youenn fablet]

Created attachment 305702 [details]
Rebasing patch

[youenn fablet]

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.

[Chris Dumez]

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, ...

[youenn fablet]

Created attachment 305862 [details]
Patch

[youenn fablet]

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, ...

[Chris Dumez]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 305865 [details]
Patch

[Chris Dumez]

Comment on attachment 305865 [details]
Patch

r=me

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 305901 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 305901 [details]
Patch for landing

Clearing flags on attachment: 305901

Committed r214627: <http://trac.webkit.org/changeset/214627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.