Decoupling audio_device from Obj-C code

Decoupling audio_device from Obj-C code

The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743
Review-Url: https://codereview.webrtc.org/2991343002
Cr-Commit-Position: refs/heads/master@{#19795}
Committed: https://chromium.googlesource.com/external/webrtc/+/bcc2176e64d82b10d105b4d3da3851971a291814

[mbonadei, 2017-08-11 12:11:48]

mbonadei@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2017-08-14 06:54:14]

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:49: rtc_static_library("objc_audio_device")
{
I prefer audio_device_objc

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:66: check_includes = false
we usually have this earlier in the target. I'm not sure if the style guide has
any preference, but let's be consistent across our codebase.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:68: # There is a cyclic dependency with
:audio_device
I prefer to have this comment at the check_includes attribute and omit the
commented dependency below. Ideally a bug reference as well that addresses
getting it fixed.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:286:
rtc_source_set("objc_audio_device_unittests") {
audio_device_objc_unittests

[mbonadei, 2017-08-16 09:15:25]

PTAL.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:49: rtc_static_library("objc_audio_device")
{
On 2017/08/14 06:54:13, kjellander_webrtc wrote:
> I prefer audio_device_objc

Done.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:66: check_includes = false
On 2017/08/14 06:54:14, kjellander_webrtc wrote:
> we usually have this earlier in the target. I'm not sure if the style guide
has
> any preference, but let's be consistent across our codebase.

Right. This has been removed in ps#6 because the circular dependency has been
broken.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:68: # There is a cyclic dependency with
:audio_device
On 2017/08/14 06:54:13, kjellander_webrtc wrote:
> I prefer to have this comment at the check_includes attribute and omit the
> commented dependency below. Ideally a bug reference as well that addresses
> getting it fixed.

Done.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:286:
rtc_source_set("objc_audio_device_unittests") {
On 2017/08/14 06:54:13, kjellander_webrtc wrote:
> audio_device_objc_unittests

Done.

[kjellander_webrtc, 2017-08-16 09:31:52]

kjellander@webrtc.org changed reviewers:
+ henrika@webrtc.org

[kjellander_webrtc, 2017-08-16 09:31:53]

+henrika for question about the audio device iOS target.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:213: if (is_ios) {
I think this would have caused an error if
rtc_include_internal_audio_device=false, which it is for Chromium builds. I
guess it haven't caused any trouble since the audio device code is not built as
part of the Chromium iOS build. Either way, we should fix the problem now as we
move things around.

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:50: "noop.cc",
IIRC, this is only needed to avoid an error when compiling static_library
targets on Mac (maybe it was iOS as well)? If so, put it inside an is_mac
|| is_ios condition.

See https://codereview.webrtc.org/2764523002 for simliar example that I was
abler to clean up (but that's because it was changed to use
complete_static_lib=true, which we shouldn't do here)

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:64: rtc_static_library("audio_device_objc")
{
I think audio_device_ios is a better name, as we've been using "objc" as a
common term for just objc code which may or may not be common for macOS and iOS.
This target contains only ios code AFAICT.

henrika@ correct me if I'm wrong.

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/noop.cc (right):

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/noop.cc:12: // to compile
webrtc/rtc_base:rtc_base_approved.
The target here is wrong, right? Better make the comment generic and mention
it's needed to compile a rtc_static_library target, since I believe that's the
reason, right?

[kjellander_webrtc, 2017-08-16 09:33:09]

Clarified one comment.

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/BUILD.gn:213: if (is_ios) {
On 2017/08/16 09:31:53, kjellander_webrtc wrote:
> I think this would have caused an error if
> rtc_include_internal_audio_device=false, which it is for Chromium builds. I
> guess it haven't caused any trouble since the audio device code is not built
as
> part of the Chromium iOS build. Either way, we should fix the problem now as
we
> move things around.

To be clear: you are fixing this in your CL, so this comment is kind of
confusing :)

[mbonadei, 2017-08-16 10:03:03]

Description was changed from

==========
Decoupling audio_device from Obj-C code [with cyclic deps]

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

to

==========
Decoupling audio_device from Obj-C code.

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

[henrika_webrtc, 2017-08-16 12:09:22]

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:64: rtc_static_library("audio_device_objc")
{
Agree. iOS should at least be part of the name. 

audio_device_ios_objc feels OK as well to me.

[mbonadei, 2017-08-16 12:19:37]

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:50: "noop.cc",
On 2017/08/16 09:31:53, kjellander_webrtc wrote:
> IIRC, this is only needed to avoid an error when compiling static_library
> targets on Mac (maybe it was iOS as well)? If so, put it inside an is_mac
> || is_ios condition.
> 
> See https://codereview.webrtc.org/2764523002 for simliar example that I was
> abler to clean up (but that's because it was changed to use
> complete_static_lib=true, which we shouldn't do here)

Right. It is only needed to compile a static_library targeting macOS or iOS.
Fixed in the next PS.

[kjellander_webrtc, 2017-08-16 12:46:17]

lgtm with my last comment addressed.

https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:51: "noop.cc",
Add a comment about why this silly thing is needed.

[mbonadei, 2017-08-16 12:50:49]

Description was changed from

==========
Decoupling audio_device from Obj-C code.

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

to

==========
Decoupling audio_device from Obj-C code.

BUG=webrtc:7743
==========

[mbonadei, 2017-08-16 14:03:10]

On 2017/08/16 12:46:17, kjellander_webrtc wrote:
> lgtm with my last comment addressed.
> 
>
https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_dev...
> File webrtc/modules/audio_device/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_dev...
> webrtc/modules/audio_device/BUILD.gn:51: "noop.cc",
> Add a comment about why this silly thing is needed.

Done.

[mbonadei, 2017-08-16 14:03:43]

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/BUILD.gn (right):

https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/BUILD.gn:64: rtc_static_library("audio_device_objc")
{
On 2017/08/16 12:09:22, henrika_webrtc wrote:
> Agree. iOS should at least be part of the name. 
> 
> audio_device_ios_objc feels OK as well to me.

Yes, let's pick audio_device_ios_objc I prefer it too.

[mbonadei, 2017-08-16 14:07:58]

Description was changed from

==========
Decoupling audio_device from Obj-C code.

BUG=webrtc:7743
==========

to

==========
The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743
==========

[mbonadei, 2017-08-31 13:52:54]

Description was changed from

==========
The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743
==========

to

==========
Decoupling audio_device from Obj-C code

The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743
==========

[henrika_webrtc, 2017-08-31 13:56:49]

lgtm

[henrika_webrtc, 2017-08-31 13:57:05]

henrika@webrtc.org changed reviewers:
+ kthelgason@webrtc.org

[henrika_webrtc, 2017-08-31 13:57:15]

+kthelgason

[kthelgason, 2017-09-01 08:29:05]

lgtm

[oprypin_webrtc, 2017-09-12 11:27:02]

The CQ bit was checked by oprypin@webrtc.org

[oprypin_webrtc, 2017-09-12 11:27:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, kthelgason@webrtc.org, henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2991343002/#ps290001 (title:
"Rebasing...")

[commit-bot: I haz the power, 2017-09-12 11:27:13]

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-09-12 11:45:25]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1505215622028340,
"parent_rev": "2475ae2e4cdbb7cbfa2a4801cbc36ff7eb73aacd", "commit_rev":
"bcc2176e64d82b10d105b4d3da3851971a291814"}

[commit-bot: I haz the power, 2017-09-12 11:45:28]

Description was changed from

==========
Decoupling audio_device from Obj-C code

The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743
==========

to

==========
Decoupling audio_device from Obj-C code

The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).

To achieve this we have created 2 targets (audio_device_ios_objc and
audio_device_generic) and audio_device will act as a proxy between these targets
(this way we can avoid a circular dependency between audio_device_generic and
audio_device_ios_objc).

BUG=webrtc:7743

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

[commit-bot: I haz the power, 2017-09-12 11:45:30]

Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/external/webrtc/+/bcc2176e64d82b10d105b4d3d...