Introduce RtpTransportInternal and SrtpTransport.

Introduce RtpTransportInternal and SrtpTransport.

SrtpTransport currently just delegates everything to RtpTransport.
Also makes BaseChannel::rtp_transport_ an RtpTransportInternal and constructs an SrtpTransport if srtp is required.

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

[Zach Stein, 2017-07-14 20:06:12]

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

[Zach Stein, 2017-07-14 20:06:13]

PTAL

[Taylor Brandstetter, 2017-07-18 22:17:53]

I like the concept of breaking the "add new level of inheritance" and "add
adapter methods" steps into a separate CL.

One idea I just thought of, though: instead of this CL introducing a skeleton
"SrtpTransport" class, what about introducing an "RtpTransportAdapter" class,
which would be a helper class with the sole responsibility of forwarding method
calls and signals along? This could be used by both SrtpTransport (wrapping
RtpTransport) and later DtlsSrtpTransport (wrapping SrtpTransport), reducing
some code duplication and making those classes' implementations cleaner, just
containing the unique stuff they're responsible for. This pattern is already
used in one place; see AsyncSocketAdapter:
https://cs.chromium.org/chromium/src/third_party/webrtc/rtc_base/asyncsocket....

If you don't like this idea, or have better plans, that's fine; just thought I'd
throw it out there.

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/rtptransportinternal.h
File webrtc/pc/rtptransportinternal.h (right):

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/rtptransportinterna...
webrtc/pc/rtptransportinternal.h:25: class RtpTransportInternal : public
RtpTransportInterface,
A brief comment summarizing this class would be nice; for example, "This
represents the internal interface beneath RtpTransportInterface; it's not
accessible to API consumers but is accessible to internal classes in order to
send/receive RTP/RTCP packets, belonging to a single RTP session."

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/srtptransport.h
File webrtc/pc/srtptransport.h (right):

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/srtptransport.h#new...
webrtc/pc/srtptransport.h:23: 
Can you leave a comment summarizing the current state of this class, so that if
someone encounters it before we've filled anything in they're not confused?

[Taylor Brandstetter, 2017-07-18 22:19:34]

On 2017/07/18 22:17:53, Taylor Brandstetter wrote:
> what about introducing an "RtpTransportAdapter" class

I just realized we already have a class named that, which is doing something
entirely different. Maybe some renaming is in order (if you like the idea).

[Zach Stein, 2017-07-18 23:26:15]

On 2017/07/18 22:17:53, Taylor Brandstetter wrote:
> I like the concept of breaking the "add new level of inheritance" and "add
> adapter methods" steps into a separate CL.
> 
> One idea I just thought of, though: instead of this CL introducing a skeleton
> "SrtpTransport" class, what about introducing an "RtpTransportAdapter" class,
> which would be a helper class with the sole responsibility of forwarding
method
> calls and signals along? This could be used by both SrtpTransport (wrapping
> RtpTransport) and later DtlsSrtpTransport (wrapping SrtpTransport), reducing
> some code duplication and making those classes' implementations cleaner, just
> containing the unique stuff they're responsible for. This pattern is already
> used in one place; see AsyncSocketAdapter:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/rtc_base/asyncsocket....
> 
> If you don't like this idea, or have better plans, that's fine; just thought
I'd
> throw it out there.

I like this idea, but would rather wait to implement it until we are ready to
implement DtlsSrtpTransport.

[Zach Stein, 2017-07-18 23:26:31]

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/rtptransportinternal.h
File webrtc/pc/rtptransportinternal.h (right):

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/rtptransportinterna...
webrtc/pc/rtptransportinternal.h:25: class RtpTransportInternal : public
RtpTransportInterface,
On 2017/07/18 22:17:53, Taylor Brandstetter wrote:
> A brief comment summarizing this class would be nice; for example, "This
> represents the internal interface beneath RtpTransportInterface; it's not
> accessible to API consumers but is accessible to internal classes in order to
> send/receive RTP/RTCP packets, belonging to a single RTP session."

Done.

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/srtptransport.h
File webrtc/pc/srtptransport.h (right):

