Bug 222965 - Make RTCDataChannel transferable
Summary: Make RTCDataChannel transferable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-09 00:39 PST by youenn fablet
Modified: 2021-03-12 05:20 PST (History)
16 users (show)

See Also:


Attachments
Patch (60.91 KB, patch)
2021-03-09 01:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (47.05 KB, patch)
2021-03-11 01:18 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (48.02 KB, patch)
2021-03-12 02:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-03-09 00:39:34 PST
This would allow to use RTCDataChannel in workers, thus allowing to process/send messages without being blocked on main thread.
Comment 1 youenn fablet 2021-03-09 01:12:10 PST
Created attachment 422675 [details]
Patch
Comment 2 youenn fablet 2021-03-11 01:18:31 PST
Created attachment 422914 [details]
Patch
Comment 3 Eric Carlson 2021-03-11 09:07:42 PST
Comment on attachment 422914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422914&action=review

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:68
> +        if (!channel->m_handler)
> +            return;

Is this unexpected? If so, it may be worth logging an error to help track down errors later.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:270
> +    ASSERT(m_isDetachable);

Maybe ASSERT(canDetach())?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:295
> +    if (identifier.processIdentifier != Process::identifier()) {
> +        // FIXME: Add support for out-of-process data channels.
> +        return createClosedChannel(context, WTFMove(label), WTFMove(options));
> +    }

Maybe log an error here to help diagnose error reports?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:301
> +    if (!handler)
> +        return createClosedChannel(context, WTFMove(label), WTFMove(options));

Ditto
Comment 4 youenn fablet 2021-03-12 01:27:44 PST
Comment on attachment 422914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422914&action=review

>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:68
>> +            return;
> 
> Is this unexpected? If so, it may be worth logging an error to help track down errors later.

It is not unexpected, in case a channel is created and closed immediately.

>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:270
>> +    ASSERT(m_isDetachable);
> 
> Maybe ASSERT(canDetach())?

Yep, sounds much better! And I will add that bufferedAmount is zero as well.

>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:295
>> +    }
> 
> Maybe log an error here to help diagnose error reports?

OK, will do.

>> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:301
>> +        return createClosedChannel(context, WTFMove(label), WTFMove(options));
> 
> Ditto

For this one, it would be an error right now, but in the future if the connection gets closed before the transfer happens, we could end up in that case.
Comment 5 youenn fablet 2021-03-12 02:16:59 PST
Created attachment 423025 [details]
Patch for landing
Comment 6 EWS 2021-03-12 05:19:14 PST
Committed r274351: <https://commits.webkit.org/r274351>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423025 [details].
Comment 7 Radar WebKit Bug Importer 2021-03-12 05:20:15 PST
<rdar://problem/75358977>