-
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
Add RtpSender.transport, RtpReceiver.transport, RTCDtlsTransport, RTCIceTransport, etc #241
Conversation
…eCandidateType, and IceCandidateProtocol.
Please fix the errors that phantomjs reports from Travis. The html5valdiator errors are a known bug in the HTML5 validator, and Dom has proposed a fix. |
|
||
<dd> | ||
<p>The <dfn id= | ||
"dom-dtlstransport-state"><code>state</code></dfn> |
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 suggest using <code id="dom-dtlstransport-state">state</code> (no <dfn>) to avoid the dup id error of ReSpec.
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.
OK, I have done that for all of the places where ReSpec was complaining. Actually, it found several real mistakes, which I fixed.
I will circle back with @pthatcherg to see where we are on this one. |
Peter is off on mini-vacation until Monday, he said he will endeavour to look at it when he is back at his desk. |
I have fixed all the errors and merged with master. |
Hopefully Peter, Harald, and I can look at this together in Prague to see if it's ready to go in. |
Ok, with that in mind I am assigning this to you Dan. |
<code>RTCRtpSender.track</code> is sent in the form of RTP | ||
packets. When BUNDLE is used, | ||
many <code>RTCRtpSender</code> objects will share | ||
one <code>RTCRtpSender.track</code> and all will all send |
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 think you mean RTCRtpSender.transport here.
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
There is ongoing discussion on the public-webrtc mail list regarding this PR: https://lists.w3.org/Archives/Public/public-webrtc/2015Jul/0080.html @pthatcherg any comment from you on where we are on this today? |
<code>RTCRtpSender.track</code> is sent in the form of RTP | ||
packets. When BUNDLE is used, | ||
many <code>RTCRtpSender</code> objects will share | ||
one <code>RTCRtpSender.transport</code> and all will all send |
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.
One "all" too much I think
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
Didn't we have a shared base class for RTCRtpSender and Receiver at some point? Feels like a lot of stuff are repeated here. Future work:
|
I don't think we ever had a common base for RtpSender and RtpReceiver. I don't think it's worth it to have one (since it's nice to have comments that say things like "the transport on which media is sent" and not "the transport on which media is sent or received". But even if we decided to do that, I'd prefer to merge the PR and address that separately. |
@adam-be Do you want me to add a TODO for the future work (Event summary section)? |
The comment about a common base class was more of a side-note. Perhaps it's enough to create an issue for updating the Event Summary section. It's a quick fix when we get the event naming sorted out. :) |
attribute is the transport over which media from | ||
<code>RTCRtpSender.track</code> is sent in the form of RTP | ||
packets. When BUNDLE is used, | ||
many <code>RTCRtpSender</code> objects will share |
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.
Please use links when referring to types like <code>RTCRtpSender</code> to make them clickable. So it should be <code><a>RTCRtpSender</a></code>. This isn't crucial for the PR, but every time we say that it's easy to fix in an follow-up PR, we never do it. :)
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.
Types are now clickable.
Thanks for fixing the links and adding the Event Summary section as well. The last commit broke Travis, but at least one error was some strange Respec.js bug (it doesn't seem possible to have the same text between two dfn-tags even though their ids are different). I've made a PR against your PR branch that fixes those. Otherwise this LGTM. |
Fix Travis warnings
Thanks! I merged your PR of my PR into my PR. PRs of PRs. We have to go deeper. On Thu, Aug 27, 2015 at 2:57 AM, Adam Bergkvist notifications@github.com
|
Add RtpSender.transport, RtpReceiver.transport, RTCDtlsTransport, RTCIceTransport, etc
No description provided.