Make RtpTransport actually implement RtpTransportInterface

Make RtpTransport actually implement RtpTransportInterface

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2805783002
Cr-Commit-Position: refs/heads/master@{#17628}
Committed: https://chromium.googlesource.com/external/webrtc/+/d9ce76444f98b7abdfcacb0e5fbdb278163e8c07

[Zach Stein, 2017-04-07 00:05:43]

The CQ bit was checked by zstein@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 00:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Zach Stein, 2017-04-07 00:06:11]

The CQ bit was unchecked by zstein@webrtc.org

[Zach Stein, 2017-04-07 00:28:21]

zstein@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Zach Stein, 2017-04-07 00:28:22]

PTAL

[Taylor Brandstetter, 2017-04-07 04:16:03]

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:46: "disabling mux is not allowed");
Can use the LOG_AND_RETURN_ERROR macro so this gets logged as well. Also, a nit:
can you change this to "Disabling RTCP muxing is not allowed."? We typically use
capitalization and punctuation for comments and log statements/error
messages/etc.

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:24: class PacketTransportWrapper : public
PacketTransportInterface {
I was confused about why this was necessary until I realized
PacketTransportInternal doesn't subclass from PacketTransportInterface. I think
this is just something I overlooked at some point; it was definitely intended
to.

We do have a diamond inheritance situation here, where UdpTransport would
inherit from PacketTransportInternal and UdpTransportInterface, which both
inherit from PacketTransportInterface. But I think we can solve that with
virtual inheritance? This worked in my local build; I just scheduled a try run
which is green so far: https://codereview.webrtc.org/2806463003/

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:66: RtcpParameters GetRtcpParameters() const override;
Since these methods aren't used anywhere yet, can you add a TODO as a reminder
of what needs to be done?

[Zach Stein, 2017-04-09 21:58:04]

I think I got everything :) Please take another look.

[Taylor Brandstetter, 2017-04-10 19:19:08]

lgtm, though I think the error type should be changed for consistency.

https://codereview.webrtc.org/2805783002/diff/120001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2805783002/diff/120001/webrtc/pc/rtptransport.c...
webrtc/pc/rtptransport.cc:38:
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
I've been using "INVALID_STATE" for situations where a parameter is valid under
some circumstances, but not with the object in its current state.

[Zach Stein, 2017-04-10 22:37:43]

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:46: "disabling mux is not allowed");
On 2017/04/07 04:16:03, Taylor Brandstetter wrote:
> Can use the LOG_AND_RETURN_ERROR macro so this gets logged as well. Also, a
nit:
> can you change this to "Disabling RTCP muxing is not allowed."? We typically
use
> capitalization and punctuation for comments and log statements/error
> messages/etc.

Done.

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:24: class PacketTransportWrapper : public
PacketTransportInterface {
On 2017/04/07 04:16:03, Taylor Brandstetter wrote:
> I was confused about why this was necessary until I realized
> PacketTransportInternal doesn't subclass from PacketTransportInterface. I
think
> this is just something I overlooked at some point; it was definitely intended
> to.
> 
> We do have a diamond inheritance situation here, where UdpTransport would
> inherit from PacketTransportInternal and UdpTransportInterface, which both
> inherit from PacketTransportInterface. But I think we can solve that with
> virtual inheritance? This worked in my local build; I just scheduled a try run
> which is green so far: https://codereview.webrtc.org/2806463003/

Done.

https://codereview.webrtc.org/2805783002/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:66: RtcpParameters GetRtcpParameters() const override;
On 2017/04/07 04:16:03, Taylor Brandstetter wrote:
> Since these methods aren't used anywhere yet, can you add a TODO as a reminder
> of what needs to be done?

Done.

https://codereview.webrtc.org/2805783002/diff/120001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2805783002/diff/120001/webrtc/pc/rtptransport.c...
webrtc/pc/rtptransport.cc:38:
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
On 2017/04/10 19:19:08, Taylor Brandstetter wrote:
> I've been using "INVALID_STATE" for situations where a parameter is valid
under
> some circumstances, but not with the object in its current state.

Done.

[Zach Stein, 2017-04-10 22:37:50]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-04-10 22:37:50]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2805783002/#ps140001 (title:
"Use INVALID_STATE instead of INVALID_PARAMETER.")

[commit-bot: I haz the power, 2017-04-10 22:37:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-10 23:17:57]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491863870652410,
"parent_rev": "0f92c796b481c7d7040b21638e709392f059005c", "commit_rev":
"d9ce76444f98b7abdfcacb0e5fbdb278163e8c07"}

[commit-bot: I haz the power, 2017-04-10 23:18:01]

Description was changed from

==========
Make RtpTransport actually implement RtpTransportInterface

BUG=webrtc:7013
==========

to

==========
Make RtpTransport actually implement RtpTransportInterface

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2805783002
Cr-Commit-Position: refs/heads/master@{#17628}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d9ce76444f98b7abdfcacb0e5...
==========

[commit-bot: I haz the power, 2017-04-10 23:18:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/d9ce76444f98b7abdfcacb0e5...