-
Notifications
You must be signed in to change notification settings - Fork 115
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
Algorithm for rejecting modification #1333
Conversation
Fix for Issue #1330
@fippo Can you review? |
LGTM. You made the second commit when I was about to nag ;-) |
webrtc.html
Outdated
then reject <var>p</var> with a newly | ||
<a data-link-for="exception" data-lt="create">created</a> | ||
<code>InvalidStateError</code> and abort these steps.</p> | ||
</li> | ||
<li> | ||
Let <var>lastOffer</var> be the result returned by the | ||
last call to <code>createOffer</code>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It's obvious enough to me what this means, but interpreted literally, it's unclear if lastOffer
is an RTCSessionDescription
, a DOMString
or a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified that this means RTCSessionDescriptionInit.sdp.
webrtc.html
Outdated
<li> | ||
If <code><var>description</var>.type</code> is | ||
<code>offer</code> and <code><var>description</var>.sdp</code> | ||
does not match <var>lastOffer</var>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"does not match" could be loosely interpreted. If our requirement is that they're byte-for-byte identical, which is what JSEP ended up saying, we should make that more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "match byte-for-byte".
webrtc.html
Outdated
<li> | ||
If <code><var>description</var>.type</code> is | ||
<code>offer</code> and <code><var>description</var>.sdp</code> | ||
does not match <var>lastOffer</var> byte-for-byte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "does not match byte-for-byte" too informal? Could just say "is not equal to".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
webrtc.html
Outdated
then reject <var>p</var> with a newly | ||
<a data-link-for="exception" data-lt="create">created</a> | ||
<code>InvalidStateError</code> and abort these steps.</p> | ||
</li> | ||
<li> | ||
Let <var>lastOffer</var> be the value of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be written such that lastOffer
is an internal slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LastOffer and LastAnswer are now defined as internal slots.
webrtc.html
Outdated
with a newly <a data-link-for="exception" | ||
data-lt="create">created</a> | ||
<code>InvalidModificationError</code> and abort these steps. | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This set of steps should only apply when setting a local description; InvalidModificationError
shouldn't be thrown when setting a remote offer that differs from lastOffer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
LGTM. |
Fix for Issues #1254, #1309, #1330 and #1344