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 RtpSender.transport, RtpReceiver.transport, RTCDtlsTransport, RTCIceTransport, etc #241

Merged
merged 20 commits into from Sep 3, 2015

Conversation

pthatcherg
Copy link
Contributor

No description provided.

@alvestrand
Copy link
Contributor

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>
Copy link
Member

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.

Copy link
Contributor Author

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.

@elagerway
Copy link
Contributor

I will circle back with @pthatcherg to see where we are on this one.

@elagerway elagerway self-assigned this Jul 2, 2015
@elagerway
Copy link
Contributor

Peter is off on mini-vacation until Monday, he said he will endeavour to look at it when he is back at his desk.

@pthatcherg
Copy link
Contributor Author

I have fixed all the errors and merged with master.

@burnburn
Copy link
Contributor

burnburn commented Jul 9, 2015

Hopefully Peter, Harald, and I can look at this together in Prague to see if it's ready to go in.

@elagerway elagerway assigned burnburn and unassigned elagerway Jul 16, 2015
@elagerway
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@elagerway
Copy link
Contributor

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?

@elagerway elagerway removed their assignment Aug 13, 2015
<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
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@adam-be
Copy link
Member

adam-be commented Aug 18, 2015

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:

  • Add the RTCDtlsTransport state event to the Event Summary section

@pthatcherg
Copy link
Contributor Author

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.

@pthatcherg
Copy link
Contributor Author

@adam-be Do you want me to add a TODO for the future work (Event summary section)?

@adam-be
Copy link
Member

adam-be commented Aug 24, 2015

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
Copy link
Member

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Types are now clickable.

@adam-be
Copy link
Member

adam-be commented Aug 27, 2015

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.

@pthatcherg
Copy link
Contributor Author

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
wrote:

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.


Reply to this email directly or view it on GitHub
#241 (comment).

adam-be added a commit that referenced this pull request Sep 3, 2015
Add RtpSender.transport, RtpReceiver.transport, RTCDtlsTransport, RTCIceTransport, etc
@adam-be adam-be merged commit 1654845 into w3c:master Sep 3, 2015
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

7 participants