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

Disambiguate all uses of RTCSessionDescription. #2614

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Dec 10, 2020

Fixes #1477.


Preview | Diff

Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

looks good directionally, but with some changes needed

webrtc.html Outdated Show resolved Hide resolved
webrtc.html Outdated Show resolved Hide resolved
webrtc.html Outdated Show resolved Hide resolved
Copy link
Contributor

@henbos henbos left a comment

Choose a reason for hiding this comment

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

There are some comments but happy with "editors can integrate"

@henbos
Copy link
Contributor

henbos commented Dec 10, 2020

The chairs/etc will have to decide what to do with editorial issues this point in time.

@jan-ivar
Copy link
Member Author

@dontcallmedom Should we merge these editorial PRs to get them in, or hold off?

@henbos
Copy link
Contributor

henbos commented Dec 10, 2020

We thought it looked good but the concern came up of "what if we accidentally introduce a bug before the cut" so we need some stance on what to do with minor editorial changes this late in the process

@dontcallmedom
Copy link
Member

from a process perspective merging them in is OK; but the concerns about introducing late bugs in what looks editorial is also reasonable. My guess is that if enough eyeballs have taken a hard look at the PR, then merging should be OK.

@jan-ivar
Copy link
Member Author

My guess is that if enough eyeballs have taken a hard look at the PR, then merging should be OK.

Thanks @dontcallmedom and @henbos. I've looked over the PR again and I found no mistakes.

  • I did a full pass of the Files changed diff.
  • I followed all "session description" links in the preview.
  • I also did the same in the preview diff though I found it unreliable (it trips over webidl that is unchanged by this patch)
  • I checked the new "pendingDescription" variable added instead of referring to the description "being rolled back".
  • I checked the three apply-rewordings to avoid the phrases "prior to applying" and "is applied" wrt the setting algorithms (replaced by "prior to pendingDescription being set" and "is set successfully" to denote more accurately what is being referred to: the former refers to pending descriptions, the latter refers to being set as current description, which frankly remains vague but is thankfully non-normative).

I think this is an improvement., so I'd like to get this in before the Dec 15th deadline. @alvestrand and @aboba can I get an Editors can integrate?

@jan-ivar
Copy link
Member Author

which frankly remains vague but is thankfully non-normative

Actually, it's not vague because an "answer" can never be pending (that would be "pranswer"), so this is good.

@henbos
Copy link
Contributor

henbos commented Dec 14, 2020

I am happy with editors can integrate but I have already approved it. So can @alvestrand or @aboba give their thumbs up or down?

@jan-ivar
Copy link
Member Author

Thanks @alvestrand I take that as a sign to go ahead.

@jan-ivar jan-ivar merged commit a07a324 into w3c:master Dec 15, 2020
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.

Minor inconsistencies around RTCSessionDescription vs. RTCSessionDescriptionInit
4 participants