Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add initialization of DataChannelId on connection #2222

Merged
merged 4 commits into from
Oct 3, 2019
+23 −11
Merged

Add initialization of DataChannelId on connection #2222

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a9651d
Add initialization of DataChannelId on connection
alvestrand Jul 8, 2019
c792701
Remove reference to DTLS role.
aboba Jul 11, 2019
70dc3e5
Review comments
alvestrand Sep 27, 2019
e8d2a4f
Add explanatory sequencing note
alvestrand Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions webrtc.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9558,9 +9558,9 @@ <h2>Methods</h2>
<div class="note">
If the <a>[[\DataChannelId]]</a>
slot is <code>null</code> after this step, it will be
populated once the DTLS role is determined during the
process of <a data-lt="set the RTCSessionDescription">
setting an <code>RTCSessionDescription</code></a>.
populated during the
<a href="#sctp-transport-connected">
<code>RTCSctpTransport</code> connected procedure</a>.
</div>
</li>
<li>
Expand Down Expand Up @@ -9684,26 +9684,38 @@ <h4 id="sctp-transport-connected">Connected procedure</h4>
<p>Set <a>[[\MaxChannels]]</a> to the minimum of the negotiated
amount of incoming and outgoing SCTP streams.</p>
</li>
<li>
<p><a>Fire an event</a> named <code><a data-lt="
RTCSctpTransport state change">statechange</a></code> at <var>
transport</var>.</p>
</li>
<li>
<p>For each of <var>connection</var>'s <code><a>RTCDataChannel</a></code>:</p>
<ol>
<li>
<p>Let <var>channel</var> be the <code><a>RTCDataChannel</a></code> object.</p>
</li>
<li>
<p>If the <var>channel</var>.<a>[[\DataChannelId]]</a> is greater or
equal to <var>transport</var>.<a>[[\MaxChannels]]</a>,
<p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the statechange event is being caught by the application, is it ensured that the ids have been populated? Probably yes since the event is being appended to the event loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I moved the statechange event to after the ID assignment, but that causes "announce the channel as open" to move before the statechange event. Does that matter?
I don't think so, because the "Announcing a data channel as open" section starts off with "MUST queue a task".

<code>null</code>, initialize <a>[[\DataChannelId]]</a>
to the value generated by the
underlying sctp data channel, according to
[[!RTCWEB-DATA-PROTOCOL]].
</p>
<li>
<p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is greater or
equal to <var>transport</var>'s <a>[[\MaxChannels]]</a> slot,
or the previous step failed to assign an id,
<a data-lt="data-channel-failure"> close the <var>channel</var> due to a failure
</a>. Otherwise, <a data-lt="announce the rtcdatachannel as open">announce the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not new in this PR: We are inside of steps invoking "announce the rtcdatachannel as open" and "close the channel due to failure". The referenced algorithm queues a task. We should refactor this so that we can invoke the algorithms without queuing a task; the queuing of a task is in response to something happening in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed in this PR.

<var>channel</var> as open</a>.</p>
</li>
</ol>
</li>
<li>
<p><a>Fire an event</a> named <code><a data-lt="
RTCSctpTransport state change">statechange</a></code> at <var>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event fire order can now be misinterpreted easily. The individual open events fire after the statechange event because the open event is scheduled in yet another task. When we refactor that as suggested by @henbos, we need to ensure not to break that order: statechange, then for each data channel, open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a note seems reasonable.

transport</var>.</p>
<p class="note">This event is fired before the "open" events
fired by <a data-lt="announce the rtcdatachannel as open">
announcing the <var>channel</var> as open</a>; the "open"
events are fired from a queued task.</p>
</li>
</ol>
</section>
<div>
Expand Down Expand Up @@ -9751,7 +9763,7 @@ <h2>Attributes</h2>
getting, return the value of the <a>[[\MaxChannels]]</a> slot.
</p>
<div class="note">This attribute's value will be
<code>null</code> until the SCTP transport went into the
<code>null</code> until the SCTP transport goes into the
<code>connected</code> state.
</div>
</dd>
Expand Down