Test submission of complete AEC-dump refactoring.

Test submission of complete AEC-dump refactoring.

DO NOT SUBMIT.

It's only up here for running the tests.

BUG=webrtc:6261

[aleloi, 2017-03-16 16:02:42]

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

[commit-bot: I haz the power, 2017-03-16 16:02:50]

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

[aleloi, 2017-03-16 16:11:26]

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

[commit-bot: I haz the power, 2017-03-16 16:11:38]

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-03-16 16:16:07]

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

[commit-bot: I haz the power, 2017-03-16 16:16:08]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15013)

[aleloi, 2017-03-16 16:44:03]

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

[commit-bot: I haz the power, 2017-03-16 16:44:08]

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-03-16 16:49:02]

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

[commit-bot: I haz the power, 2017-03-16 16:49:03]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15016)

[aleloi, 2017-03-17 09:06:42]

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

[commit-bot: I haz the power, 2017-03-17 09:06:52]

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-03-17 09:11:02]

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

[commit-bot: I haz the power, 2017-03-17 09:11:03]

Dry run: Try jobs failed on following builders:
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22748)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/24403)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/7439)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/23823)

[aleloi, 2017-03-17 13:03:47]

Patchset #3 (id:40001) has been deleted

[aleloi, 2017-03-17 13:03:56]

Patchset #1 (id:1) has been deleted

[aleloi, 2017-03-17 13:04:03]

Patchset #1 (id:20001) has been deleted

[aleloi, 2017-03-17 13:04:13]

Patchset #1 (id:60001) has been deleted

[aleloi, 2017-03-20 14:27:28]

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

[commit-bot: I haz the power, 2017-03-20 14:27:39]

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

[aleloi, 2017-03-20 14:30:42]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-20 14:31:49]

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

[commit-bot: I haz the power, 2017-03-20 14:31:50]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15128)

[aleloi, 2017-03-20 14:42:36]

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

[commit-bot: I haz the power, 2017-03-20 14:42:45]

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-03-20 14:47:14]

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

[commit-bot: I haz the power, 2017-03-20 14:47:15]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15131)

[aleloi, 2017-03-20 14:56:45]

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

[commit-bot: I haz the power, 2017-03-20 14:56:55]

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-03-20 15:01:05]

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

[commit-bot: I haz the power, 2017-03-20 15:01:06]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15134)

[aleloi, 2017-03-20 15:25:37]

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

[commit-bot: I haz the power, 2017-03-20 15:25:42]

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

[aleloi, 2017-03-20 15:27:57]

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

[commit-bot: I haz the power, 2017-03-20 15:28:02]

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-03-20 15:32:39]

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

[commit-bot: I haz the power, 2017-03-20 15:32:40]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15138)

[aleloi, 2017-03-20 15:56:19]

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

[commit-bot: I haz the power, 2017-03-20 15:56:29]

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-03-20 16:01:01]

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

[commit-bot: I haz the power, 2017-03-20 16:01:02]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15140)

[aleloi, 2017-03-20 16:36:11]

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

[commit-bot: I haz the power, 2017-03-20 16:36:20]

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-03-20 16:40:50]

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

[commit-bot: I haz the power, 2017-03-20 16:40:51]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15143)

[aleloi, 2017-03-21 09:13:23]

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

[commit-bot: I haz the power, 2017-03-21 09:13:27]

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-03-21 09:17:36]

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

[commit-bot: I haz the power, 2017-03-21 09:17:37]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15167)

[aleloi, 2017-03-21 09:24:29]

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

[commit-bot: I haz the power, 2017-03-21 09:24:36]

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-03-21 09:28:46]

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

[commit-bot: I haz the power, 2017-03-21 09:28:47]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15168)

[aleloi, 2017-03-21 16:19:30]

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

[commit-bot: I haz the power, 2017-03-21 16:19:38]

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-03-21 16:21:29]

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

[commit-bot: I haz the power, 2017-03-21 16:21:29]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24078)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22873)

[aleloi, 2017-03-22 09:03:43]

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

[commit-bot: I haz the power, 2017-03-22 09:03:52]

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-03-22 09:05:41]

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

[commit-bot: I haz the power, 2017-03-22 09:05:42]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18810)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18610)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24095)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22890)

[aleloi, 2017-03-22 09:09:06]

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

