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

Don't fire connectionstatechange on pc.close(). #2876

Merged
merged 3 commits into from
Jul 13, 2023
Merged

Don't fire connectionstatechange on pc.close(). #2876

merged 3 commits into from
Jul 13, 2023

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Jun 2, 2023 •
edited by pr-preview bot

Fixes #2865.

Preview | Diff

@jan-ivar jan-ivar self-assigned this Jun 2, 2023
@jan-ivar
Copy link
Member Author

I'm getting validation errors, but I don't see anything wrong. @dontcallmedom am I using the append amendments type wrong?

"file:/home/runner/work/webrtc-pc/webrtc-pc.w3c/webrtc.html":2675.15-2675.146: info warning: Possible misuse of “aria-label”. (If you disagree with this warning, file an issue report or send e-mail to www-validator@w3.org.)
"file:/home/runner/work/webrtc-pc/webrtc-pc.w3c/webrtc.html":2705.13-2705.58: error: Element “div” not allowed as child of element “ol” in this context. (Suppressing further errors from this subtree.)

youennf
youennf approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@youennf youennf left a comment •
edited

Choose a reason for hiding this comment

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

I don't think this is a change of behaviour.
Close is probably setting the connection state to closed synchronously.
Then the algorithm will try to update and be a no-op.
Worth clarifying though

alvestrand
alvestrand approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

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

I don't understand the markup, but agree with the behavior change (which I think is probably the current behavior of browsers; test needed), so marking myself as approving.

base-rec.html Show resolved Hide resolved
webrtc.html
object associated with the {{RTCDtlsTransport}} object whose state changed.
</p>
</li>
<li class="add-to-connectionstate-algo">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a proper use of the "class" attribute?

@dontcallmedom
Copy link
Member

I've fixed the markup issue in 46a3411 - the "append" treatment needed to be handled on the <li>, not the parent <ol>

@jan-ivar jan-ivar merged commit 98a2fc0 into w3c:main Jul 13, 2023
3 checks passed
@jan-ivar jan-ivar deleted the noconnectionstateclose branch July 13, 2023 13:38
@dontcallmedom dontcallmedom mentioned this pull request Nov 15, 2023
Merged
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvestrand alvestrand alvestrand approved these changes

@youennf youennf youennf approved these changes

@aboba aboba Awaiting requested review from aboba

Assignees

@jan-ivar jan-ivar

Labels
Editors can integrate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The steps in "Update the connection state" say to fire connectionstatechange when [[IsClosed]] is set to true
5 participants
@jan-ivar @dontcallmedom @alvestrand @youennf @Orphis