https://codereview.webrtc.org/2981013002/diff/1/webrtc/pc/srtptransport.h#new...
webrtc/pc/srtptransport.h:23: 
On 2017/07/18 22:17:53, Taylor Brandstetter wrote:
> Can you leave a comment summarizing the current state of this class, so that
if
> someone encounters it before we've filled anything in they're not confused?

Done.

[Taylor Brandstetter, 2017-07-19 01:37:49]

On 2017/07/18 23:26:15, Zach Stein wrote:
> On 2017/07/18 22:17:53, Taylor Brandstetter wrote:
> > I like the concept of breaking the "add new level of inheritance" and "add
> > adapter methods" steps into a separate CL.
> > 
> > One idea I just thought of, though: instead of this CL introducing a
skeleton
> > "SrtpTransport" class, what about introducing an "RtpTransportAdapter"
class,
> > which would be a helper class with the sole responsibility of forwarding
> method
> > calls and signals along? This could be used by both SrtpTransport (wrapping
> > RtpTransport) and later DtlsSrtpTransport (wrapping SrtpTransport), reducing
> > some code duplication and making those classes' implementations cleaner,
just
> > containing the unique stuff they're responsible for. This pattern is already
> > used in one place; see AsyncSocketAdapter:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/rtc_base/asyncsocket....
> > 
> > If you don't like this idea, or have better plans, that's fine; just thought
> I'd
> > throw it out there.
> 
> I like this idea, but would rather wait to implement it until we are ready to
> implement DtlsSrtpTransport.

Fair enough. lgtm then.

[Zach Stein, 2017-07-19 18:09:22]

Description was changed from

==========
Introduce RtpTransportInternal and SrtpTransport.

SrtpTransport currently just delegates everything to RtpTransport.
Make BaseChannel::rtp_transport_ an RtpTransportInternal and use SrtpTransport
if srtp is required.

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

to

==========
Introduce RtpTransportInternal and SrtpTransport.

SrtpTransport currently just delegates everything to RtpTransport.
Also makes BaseChannel::rtp_transport_ an RtpTransportInternal and constructs an
SrtpTransport if srtp is required.

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

[Zach Stein, 2017-07-19 18:09:41]

The CQ bit was checked by zstein@webrtc.org

[commit-bot: I haz the power, 2017-07-19 18:09:47]

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-07-19 18:12:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-19 18:12:18]

Try jobs failed on following builders:
  linux32_arm_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux32_arm_dbg/builds/...)
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/27915)

[Zach Stein, 2017-07-19 18:50:13]

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

[commit-bot: I haz the power, 2017-07-19 18:50:16]

Dry run: 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-07-19 19:21:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-19 19:21:47]

Dry run: This issue passed the CQ dry run.

[Zach Stein, 2017-07-19 20:34:27]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-07-19 20:34:28]

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/2981013002/#ps40001 (title:
"Depend on test:test_support for gmock.")

[commit-bot: I haz the power, 2017-07-19 20:34:40]

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-07-19 20:38:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500496467519590,
"parent_rev": "f6a861ab6c767cfb9579f43cba7f5e55a09e3848", "commit_rev":
"398c3fd6c220531a05315dfe3ce871ec64686402"}

[commit-bot: I haz the power, 2017-07-19 20:38:14]

Description was changed from

==========
Introduce RtpTransportInternal and SrtpTransport.

SrtpTransport currently just delegates everything to RtpTransport.
Also makes BaseChannel::rtp_transport_ an RtpTransportInternal and constructs an
SrtpTransport if srtp is required.

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

to

==========
Introduce RtpTransportInternal and SrtpTransport.

SrtpTransport currently just delegates everything to RtpTransport.
Also makes BaseChannel::rtp_transport_ an RtpTransportInternal and constructs an
SrtpTransport if srtp is required.

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2981013002
Cr-Commit-Position: refs/heads/master@{#19095}
Committed:
https://chromium.googlesource.com/external/webrtc/+/398c3fd6c220531a05315dfe3...
==========

[commit-bot: I haz the power, 2017-07-19 20:38:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/398c3fd6c220531a05315dfe3...