[commit-bot: I haz the power, 2017-03-22 09:09:11]

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-03-22 09:12:33]

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

[commit-bot: I haz the power, 2017-03-22 09:12:34]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)

[aleloi, 2017-03-22 09:22:07]

Patchset #12 (id:320001) has been deleted

[aleloi, 2017-03-22 09:25:17]

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

[commit-bot: I haz the power, 2017-03-22 09:25:25]

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-03-22 09:30:13]

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

[commit-bot: I haz the power, 2017-03-22 09:30:15]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15219)

[aleloi, 2017-03-22 09:55:04]

Description was changed from

==========
Test submission of complete AEC-dump refactoring.

DO NOT SUBMIT.

It's only up here for running the tests.

BUG=
==========

to

==========
Test submission of complete AEC-dump refactoring.

DO NOT SUBMIT.

It's only up here for running the tests.

BUG=6261
==========

[aleloi, 2017-03-22 09:55:25]

Description was changed from

==========
Test submission of complete AEC-dump refactoring.

DO NOT SUBMIT.

It's only up here for running the tests.

BUG=6261
==========

to

==========
Test submission of complete AEC-dump refactoring.

DO NOT SUBMIT.

It's only up here for running the tests.

BUG=webrtc:6261
==========

[aleloi, 2017-03-22 10:21:47]

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

[commit-bot: I haz the power, 2017-03-22 10:21:53]

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-03-22 10:32:47]

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

[commit-bot: I haz the power, 2017-03-22 10:32:49]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[aleloi, 2017-03-22 10:46:22]

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

[commit-bot: I haz the power, 2017-03-22 10:46:28]

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-03-22 11:10:52]

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

[commit-bot: I haz the power, 2017-03-22 11:10:53]

Dry run: This issue passed the CQ dry run.

[aleloi, 2017-03-27 11:07:10]

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

[commit-bot: I haz the power, 2017-03-27 11:07:17]

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-03-27 11:10:26]

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

[commit-bot: I haz the power, 2017-03-27 11:10:27]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18939)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24227)

[aleloi, 2017-03-27 11:22:51]

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

[commit-bot: I haz the power, 2017-03-27 11:22:56]

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-03-27 11:33:33]

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

[commit-bot: I haz the power, 2017-03-27 11:33:35]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[aleloi, 2017-03-27 12:33:24]

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

[commit-bot: I haz the power, 2017-03-27 12:33:31]

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-03-27 13:05:47]

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

[commit-bot: I haz the power, 2017-03-27 13:05:48]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-03-28 08:54:56]

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

[kwiberg-webrtc, 2017-03-28 08:54:59]

Some drive-by comments on the interface. (I realized after writing the comments
that they should probably have been on https://codereview.webrtc.org/2778783002/
instead, but I'm too lazy to move them and it looks like the two CLs have pretty
much identical versions of aec_dumper.h anyway.)

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right):

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:37:
InternalAPMConfig(InternalAPMConfig&);
Is this intended to be a copy constructor? In that case, you want const T&.
Also, since the struct contains a std::string, consider also adding a move
constructor.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: inline
InternalAPMConfig::~InternalAPMConfig() = default;
Either drop the "inline" and move these lines to the .cc file, or just write
"=default" on the lines in the class declaration. (But the second option is
equivalent to simply not writing the lines even in the class declaration, so in
that case just drop them.)

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:76: CaptureStreamInfo()
= default;
This is a pure interface, so don't try to add a constructor. It won't do
anything...

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:81: const
std::vector<rtc::ArrayView<const float>> src) = 0;
Drop the outermost "const" here. You're simply taking the vectors by value.

Would a typical implementation actually std::move the passed vectors and save
them (as opposed to just reading from it)? If not, consider making the outer
layer an ArrayView too.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:92: AecDumper() =
default;
Again: pure interface, so no need to even mention a constructor.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:114: static
std::unique_ptr<AecDumper> CreateNullDumper();
For modularity, consider having the interface and the create functions in
different build targets. That way, it becomes possible to implement the
interface without having to link with the create functions. (It's totally fine
to have a .h file that has *just* these three functions in it.)

