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.

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

Add internal slots for signaling state, connection state, ice gatheri… #2826

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Feb 13, 2023

…ng state, and ice connection state, and fix ongatheringstatechange to use the right one.


Preview | Diff

@alvestrand
Copy link
Contributor

Please don't merge until the REC process has completed.

…ng state, and ice connection state, and fix ongatheringstatechange to use the right one.
@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 9, 2023

Hi @dontcallmedom, I noticed you added amendments to this PR. Thanks for doing that!

I see you also removed the Editorial label. I just had a question about which part(s) are not considered editorial?

In my view, this PR mostly changes the formatting of four internal variables to be consistent with the others.

Before:
image

After:
image

These were already main-thread variables set by algorithms, e.g. before:
image
and after:
image

I.e. they already acted as internal slots. IOW, this PR isn't solving any race which internal slots are sometimes introduced to solve. So does it need amendments?

The PR also fixes this typo (a RTCIceTransport doesn't have an ICE gathering state):
image
...but this prose is not just wrong but normatively redundant since the actual algorithm that fires the event is correct. — we could break that out into a separate PR, since I just noticed the prose on the handlers above and below are also redundant, and one of them also wrong (refers to the WebIDL state as the trigger of the event).

Since we'll be working with the amendments process for the foreseeable future, I wanted to understand whether editorial changes of REC are:

  1. discouraged?
  2. require tracking as amendments?

E.g. do readers care about editorial differences? I can see advantages either way.

Sorry for the long message, but editing on REC documents is new and it's not always clear what's expected.

@dontcallmedom
Copy link
Member

What triggers the usage of the amendment process (and for our repo, the use of the "editorial" label or not) is based on the class of change as described in the W3C Process - essentially, anything that might affect the conformance of an existing implementation.

I had read the fix on the gatheringstatechange event as crossing that line (hence the removal of the Editorial label), but now your description of this being mostly an alignment with other normative text and the text not making sense as is makes a convincing argument that it is in fact a class 2 change, and so the "Editorial" label should be reinstated, and the pull request should not touch amendments.json at all - I'll get that done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants