Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(593)

Issue 2991343002: Decoupling audio_device from Obj-C code (Closed)

Created:
3 years, 4 months ago by mbonadei
Modified:
3 years, 3 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, audio-team_agora.io, sdk-team_agora.io, peah-webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Fixing errors #

Patch Set 3 : Fixing linux/win/android trybots #

Patch Set 4 : Trying to fix chromium builds #

Patch Set 5 : Fixing ios tests #

Total comments: 10

Patch Set 6 : Renaming objc targets + private visibility #

Patch Set 7 : Fixing include_dirs #

Patch Set 8 : Private visibility for audio_device_objc_unittests #

Total comments: 6

Patch Set 9 : _ios suffix and fixing noop.cc trick #

Total comments: 1

Patch Set 10 : Renaming audio_device_ios to audio_device_ios_objc #

Patch Set 11 : Adding documentation regarding noop.cc trick #

Patch Set 12 : Improving documentation #

Patch Set 13 : Switching to rtc_source_set to avoid noop.cc #

Patch Set 14 : Removing noop.cc file #

Patch Set 15 : Fixing "gn check" #

Patch Set 16 : Rebasing... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -35 lines) Patch
M webrtc/modules/audio_device/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +76 lines, -32 lines 0 comments Download
M webrtc/modules/audio_device/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/modules/audio_device/ios/audio_device_ios.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioSession+Private.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAudioSession.h View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 26 (11 generated)
mbonadei
3 years, 4 months ago (2017-08-11 12:11:49 UTC) #2
kjellander_webrtc
https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn#newcode49 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_device/BUILD.gn#newcode66 webrtc/modules/audio_device/BUILD.gn:66: check_includes = ...
3 years, 4 months ago (2017-08-14 06:54:14 UTC) #3
mbonadei
PTAL. https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn#newcode49 webrtc/modules/audio_device/BUILD.gn:49: rtc_static_library("objc_audio_device") { On 2017/08/14 06:54:13, kjellander_webrtc wrote: > ...
3 years, 4 months ago (2017-08-16 09:15:25 UTC) #4
kjellander_webrtc
+henrika for question about the audio device iOS target. https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn#newcode213 webrtc/modules/audio_device/BUILD.gn:213: ...
3 years, 4 months ago (2017-08-16 09:31:53 UTC) #6
kjellander_webrtc
Clarified one comment. https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/80001/webrtc/modules/audio_device/BUILD.gn#newcode213 webrtc/modules/audio_device/BUILD.gn:213: if (is_ios) { On 2017/08/16 09:31:53, ...
3 years, 4 months ago (2017-08-16 09:33:09 UTC) #7
henrika_webrtc
https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn#newcode64 webrtc/modules/audio_device/BUILD.gn:64: rtc_static_library("audio_device_objc") { Agree. iOS should at least be part ...
3 years, 4 months ago (2017-08-16 12:09:22 UTC) #9
mbonadei
https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn#newcode50 webrtc/modules/audio_device/BUILD.gn:50: "noop.cc", On 2017/08/16 09:31:53, kjellander_webrtc wrote: > IIRC, this ...
3 years, 4 months ago (2017-08-16 12:19:37 UTC) #10
kjellander_webrtc
lgtm with my last comment addressed. https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/160001/webrtc/modules/audio_device/BUILD.gn#newcode51 webrtc/modules/audio_device/BUILD.gn:51: "noop.cc", Add a ...
3 years, 4 months ago (2017-08-16 12:46:17 UTC) #11
mbonadei
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_device/BUILD.gn ...
3 years, 4 months ago (2017-08-16 14:03:10 UTC) #13
mbonadei
https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2991343002/diff/140001/webrtc/modules/audio_device/BUILD.gn#newcode64 webrtc/modules/audio_device/BUILD.gn:64: rtc_static_library("audio_device_objc") { On 2017/08/16 12:09:22, henrika_webrtc wrote: > Agree. ...
3 years, 4 months ago (2017-08-16 14:03:43 UTC) #14
henrika_webrtc
lgtm
3 years, 3 months ago (2017-08-31 13:56:49 UTC) #17
henrika_webrtc
+kthelgason
3 years, 3 months ago (2017-08-31 13:57:15 UTC) #19
kthelgason
lgtm
3 years, 3 months ago (2017-09-01 08:29:05 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2991343002/290001
3 years, 3 months ago (2017-09-12 11:27:13 UTC) #23
commit-bot: I haz the power
3 years, 3 months ago (2017-09-12 11:45:30 UTC) #26
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/external/webrtc/+/bcc2176e64d82b10d105b4d3d...

Powered by Google App Engine
This is Rietveld 408576698