It's fine to keep them here, too, *if* it's acceptable that everyone that
implements the interface has to link with the implementations of the create
functions.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:127: const
std::vector<rtc::ArrayView<const float>> src) = 0;
The comment for AddInput/AddOutput applies here too.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:135: virtual void
WriteConfig(InternalAPMConfig config, bool forced) = 0;
InternalAPMConfig is somewhat big, so isn't exactly free even to move. Pass by
const ref?

[aleloi, 2017-03-29 08:41:31]

Thanks for the comments, Karl!

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right):

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:37:
InternalAPMConfig(InternalAPMConfig&);
On 2017/03/28 08:54:58, kwiberg-webrtc wrote:
> Is this intended to be a copy constructor? In that case, you want const T&.
> Also, since the struct contains a std::string, consider also adding a move
> constructor.

I now pass by const ref and have deleted the other c-tors.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: inline
InternalAPMConfig::~InternalAPMConfig() = default;
On 2017/03/28 08:54:58, kwiberg-webrtc wrote:
> Either drop the "inline" and move these lines to the .cc file, or just write
> "=default" on the lines in the class declaration. (But the second option is
> equivalent to simply not writing the lines even in the class declaration, so
in
> that case just drop them.)

I've moved them to a cc file. I think the chrome style plugin thinks this is a
complex class (probably because of std::string) and requires definitions outside
class decl.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:76: CaptureStreamInfo()
= default;
On 2017/03/28 08:54:58, kwiberg-webrtc wrote:
> This is a pure interface, so don't try to add a constructor. It won't do
> anything...

Thanks! I've just read tip of the week #131. There should be a compiler warning
flag for when '= default' doesn't generate anything.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:81: const
std::vector<rtc::ArrayView<const float>> src) = 0;
On 2017/03/28 08:54:59, kwiberg-webrtc wrote:
> Drop the outermost "const" here. You're simply taking the vectors by value.
> 
> Would a typical implementation actually std::move the passed vectors and save
> them (as opposed to just reading from it)? If not, consider making the outer
> layer an ArrayView too.

Done.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:92: AecDumper() =
default;
On 2017/03/28 08:54:58, kwiberg-webrtc wrote:
> Again: pure interface, so no need to even mention a constructor.

Done.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:114: static
std::unique_ptr<AecDumper> CreateNullDumper();
The whole interface is rather specialized for use from inside
audio_processing_impl.cc. I can't think of where else this could be
use used. Therefore I'd like to keep the build targets as they
currently are. Declaring Create methods in a different target seems
something that shouldn't be difficult to do later if it's needed.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:127: const
std::vector<rtc::ArrayView<const float>> src) = 0;
On 2017/03/28 08:54:59, kwiberg-webrtc wrote:
> The comment for AddInput/AddOutput applies here too.

Done.

https://codereview.webrtc.org/2747123007/diff/440001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:135: virtual void
WriteConfig(InternalAPMConfig config, bool forced) = 0;
On 2017/03/28 08:54:59, kwiberg-webrtc wrote:
> InternalAPMConfig is somewhat big, so isn't exactly free even to move. Pass by
> const ref?

Done.

[aleloi, 2017-03-29 08:41:46]

aleloi@webrtc.org changed reviewers:
- kwiberg@webrtc.org

[kwiberg-webrtc, 2017-03-29 08:57:10]

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

[kwiberg-webrtc, 2017-03-29 08:57:11]

Looks much better!

https://codereview.webrtc.org/2747123007/diff/460001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right):

https://codereview.webrtc.org/2747123007/diff/460001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:58:
RTC_DISALLOW_COPY_AND_ASSIGN(InternalAPMConfig);
You don't have to use this macro anymore. Just do

  InternalAPMConfig(const InternalAPMConfig&) = delete;
  InternalAPMConfig& operator=(const InternalAPMConfig&) = delete;

It's a bit wordier, but you get to not call a macro, and it's the same syntax
you would use to define copy construction and assignment, and you put it in the
same place as the other constructors and assignment operators.

By the way, did you intend to prohibit moves too?

https://codereview.webrtc.org/2747123007/diff/460001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:86: AecDumper() =
default;
You forgot to remove this one.

[aleloi, 2017-03-29 14:39:55]

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

[commit-bot: I haz the power, 2017-03-29 14:40:13]

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-03-29 15:04:57]

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

[commit-bot: I haz the power, 2017-03-29 15:04:58]

Dry run: This issue passed the CQ dry run.