NetEq: Dependency injection through one struct instead of many params

NetEq: Dependency injection through one struct instead of many params

With this change, the NetEqImpl constructor takes a struct
(NetEqImpl::Dependencies) as input instead of a collection of
individual dependencies. The NetEqImpl unit test fixture is modified
to make better used of unique_ptrs.

Committed: https://crrev.com/1d9061e8ae00a7150949810b006f8c84d13cbadf
Cr-Commit-Position: refs/heads/master@{#12514}

[hlundin-webrtc, 2016-04-26 11:38:10]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-04-26 11:39:16]

Patchset #1 (id:20001) has been deleted

[hlundin-webrtc, 2016-04-26 11:41:10]

Patchset #1 (id:40001) has been deleted

[hlundin-webrtc, 2016-04-26 11:41:30]

henrik.lundin@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[hlundin-webrtc, 2016-04-26 11:41:31]

kwiberg, PTAL.

Thanks!

[kwiberg-webrtc, 2016-04-26 13:00:04]

lgtm with some nits

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq.cc:36: return new NetEqImpl(config,
std::move(deps));
Just

  return new NetEqImpl(config, NetEqImpl::Dependencies(config));

Also, NetEq::Create is a good example of a function that ought to be returning a
unique_ptr. Not in this CL, though...

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:68: timestamp_scaler(new
TimestampScaler(*decoder_database.get())),
Just "*decoder_database" should do the same thing.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.h (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.h:94: // injected dependencies, and
will delete them when done.
You no longer need the second sentence in this comment; the use of unique_ptrs
makes it obvious that this is the case.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:57: NetEqImplTest() :
config_() { config_.sample_rate_hz = 8000; }
"config_()" is unnecessary; a call to the default constructor is what will
happen anyway if you don't say anything.

[hlundin-webrtc, 2016-04-26 13:19:38]

Updates after review

[hlundin-webrtc, 2016-04-26 13:20:38]

Thanks! I will commit the revised patch set.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq.cc:36: return new NetEqImpl(config,
std::move(deps));
On 2016/04/26 13:00:04, kwiberg-webrtc wrote:
> Just
> 
>   return new NetEqImpl(config, NetEqImpl::Dependencies(config));
> 
> Also, NetEq::Create is a good example of a function that ought to be returning
a
> unique_ptr. Not in this CL, though...

Done.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:68: timestamp_scaler(new
TimestampScaler(*decoder_database.get())),
On 2016/04/26 13:00:04, kwiberg-webrtc wrote:
> Just "*decoder_database" should do the same thing.

Done.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.h (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.h:94: // injected dependencies, and
will delete them when done.
On 2016/04/26 13:00:04, kwiberg-webrtc wrote:
> You no longer need the second sentence in this comment; the use of unique_ptrs
> makes it obvious that this is the case.

Done.

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1921243002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:57: NetEqImplTest() :
config_() { config_.sample_rate_hz = 8000; }
On 2016/04/26 13:00:04, kwiberg-webrtc wrote:
> "config_()" is unnecessary; a call to the default constructor is what will
> happen anyway if you don't say anything.

Done.

[hlundin-webrtc, 2016-04-26 18:14:57]

Rebase, with conflicts

[hlundin-webrtc, 2016-04-26 18:17:08]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-04-26 18:17:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1921243002/#ps100001 (title:
"Rebase, with conflicts")

[commit-bot: I haz the power, 2016-04-26 18:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921243002/100001

[commit-bot: I haz the power, 2016-04-26 19:19:40]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-04-26 19:19:47]

Description was changed from

==========
NetEq: Dependency injection through one struct instead of many params

With this change, the NetEqImpl constructor takes a struct
(NetEqImpl::Dependencies) as input instead of a collection of
individual dependencies. The NetEqImpl unit test fixture is modified
to make better used of unique_ptrs.
==========

to

==========
NetEq: Dependency injection through one struct instead of many params

With this change, the NetEqImpl constructor takes a struct
(NetEqImpl::Dependencies) as input instead of a collection of
individual dependencies. The NetEqImpl unit test fixture is modified
to make better used of unique_ptrs.

Committed: https://crrev.com/1d9061e8ae00a7150949810b006f8c84d13cbadf
Cr-Commit-Position: refs/heads/master@{#12514}
==========

[commit-bot: I haz the power, 2016-04-26 19:19:48]

Patchset 3 (id:??) landed as
https://crrev.com/1d9061e8ae00a7150949810b006f8c84d13cbadf
Cr-Commit-Position: refs/heads/master@{#12514}

[hlundin-webrtc, 2016-04-26 19:35:47]

On 2016/04/26 19:19:48, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/1d9061e8ae00a7150949810b006f8c84d13cbadf
> Cr-Commit-Position: refs/heads/master@{#12514}

A correction to this CL was uploaded at
https://codereview.webrtc.org/1920333002.