Only one implementation of MockRtpPacketSink once

Only one implementation of MockRtpPacketSink once

MockRtpPacketSink has three identical implementations now, so time to move it to its own file.

BUG=None

Review-Url: https://codereview.webrtc.org/2988853002
Cr-Commit-Position: refs/heads/master@{#19183}
Committed: https://chromium.googlesource.com/external/webrtc/+/e2173d9f0d2d991d576ffa71a18761af69b72ede

[eladalon, 2017-07-26 08:21:39]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[eladalon, 2017-07-26 08:21:40]

PTAL

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
File webrtc/call/rtp_packet_sink_interface_mock.h (right):

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
webrtc/call/rtp_packet_sink_interface_mock.h:1: /*
Please note that I've suffixed "mock" rather than prefix, to make sure this
appears close to the file it's mocking. Please let me know if this is counter
the style-guide.

[danilchap, 2017-07-26 08:38:13]

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/BUILD.gn#newcode42
webrtc/call/BUILD.gn:42: "rtp_packet_sink_interface_mock.h",
use own test target for mocks - rtp_interfaces shouldn't contain pure test file.

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
File webrtc/call/rtp_packet_sink_interface_mock.h (right):

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
webrtc/call/rtp_packet_sink_interface_mock.h:1: /*
On 2017/07/26 08:21:39, eladalon wrote:
> Please note that I've suffixed "mock" rather than prefix, to make sure this
> appears close to the file it's mocking. Please let me know if this is counter
> the style-guide.

in webrtc mocks usually go into own folder and use mock prefix rather than
suffix, i.e. I suggest to rename file to
webrtc/call/(mock|test)/mock_rtp_packet_sink_interface.h

[eladalon, 2017-07-26 09:01:06]

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/BUILD.gn#newcode42
webrtc/call/BUILD.gn:42: "rtp_packet_sink_interface_mock.h",
On 2017/07/26 08:38:13, danilchap wrote:
> use own test target for mocks - rtp_interfaces shouldn't contain pure test
file.

Done.

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
File webrtc/call/rtp_packet_sink_interface_mock.h (right):

https://codereview.webrtc.org/2988853002/diff/1/webrtc/call/rtp_packet_sink_i...
webrtc/call/rtp_packet_sink_interface_mock.h:1: /*
On 2017/07/26 08:38:13, danilchap wrote:
> On 2017/07/26 08:21:39, eladalon wrote:
> > Please note that I've suffixed "mock" rather than prefix, to make sure this
> > appears close to the file it's mocking. Please let me know if this is
counter
> > the style-guide.
> 
> in webrtc mocks usually go into own folder and use mock prefix rather than
> suffix, i.e. I suggest to rename file to
> webrtc/call/(mock|test)/mock_rtp_packet_sink_interface.h

Done.

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:154: ":mock_rtp_interfaces",
I find it odd that this didn't actually appear to be necessary.
1. Any idea why?
2. Shall we keep it, then?
3. rtp_video_stream_receiver_unittest.cc does not seem to depend on
mock_rtp_interfaces, which I also find odd.

[danilchap, 2017-07-26 10:28:59]

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:37: rtc_source_set("mock_rtp_interfaces") {
probably better to move inside if (rtc_include_tests) section below

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:38: sources = [
add
testonly = true

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:40: ]
add deps on rtp_interfaces and //testing/gmock
It is header-only dependencies, but still nicer to have direct dependencies
explicit.

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:154: ":mock_rtp_interfaces",
On 2017/07/26 09:01:05, eladalon wrote:
> I find it odd that this didn't actually appear to be necessary.
> 1. Any idea why?
> 2. Shall we keep it, then?
> 3. rtp_video_stream_receiver_unittest.cc does not seem to depend on
> mock_rtp_interfaces, which I also find odd.

Because we currently do not check that only headers from the dependent targets
are included and there is no code to link, so no linker error either.

It would be nice to eventually turn that feature on for stricter dependency
graph,
so yes, better keep it.

and better make other unittests that depend on the mock to explicitly depend on
this mock target too.

[eladalon, 2017-07-26 11:24:27]

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:37: rtc_source_set("mock_rtp_interfaces") {
On 2017/07/26 10:28:59, danilchap wrote:
> probably better to move inside if (rtc_include_tests) section below

Done.

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:38: sources = [
On 2017/07/26 10:28:59, danilchap wrote:
> add
> testonly = true

Done.

https://codereview.webrtc.org/2988853002/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:154: ":mock_rtp_interfaces",
On 2017/07/26 10:28:59, danilchap wrote:
> On 2017/07/26 09:01:05, eladalon wrote:
> > I find it odd that this didn't actually appear to be necessary.
> > 1. Any idea why?
> > 2. Shall we keep it, then?
> > 3. rtp_video_stream_receiver_unittest.cc does not seem to depend on
> > mock_rtp_interfaces, which I also find odd.
> 
> Because we currently do not check that only headers from the dependent targets
> are included and there is no code to link, so no linker error either.
> 
> It would be nice to eventually turn that feature on for stricter dependency
> graph,
> so yes, better keep it.
> 
> and better make other unittests that depend on the mock to explicitly depend
on
> this mock target too.

Done.

[danilchap, 2017-07-26 11:27:42]

lgtm

[eladalon, 2017-07-26 13:24:07]

eladalon@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[eladalon, 2017-07-26 13:24:08]

Adding Fredrick, in case he has time to owner-lgtm.

[the sun, 2017-07-27 07:42:38]

lgtm

https://codereview.webrtc.org/2988853002/diff/40001/webrtc/call/test/mock_rtp...
File webrtc/call/test/mock_rtp_packet_sink_interface.h (right):

https://codereview.webrtc.org/2988853002/diff/40001/webrtc/call/test/mock_rtp...
webrtc/call/test/mock_rtp_packet_sink_interface.h:19: class RtpPacketReceived;
nit: isn't this already forward declared or included in
rtp_packet_sink_interface.h? it should be

[eladalon, 2017-07-27 09:10:03]

https://codereview.webrtc.org/2988853002/diff/40001/webrtc/call/test/mock_rtp...
File webrtc/call/test/mock_rtp_packet_sink_interface.h (right):

https://codereview.webrtc.org/2988853002/diff/40001/webrtc/call/test/mock_rtp...
webrtc/call/test/mock_rtp_packet_sink_interface.h:19: class RtpPacketReceived;
On 2017/07/27 07:42:37, the sun wrote:
> nit: isn't this already forward declared or included in
> rtp_packet_sink_interface.h? it should be

Done.

[eladalon, 2017-07-28 16:40:25]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-07-28 16:40:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2988853002/#ps100001 (title:
".")

[commit-bot: I haz the power, 2017-07-28 16:40:30]

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-28 17:05:46]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1501260025630940,
"parent_rev": "9f0ef390f1d2126f2f35ce3ad75cac1d86b632bb", "commit_rev":
"e2173d9f0d2d991d576ffa71a18761af69b72ede"}

[commit-bot: I haz the power, 2017-07-28 17:05:51]

Description was changed from

==========
Only one implementation of MockRtpPacketSink once

MockRtpPacketSink has three identical implementations now, so time to move it to
its own file.

BUG=None
==========

to

==========
Only one implementation of MockRtpPacketSink once

MockRtpPacketSink has three identical implementations now, so time to move it to
its own file.

BUG=None

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

[commit-bot: I haz the power, 2017-07-28 17:05:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/e2173d9f0d2d991d576ffa71a...