Move FilePlayer and FileRecorder to Voice Engine

Move FilePlayer and FileRecorder to Voice Engine

Because Voice Engine was the only user.

(We have tried to land this many times before. I'm hoping that this
time all external dependencies on these files will really be gone.)

BUG=none

Review-Url: https://codereview.webrtc.org/2622493002
Cr-Commit-Position: refs/heads/master@{#15978}
Committed: https://chromium.googlesource.com/external/webrtc/+/9774447b8f30a531caea824b5d9b208abf182e7c

[kwiberg-webrtc, 2017-01-09 09:06:46]

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

[commit-bot: I haz the power, 2017-01-09 09:06:50]

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

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

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

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

[kwiberg-webrtc, 2017-01-09 09:37:01]

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

[commit-bot: I haz the power, 2017-01-09 09:37:10]

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-01-09 10:38:06]

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

[commit-bot: I haz the power, 2017-01-09 10:38:06]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-01-09 12:20:11]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2017-01-09 12:22:42]

kwiberg@webrtc.org changed reviewers:
+ kjellander@webrtc.org, solenberg@webrtc.org

[kwiberg-webrtc, 2017-01-09 12:22:43]

PTAL. solenberg@: everything. kjellander@: .gn and .isolate files

Interestingly, this fails on the Chromium bots with a patch application
error---it seems they don't like that I modify the top-level .gn file.

[the sun, 2017-01-09 12:44:44]

lgtm

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD.gn
File webrtc/voice_engine/BUILD.gn (right):

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD...
webrtc/voice_engine/BUILD.gn:11: rtc_static_library("audio_coder") {
Do we really benefit from having separate targets for these? can we just add the
files to the voice_engine static lib? It seems to pull in the same deps, and
also, "file_player_unittest.cc" is included in voice_engine_unittests.

[kwiberg-webrtc, 2017-01-09 12:53:56]

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD.gn
File webrtc/voice_engine/BUILD.gn (right):

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD...
webrtc/voice_engine/BUILD.gn:11: rtc_static_library("audio_coder") {
On 2017/01/09 12:44:43, the sun wrote:
> Do we really benefit from having separate targets for these? can we just add
the
> files to the voice_engine static lib? It seems to pull in the same deps, and
> also, "file_player_unittest.cc" is included in voice_engine_unittests.

Grouping them by themselves like this makes it easier to see what pieces there
are, instead of just seeing it as one big Voice Engine pile. In particular, it's
now obvious at a glance which source files are responsible for which
dependencies.

There are currently two more advantages that will eventually go away: I was able
to easily mark the new targets as being subject to "gn check", and will easily
be able to fix the find_bad_constructs errors and remove the suppressions. Being
able to do this for several small targets, one by one, rather than having to do
it for giant targets increases the chance that it gets done.

[the sun, 2017-01-09 12:57:37]

On 2017/01/09 12:53:56, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD.gn
> File webrtc/voice_engine/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD...
> webrtc/voice_engine/BUILD.gn:11: rtc_static_library("audio_coder") {
> On 2017/01/09 12:44:43, the sun wrote:
> > Do we really benefit from having separate targets for these? can we just add
> the
> > files to the voice_engine static lib? It seems to pull in the same deps, and
> > also, "file_player_unittest.cc" is included in voice_engine_unittests.
> 
> Grouping them by themselves like this makes it easier to see what pieces there
> are, instead of just seeing it as one big Voice Engine pile. In particular,
it's
> now obvious at a glance which source files are responsible for which
> dependencies.
> 
> There are currently two more advantages that will eventually go away: I was
able
> to easily mark the new targets as being subject to "gn check", and will easily
> be able to fix the find_bad_constructs errors and remove the suppressions.
Being
> able to do this for several small targets, one by one, rather than having to
do
> it for giant targets increases the chance that it gets done.

Ok, thanks for explaining!

[kjellander_webrtc, 2017-01-09 15:17:21]

*.gn: lgtm
Can you add a BUG= reference?

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD.gn
File webrtc/voice_engine/BUILD.gn (right):

https://codereview.webrtc.org/2622493002/diff/20001/webrtc/voice_engine/BUILD...
webrtc/voice_engine/BUILD.gn:11: rtc_static_library("audio_coder") {
On 2017/01/09 12:53:55, kwiberg-webrtc wrote:
> On 2017/01/09 12:44:43, the sun wrote:
> > Do we really benefit from having separate targets for these? can we just add
> the
> > files to the voice_engine static lib? It seems to pull in the same deps, and
> > also, "file_player_unittest.cc" is included in voice_engine_unittests.
> 
> Grouping them by themselves like this makes it easier to see what pieces there
> are, instead of just seeing it as one big Voice Engine pile. In particular,
it's
> now obvious at a glance which source files are responsible for which
> dependencies.
> 
> There are currently two more advantages that will eventually go away: I was
able
> to easily mark the new targets as being subject to "gn check", and will easily
> be able to fix the find_bad_constructs errors and remove the suppressions.
Being
> able to do this for several small targets, one by one, rather than having to
do
> it for giant targets increases the chance that it gets done.

Nice, I like small targets. There are many benefits with that and you already
mentioned a few of them.

[kwiberg-webrtc, 2017-01-09 17:35:08]

On 2017/01/09 15:17:21, kjellander_webrtc wrote:
> *.gn: lgtm
> Can you add a BUG= reference?

There already is one. Did you mean I should post a bug, and have the BUG= filed
point to that instead?

[kjellander_webrtc, 2017-01-10 08:51:48]

On 2017/01/09 17:35:08, kwiberg-webrtc wrote:
> On 2017/01/09 15:17:21, kjellander_webrtc wrote:
> > *.gn: lgtm
> > Can you add a BUG= reference?
> 
> There already is one. Did you mean I should post a bug, and have the BUG=
filed
> point to that instead?

I meant pointing at a bug at bugs.webrtc.org, but maybe it's just silly to file
one if this is completely unrelated to anything else you've been doing. Then
it's fine with BUG=none.

[kwiberg-webrtc, 2017-01-10 09:10:45]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-01-10 09:10:55]

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-01-10 09:12:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484039445049290,
"parent_rev": "429a407b8f9aa019e41b502f213f25d49581a3fa", "commit_rev":
"9774447b8f30a531caea824b5d9b208abf182e7c"}

[commit-bot: I haz the power, 2017-01-10 09:12:54]

Description was changed from

==========
Move FilePlayer and FileRecorder to Voice Engine

Because Voice Engine was the only user.

(We have tried to land this many times before. I'm hoping that this
time all external dependencies on these files will really be gone.)

BUG=none
==========

to

==========
Move FilePlayer and FileRecorder to Voice Engine

Because Voice Engine was the only user.

(We have tried to land this many times before. I'm hoping that this
time all external dependencies on these files will really be gone.)

BUG=none

Review-Url: https://codereview.webrtc.org/2622493002
Cr-Commit-Position: refs/heads/master@{#15978}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9774447b8f30a531caea824b5...
==========

[commit-bot: I haz the power, 2017-01-10 09:12:56]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/9774447b8f30a531caea824b5...