Allow mocking SendSideCongestionController for Call tests.

This will allow me to test that Call invokes SendSideCongestionController::SetBweBitrates as expected (for https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is changed to take a SendSideCongestionController to enable injecting the mock.

BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2834663003
Cr-Commit-Position: refs/heads/master@{#18055}
Committed: https://chromium.googlesource.com/external/webrtc/+/7cb69d5cc71a496b0e9372001ddd22f471d4d076

[Zach Stein, 2017-04-25 23:08:31]

Description was changed from

==========
Remove Observer* argument from SendSideCongestionController constructor.

BUG=NONE
==========

to

==========
This will allow me to test that Call invokes
SendSideCongestionController::SetBweBitrates as expected (for
https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is
changed to take a SendSideCongestionController to enable injecting the mock.

BUG=NONE
==========

[Zach Stein, 2017-04-25 23:08:31]

zstein@webrtc.org changed reviewers:
+ mflodman@webrtc.org, nisse@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[Zach Stein, 2017-04-25 23:14:55]

Description was changed from

==========
This will allow me to test that Call invokes
SendSideCongestionController::SetBweBitrates as expected (for
https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is
changed to take a SendSideCongestionController to enable injecting the mock.

BUG=NONE
==========

to

==========
This will allow me to test that Call invokes
SendSideCongestionController::SetBweBitrates as expected (for
https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is
changed to take a SendSideCongestionController to enable injecting the mock.

BUG=webrtc:7395
==========

[Zach Stein, 2017-04-25 23:16:31]

PTAL. I noticed some of these files changed recently, so let me know if I am
getting in anyone's way or missing a better approach! Thanks.

[nisse-webrtc, 2017-04-26 07:37:15]

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/BUILD.gn (right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/BUILD.gn:104:
rtc_source_set("mock_send_side_congestion_controller") {
Are there any naming conventions for this type of targets? 

Might be good to generalize it a little, and include mock_congestion_observer.h
too (currently not included in any gn target, it seems). It's used by
webrtc/audio/audio_send_stream_unittest.cc and probably by some downstream tests
too.

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:53:
Observer* /* observer */,
I think it's best to keep this argument. We plan to delete this all-in-one class
fairly soon, in favor of the receive side and send side classes, but it's kept
for now to not break downstream projects.

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/mock/mock_send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/mock/mock_send_side_congestion_controller.h:28:
MOCK_METHOD3(SetBweBitrates,
It makes some sense to me to add only mock methods that tests actually use, but
I'm not sure if that's how we usually do things? Is it common practice to mock
only one or a few methods of a large interface like this?

I've been bitten by crashes when a mock class was missing an implementation of a
certain method, and the test crashed because the other methods of the mock
object didn't set up the proper state required for the real implementation of
that method.

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(left):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:60:
// TODO(nisse): Consider deleting the |observer| argument to constructors.
I'm afraid this has to wait until the old CongestionController class is deleted.
Sorry the comment didn't explain that.

[Zach Stein, 2017-04-26 20:39:53]

Thanks for the feedback nisse!

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/BUILD.gn (right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/BUILD.gn:104:
rtc_source_set("mock_send_side_congestion_controller") {
On 2017/04/26 07:37:14, nisse-webrtc wrote:
> Are there any naming conventions for this type of targets? 
> 
> Might be good to generalize it a little, and include
mock_congestion_observer.h
> too (currently not included in any gn target, it seems). It's used by
> webrtc/audio/audio_send_stream_unittest.cc and probably by some downstream
tests
> too.

Not sure about naming conventions. I was copying what the
mock_remote_bitrate_observer target does, but it looks unusual to me too.

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:53:
Observer* /* observer */,
On 2017/04/26 07:37:14, nisse-webrtc wrote:
> I think it's best to keep this argument. We plan to delete this all-in-one
class
> fairly soon, in favor of the receive side and send side classes, but it's kept
> for now to not break downstream projects. 

What downstream projects might this break? I'm not seeing anything worrying in
code search, but I might not be looking in the right places. Or perhaps this is
more public than I am assuming?

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/mock/mock_send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/mock/mock_send_side_congestion_controller.h:28:
MOCK_METHOD3(SetBweBitrates,
On 2017/04/26 07:37:14, nisse-webrtc wrote:
> It makes some sense to me to add only mock methods that tests actually use,
but
> I'm not sure if that's how we usually do things? Is it common practice to mock
> only one or a few methods of a large interface like this?
> 
> I've been bitten by crashes when a mock class was missing an implementation of
a
> certain method, and the test crashed because the other methods of the mock
> object didn't set up the proper state required for the real implementation of
> that method.

I'm not sure what we usually do - this is my first time trying to introduce a
mock :) I just provided the minimal implementation for the tests I need.

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(left):

https://codereview.webrtc.org/2834663003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:60:
// TODO(nisse): Consider deleting the |observer| argument to constructors.
On 2017/04/26 07:37:14, nisse-webrtc wrote:
> I'm afraid this has to wait until the old CongestionController class is
deleted.
> Sorry the comment didn't explain that.

I'll see what I can get working without this change.

[Stefan, 2017-05-01 11:17:05]

holmer@chromium.org changed reviewers:
+ holmer@chromium.org

[Stefan, 2017-05-01 11:17:06]

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/call.h#newcod...
webrtc/call/call.h:103: // Allows mocking |transport_send| for testing.
I think this is what we want the interface to be in the end, so perhaps this
comment isn't needed?

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
File webrtc/call/fake_rtp_transport_controller_send.h (right):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
webrtc/call/fake_rtp_transport_controller_send.h:20: class
FakeRtpTransportController : public RtpTransportControllerSendInterface {
FakeRtpTransportControllerSend?

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
webrtc/call/fake_rtp_transport_controller_send.h:33: TransportFeedbackObserver*
transport_feedback_observer() override {
Empty line above for consistency

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.h (left):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.h:30:
SendSideCongestionController::Observer* observer);
I think we have to decide if we want send_side_cc to be exposed, or if we should
only expose the necessary methods on RtpTransportControllerSend? This is a move
towards the former. As I see it, the RtpTransportSenders will talk to the
CongestionController by queueing packets to be sent etc, so the user of
RtpTransportControllerSend shouldn't have to know about it. That may be an
argument for not exposing it at all from RtpTransportControllerSend?

Nisse, what's your opinion?

[nisse-webrtc, 2017-05-02 07:01:30]

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/call.h#newcod...
webrtc/call/call.h:103: // Allows mocking |transport_send| for testing.
On 2017/05/01 11:17:06, Stefan wrote:
> I think this is what we want the interface to be in the end

Not quite. My plan is that RtpTransportController should be owned by
WebRtcSession/Peerconnection.

Call should get two pointers, one for audio and one for video (to support
unbundled), which in the common, bundled, case would be pointers to the same
object.

And I think it would improve consistency to put these pointers inside the Config
struct, instead of adding additional arguments.

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.h (left):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.h:30:
SendSideCongestionController::Observer* observer);
On 2017/05/01 11:17:06, Stefan wrote:
> I think we have to decide if we want send_side_cc to be exposed, or if we
should
> only expose the necessary methods on RtpTransportControllerSend? This is a
move
> towards the former. As I see it, the RtpTransportSenders will talk to the
> CongestionController by queueing packets to be sent etc, so the user of
> RtpTransportControllerSend shouldn't have to know about it. That may be an
> argument for not exposing it at all from RtpTransportControllerSend?
> 
> Nisse, what's your opinion?

It's not clear to me how this will look in the end, but Call implements this
observer interface.

Also note that this is the implementation class. For the corresponding interface
(intended for use by send streams), it currently has a send_side_cc accessor,
which I think we should aim to delete.

[Zach Stein, 2017-05-04 21:30:02]

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
File webrtc/call/fake_rtp_transport_controller_send.h (right):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
webrtc/call/fake_rtp_transport_controller_send.h:20: class
FakeRtpTransportController : public RtpTransportControllerSendInterface {
On 2017/05/01 11:17:06, Stefan wrote:
> FakeRtpTransportControllerSend?

Done.

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/fake_rtp_tran...
webrtc/call/fake_rtp_transport_controller_send.h:33: TransportFeedbackObserver*
transport_feedback_observer() override {
On 2017/05/01 11:17:06, Stefan wrote:
> Empty line above for consistency

Done.

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.h (left):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.h:30:
SendSideCongestionController::Observer* observer);
On 2017/05/02 07:01:29, nisse-webrtc wrote:
> On 2017/05/01 11:17:06, Stefan wrote:
> > I think we have to decide if we want send_side_cc to be exposed, or if we
> should
> > only expose the necessary methods on RtpTransportControllerSend? This is a
> move
> > towards the former. As I see it, the RtpTransportSenders will talk to the
> > CongestionController by queueing packets to be sent etc, so the user of
> > RtpTransportControllerSend shouldn't have to know about it. That may be an
> > argument for not exposing it at all from RtpTransportControllerSend?
> > 
> > Nisse, what's your opinion?
> 
> It's not clear to me how this will look in the end, but Call implements this
> observer interface.
> 
> Also note that this is the implementation class. For the corresponding
interface
> (intended for use by send streams), it currently has a send_side_cc accessor,
> which I think we should aim to delete.

RegisterNetworkObserver would need to be added to
RtpTransportControllerSendInterface if send_side_cc was removed for this change
to work (Call's constructor takes the interface and currently registers the
network observer by accessing the send_side_cc). Should I go ahead and do that?

[nisse-webrtc, 2017-05-05 07:12:38]

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.h (left):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.h:30:
SendSideCongestionController::Observer* observer);
On 2017/05/04 21:30:02, Zach Stein wrote:
> On 2017/05/02 07:01:29, nisse-webrtc wrote:
> > On 2017/05/01 11:17:06, Stefan wrote:
> > > I think we have to decide if we want send_side_cc to be exposed, or if we
> > should
> > > only expose the necessary methods on RtpTransportControllerSend? This is a
> > move
> > > towards the former. As I see it, the RtpTransportSenders will talk to the
> > > CongestionController by queueing packets to be sent etc, so the user of
> > > RtpTransportControllerSend shouldn't have to know about it. That may be an
> > > argument for not exposing it at all from RtpTransportControllerSend?
> > > 
> > > Nisse, what's your opinion?
> > 
> > It's not clear to me how this will look in the end, but Call implements this
> > observer interface.
> > 
> > Also note that this is the implementation class. For the corresponding
> interface
> > (intended for use by send streams), it currently has a send_side_cc
accessor,
> > which I think we should aim to delete.
> 
> RegisterNetworkObserver would need to be added to
> RtpTransportControllerSendInterface if send_side_cc was removed for this
change
> to work (Call's constructor takes the interface and currently registers the
> network observer by accessing the send_side_cc). Should I go ahead and do
that?

No, I think it's better to leave send_side_cc() in the interface for the time
being.

https://codereview.webrtc.org/2834663003/diff/70001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/BUILD.gn (right):

https://codereview.webrtc.org/2834663003/diff/70001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/BUILD.gn:108:
"include/mock/mock_send_side_congestion_controller.h",
I still think it makes sense with a target including both this and
mock_congestion_observer.h. Maybe target name "mock_congestion_controller" is
good enough.

[Zach Stein, 2017-05-05 23:28:13]

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.h (left):

https://codereview.webrtc.org/2834663003/diff/50001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.h:30:
SendSideCongestionController::Observer* observer);
On 2017/05/05 07:12:38, nisse-webrtc wrote:
> On 2017/05/04 21:30:02, Zach Stein wrote:
> > On 2017/05/02 07:01:29, nisse-webrtc wrote:
> > > On 2017/05/01 11:17:06, Stefan wrote:
> > > > I think we have to decide if we want send_side_cc to be exposed, or if
we
> > > should
> > > > only expose the necessary methods on RtpTransportControllerSend? This is
a
> > > move
> > > > towards the former. As I see it, the RtpTransportSenders will talk to
the
> > > > CongestionController by queueing packets to be sent etc, so the user of
> > > > RtpTransportControllerSend shouldn't have to know about it. That may be
an
> > > > argument for not exposing it at all from RtpTransportControllerSend?
> > > > 
> > > > Nisse, what's your opinion?
> > > 
> > > It's not clear to me how this will look in the end, but Call implements
this
> > > observer interface.
> > > 
> > > Also note that this is the implementation class. For the corresponding
> > interface
> > > (intended for use by send streams), it currently has a send_side_cc
> accessor,
> > > which I think we should aim to delete.
> > 
> > RegisterNetworkObserver would need to be added to
> > RtpTransportControllerSendInterface if send_side_cc was removed for this
> change
> > to work (Call's constructor takes the interface and currently registers the
> > network observer by accessing the send_side_cc). Should I go ahead and do
> that?
> 
> No, I think it's better to leave send_side_cc() in the interface for the time
> being.

Acknowledged.

https://codereview.webrtc.org/2834663003/diff/70001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/BUILD.gn (right):

https://codereview.webrtc.org/2834663003/diff/70001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/BUILD.gn:108:
"include/mock/mock_send_side_congestion_controller.h",
On 2017/05/05 07:12:38, nisse-webrtc wrote:
> I still think it makes sense with a target including both this and
> mock_congestion_observer.h. Maybe target name "mock_congestion_controller" is
> good enough.

Done.

[stefan-webrtc, 2017-05-06 06:27:45]

lgtm

[nisse-webrtc, 2017-05-08 08:45:22]

lgtm

[Zach Stein, 2017-05-08 17:49:33]

The CQ bit was checked by zstein@webrtc.org

[commit-bot: I haz the power, 2017-05-08 17:49:43]

CQ is trying da patch.

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

[Zach Stein, 2017-05-08 17:51:07]

The CQ bit was unchecked by zstein@webrtc.org

[Zach Stein, 2017-05-08 18:00:29]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-05-08 18:00:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2834663003/#ps110001 (title:
"rebase")

[commit-bot: I haz the power, 2017-05-08 18:00:37]

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-05-08 18:52:39]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1494266429109240,
"parent_rev": "966963ac7d62bdac051eacae234495fd355919e6", "commit_rev":
"7cb69d5cc71a496b0e9372001ddd22f471d4d076"}

[commit-bot: I haz the power, 2017-05-08 18:52:42]

Description was changed from

==========
This will allow me to test that Call invokes
SendSideCongestionController::SetBweBitrates as expected (for
https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is
changed to take a SendSideCongestionController to enable injecting the mock.

BUG=webrtc:7395
==========

to

==========
This will allow me to test that Call invokes
SendSideCongestionController::SetBweBitrates as expected (for
https://codereview.chromium.org/2793913008).

FakeRtpTransportController moves to a common header and its constructor is
changed to take a SendSideCongestionController to enable injecting the mock.

BUG=webrtc:7395

Review-Url: https://codereview.webrtc.org/2834663003
Cr-Commit-Position: refs/heads/master@{#18055}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7cb69d5cc71a496b0e9372001...
==========

[commit-bot: I haz the power, 2017-05-08 18:52:44]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/external/webrtc/+/7cb69d5cc71a496b0e9372001...