-
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
RTCDataChannel: Use internal slots and specify default values at one location #1358
Conversation
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 looks good; just some minor comments
webrtc.html
Outdated
<p>Initialize <var>channel</var>'s <code><a data-for= | ||
"RTCDataChannel">label</a></code> attribute to the value of | ||
the first argument.</p> | ||
<p>Let <var>channel</var> have an [[<dfn id= |
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.
Should be "a [[label]] slot" instead of "an [[label]] slot". Also, we should decide on a consistent naming approach for internal slots that correspond to attributes (since you can't have two "dfn"s with the same name).
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.
Will fix typo. I agree that we should try to be consistent on the naming and the "link names", if we need links to all slots.
webrtc.html
Outdated
"RTCDataChannelInit">maxPacketLifeTime</a></code>, <code><a | ||
data-for="RTCDataChannelInit">maxRetransmits</a></code>, and | ||
<code><a data-for="RTCDataChannelInit">id</a></code>.</div> | ||
<p>Let <var>options</var> be the second argument, if present, |
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.
I don't think we need to say "if present", because:
If the type of an argument is a dictionary type or a union type that has a dictionary as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified.
In other words, this algorithm always at least has an options
dictionary to work with, though it may be empty.
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.
Thanks for the pointer. The default value makes things a bit easier.
webrtc.html
Outdated
"maxPacketLifeTime-slot">MaxPacketLifeTime</dfn>]] internal | ||
slot initialized to <var>option</var>'s | ||
<code>maxPacketLifeTime</code> member, given that | ||
<var>options</var> is set and the member is present, otherwise |
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.
Should be able to remove "given that options
is set" from these descriptions.
webrtc.html
Outdated
<li> | ||
<p>If <var>options</var> is still unset, let it be a new | ||
<code><a>RTCDataChannelInit</a></code> dictionary instance | ||
with the specified default values.</p> |
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.
Again, WebIDL bindings already do this.
Thanks for reviewing @taylor-b. The next version of the patch should be simpler when we can assume a default value for the options dictionary. |
LGTM. |
fixes #1353