Split iOS sdk in to separate targets

Split iOS sdk in to separate targets

This CL splits the iOS sdk into separate static libraries for video,
audio, ui, common, and peerconnection-related code. This will in the
future make it easier to compile WebRTC without unneeded components.

BUG=webrtc:4867
Review-Url: https://codereview.webrtc.org/2862543002
Cr-Commit-Position: refs/heads/master@{#18166}
Committed: https://chromium.googlesource.com/external/webrtc/+/52c83fe7102f566cf35a7533092873d58b38f426

[kthelgason, 2017-05-03 10:32:22]

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

[commit-bot: I haz the power, 2017-05-03 10:32: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-05-03 10:35:56]

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

[commit-bot: I haz the power, 2017-05-03 10:35:57]

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

[kthelgason, 2017-05-03 12:44:05]

Patchset #2 (id:20001) has been deleted

[kthelgason, 2017-05-03 13:29:16]

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

[commit-bot: I haz the power, 2017-05-03 13:29: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-05-03 13:34:15]

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

[commit-bot: I haz the power, 2017-05-03 13:34:16]

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

[kthelgason, 2017-05-03 14:08:56]

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

[commit-bot: I haz the power, 2017-05-03 14:09: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-05-03 14:12:15]

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

[commit-bot: I haz the power, 2017-05-03 14:12:17]

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

[kthelgason, 2017-05-03 14:25:08]

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

[commit-bot: I haz the power, 2017-05-03 14:25:16]

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-05-03 14:28:28]

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

[commit-bot: I haz the power, 2017-05-03 14:28:30]

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...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/20033)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25347)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24222)

[kthelgason, 2017-05-03 14:47:51]

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

[commit-bot: I haz the power, 2017-05-03 14:48:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kthelgason, 2017-05-03 14:48:53]

kthelgason@webrtc.org changed reviewers:
+ denicija@webrtc.org, magjed@webrtc.org

[kthelgason, 2017-05-03 14:48:55]

PTAL when you have time :)

I'd recommend just looking at the gn files, the rest is basically just noise.

[commit-bot: I haz the power, 2017-05-03 14:56:50]

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

[commit-bot: I haz the power, 2017-05-03 14:56:51]

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

[daniela-webrtc, 2017-05-04 08:32:58]

Quick review of the metal stuff to help with the build problems

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:230: libs = [
You need CoreVideo here as well.

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:376: if (is_ios && rtc_use_metal_rendering) {
No need for the rtc_use_metal_rendering check, the test can be compiled and run
on simulator as well.

[kthelgason, 2017-05-04 09:13:16]

Thanks for the review Daniela

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:230: libs = [
On 2017/05/04 08:32:58, daniela-webrtc wrote:
> You need CoreVideo here as well.

Thanks! When building locally clang seems to think it's being helpful when it
does the include implicitly 😒

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:376: if (is_ios && rtc_use_metal_rendering) {
On 2017/05/04 08:32:58, daniela-webrtc wrote:
> No need for the rtc_use_metal_rendering check, the test can be compiled and
run
> on simulator as well.

How does the test work on simulator without metal support? IMO there is no
reason to compile and run it if it's not actually testing anything useful

[kthelgason, 2017-05-04 10:10:15]

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

[commit-bot: I haz the power, 2017-05-04 10:10: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-05-04 10:13:48]

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

[commit-bot: I haz the power, 2017-05-04 10:13:49]

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

[magjed_webrtc, 2017-05-04 10:54:40]

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:124: "objc/Framework/Classes/UI/RTCEAGLVideoView.m",
I would like to be consistent with the folders and targets, so if
RTCEAGLVideoView and RTCNSGLVideoView goes in the video target, maybe we should
move them to the video folder?

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:479: rtc_static_library("rtc_sdk_corevideoframebuffer") {
I think we should just merge this into the Video target now.

[daniela-webrtc, 2017-05-04 12:10:16]

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:376: if (is_ios && rtc_use_metal_rendering) {
On 2017/05/04 09:13:16, kthelgason wrote:
> On 2017/05/04 08:32:58, daniela-webrtc wrote:
> > No need for the rtc_use_metal_rendering check, the test can be compiled and
> run
> > on simulator as well.
> 
> How does the test work on simulator without metal support? IMO there is no
> reason to compile and run it if it's not actually testing anything useful
The class that is tested, was written in such way that it can be compiled on
both simulator and device. Please re-add the test for simulator as well so that
the bots continue to run the test as they did before

[tkchin_webrtc, 2017-05-05 00:28:44]

Description was changed from

==========
Split iOS sdk in to separate targets

This CL splits the iOS sdk into separate static libraries for video,
audio, ui, common, and peerconnection-related code. This will in the
future make it easier to compile WebRTC without unneeded components.

BUG=webrtc:4867
==========

to

==========
Split iOS sdk in to separate targets

This CL splits the iOS sdk into separate static libraries for video,
audio, ui, common, and peerconnection-related code. This will in the
future make it easier to compile WebRTC without unneeded components.

BUG=webrtc:4867
==========

[tkchin_webrtc, 2017-05-05 00:28:44]

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

[tkchin_webrtc, 2017-05-05 00:36:22]

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioSource.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioSource.mm:2: *  Copyright 2016
The WebRTC project authors. All Rights Reserved.
These belong in PeerConnection/

Audio should only include the RTCAudioSession related bits.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/RTCMetrics.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/RTCMetrics.mm:2: *  Copyright 2016 The
WebRTC project authors. All Rights Reserved.
Metrics, SSLAdapter and Tracing are only used in relation to PeerConnection -
suggest moving them there instead

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/RTCMetricsSampleInfo.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/RTCMetricsSampleInfo.mm:13: #import
"Common/NSString+StdString.h"
Recommend either full paths, or set include header paths in BUILD.gn, but not a
mix of the two.

The WebRTC/<foo.h> includes we see before were because that's how those headers
would be consumed externally.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/RTCSSLAdapter.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/RTCSSLAdapter.mm:2: *  Copyright 2016
The WebRTC project authors. All Rights Reserved.
This belongs in PeerConnection/

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/helpers.h (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/helpers.h:21: bool
CheckAndLogError(BOOL success, NSError* error);
We should eventually convert these to ObjC functions and delete them

[kthelgason, 2017-05-08 13:40:08]

please take another look :)

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:124: "objc/Framework/Classes/UI/RTCEAGLVideoView.m",
On 2017/05/04 10:54:40, magjed_webrtc wrote:
> I would like to be consistent with the folders and targets, so if
> RTCEAGLVideoView and RTCNSGLVideoView goes in the video target, maybe we
should
> move them to the video folder?

That's fine by me. I think both are slightly confusing, but having the targets
consistent with the file structure is probably less so.

https://codereview.webrtc.org/2862543002/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:376: if (is_ios && rtc_use_metal_rendering) {
On 2017/05/04 12:10:16, daniela-webrtc wrote:
> On 2017/05/04 09:13:16, kthelgason wrote:
> > On 2017/05/04 08:32:58, daniela-webrtc wrote:
> > > No need for the rtc_use_metal_rendering check, the test can be compiled
and
> > run
> > > on simulator as well.
> > 
> > How does the test work on simulator without metal support? IMO there is no
> > reason to compile and run it if it's not actually testing anything useful
> The class that is tested, was written in such way that it can be compiled on
> both simulator and device. Please re-add the test for simulator as well so
that
> the bots continue to run the test as they did before  

Done.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioSource.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioSource.mm:2: *  Copyright 2016
The WebRTC project authors. All Rights Reserved.
On 2017/05/05 00:36:21, tkchin_webrtc wrote:
> These belong in PeerConnection/
> 
> Audio should only include the RTCAudioSession related bits.

Done.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/RTCMetrics.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/RTCMetrics.mm:2: *  Copyright 2016 The
WebRTC project authors. All Rights Reserved.
On 2017/05/05 00:36:22, tkchin_webrtc wrote:
> Metrics, SSLAdapter and Tracing are only used in relation to PeerConnection -
> suggest moving them there instead

Done.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/RTCSSLAdapter.mm (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/RTCSSLAdapter.mm:2: *  Copyright 2016
The WebRTC project authors. All Rights Reserved.
On 2017/05/05 00:36:22, tkchin_webrtc wrote:
> This belongs in PeerConnection/

Acknowledged.

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/helpers.h (right):

https://codereview.webrtc.org/2862543002/diff/120001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/helpers.h:21: bool
CheckAndLogError(BOOL success, NSError* error);
On 2017/05/05 00:36:22, tkchin_webrtc wrote:
> We should eventually convert these to ObjC functions and delete them

Agreed, but that's left for another CL

[kthelgason, 2017-05-08 14:06:23]

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

[commit-bot: I haz the power, 2017-05-08 14:06:33]

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-05-08 14:24:42]

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

[commit-bot: I haz the power, 2017-05-08 14:24:45]

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...)

[daniela-webrtc, 2017-05-08 14:30:40]

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:219: "objc/Framework/Classes/Metal/RTCMTLI420Renderer.h",
RTCMTLI420Renderer should be added for both ios and mac

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:384: if (is_ios && rtc_use_metal_rendering) {
Remove the rtc_use_metal_rendering check.

[magjed_webrtc, 2017-05-08 15:57:36]

lgtm % comments

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Common/helpers.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Common/helpers.h:11: #ifndef
WEBRTC_SDK_OBJC_FRAMEWORK_CLASSES_HELPERS_H_
Update the name of the include guard

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/UI/RTCUIApplication.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/UI/RTCUIApplication.h:11: #ifndef
WEBRTC_BASE_OBJC_RTC_UI_APPLICATION_H_
Update the name of the include guard.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Video/corevideo_frame_buffer.cc (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Video/corevideo_frame_buffer.cc:10: 
Remove webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.cc.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Video/corevideo_frame_buffer.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Video/corevideo_frame_buffer.h:1: /*
Remove webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Video/corevideo_frame_buffer.h:11: #ifndef
WEBRTC_COMMON_VIDEO_INCLUDE_COREVIDEO_FRAME_BUFFER_H_
Update the name of the include guard.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Video/objcvideotracksource.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Video/objcvideotracksource.h:12: #define
WEBRTC_SDK_OBJC_FRAMEWORK_CLASSES_OBJCVIDEOTRACKSOURCE_H_
Update the name of the include guard.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:12: #ifndef
WEBRTC_SDK_OBJC_FRAMEWORK_CLASSES_H264_VIDEO_TOOLBOX_DECODER_H_
Update the name of the include guard.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:12: #ifndef
WEBRTC_SDK_OBJC_FRAMEWORK_CLASSES_H264_VIDEO_TOOLBOX_ENCODER_H_
Update the name of the include guard.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/videocodecfactory.h (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/videocodecfactory.h:11: #ifndef
WEBRTC_SDK_OBJC_FRAMEWORK_CLASSES_VIDEOTOOLBOXVIDEOCODECFACTORY_H_
Update the name of the include guard.

[kthelgason, 2017-05-09 08:57:09]

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:219: "objc/Framework/Classes/Metal/RTCMTLI420Renderer.h",
On 2017/05/08 14:30:40, daniela-webrtc wrote:
> RTCMTLI420Renderer should be added for both ios and mac

Done.

https://codereview.webrtc.org/2862543002/diff/140001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:384: if (is_ios && rtc_use_metal_rendering) {
On 2017/05/08 14:30:40, daniela-webrtc wrote:
> Remove the rtc_use_metal_rendering check.

Done.

[tkchin_webrtc, 2017-05-09 18:39:25]

This is actually really hard to review because it's large and touches many
files. Would feel better about it if we moved one directory at a time. (e.g.
common)

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:42: rtc_static_library("rtc_sdk_common_module") {
From what I last remembered - webrtc/sdk is intended for future Android as well
It is probably better to keep the objc suffix

here and elsewhere

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:72: libs = [ "AVFoundation.framework" ]
why is AVFoundation needed for common?

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:102: "objc/Framework/Classes/Video/RTCVideoSource.mm",
RTCVideoSource and RTCVideoTrack belong in PeerConnection, as does RTCVideoFrame
This should only contain additional platform specific capture and render code

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioTrack.mm (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioTrack.mm:2: *  Copyright 2015
The WebRTC project authors. All Rights Reserved.
RTCAudioTrack* belongs in PeerConnection/

[kthelgason, 2017-05-10 07:08:57]

I agree that this is super tedious to review. I should almost certainly have
submitted this as multiple CLs instead. In anycase I don't think it's worth it
to split it up now.

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:72: libs = [ "AVFoundation.framework" ]
On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> why is AVFoundation needed for common?

It shouldn't, this probably appeared as an artifact of one of the many rebases
I've had to do. Thanks!

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:102: "objc/Framework/Classes/Video/RTCVideoSource.mm",
On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> RTCVideoSource and RTCVideoTrack belong in PeerConnection, as does
RTCVideoFrame
> This should only contain additional platform specific capture and render code

My thinking here was that this would also make it easier to compile audio-only
or data-only versions of webrtc in the future. For those cases we'd want to skip
_all_ video stuff right? How do you distinguish between things that you think
belong in PeerConnection vs other targets. I've been using naive string matching
on the target name :)

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioTrack.mm (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/Audio/RTCAudioTrack.mm:2: *  Copyright 2015
The WebRTC project authors. All Rights Reserved.
On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> RTCAudioTrack* belongs in PeerConnection/

Acknowledged.

[kthelgason, 2017-05-10 07:57:28]

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

[commit-bot: I haz the power, 2017-05-10 07:57:34]

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-05-10 08:03:49]

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

[commit-bot: I haz the power, 2017-05-10 08:03: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/16901)

[kthelgason, 2017-05-10 12:56:55]

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:42: rtc_static_library("rtc_sdk_common_module") {
On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> From what I last remembered - webrtc/sdk is intended for future Android as
well
> It is probably better to keep the objc suffix
> 
> here and elsewhere

Done.

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:72: libs = [ "AVFoundation.framework" ]
On 2017/05/10 07:08:57, kthelgason wrote:
> On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> > why is AVFoundation needed for common?
> 
> It shouldn't, this probably appeared as an artifact of one of the many rebases
> I've had to do. Thanks!

Turns out this was here for a function in helpers that was unused. I removed the
lib and the function that required it.

[tkchin_webrtc, 2017-05-10 17:13:19]

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:102: "objc/Framework/Classes/Video/RTCVideoSource.mm",
On 2017/05/10 07:08:57, kthelgason wrote:
> On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> > RTCVideoSource and RTCVideoTrack belong in PeerConnection, as does
> RTCVideoFrame
> > This should only contain additional platform specific capture and render
code
> 
> My thinking here was that this would also make it easier to compile audio-only
> or data-only versions of webrtc in the future. For those cases we'd want to
skip
> _all_ video stuff right? How do you distinguish between things that you think
> belong in PeerConnection vs other targets. I've been using naive string
matching
> on the target name :)

But RTCVideoSource / RTCVideoTrack / RTCVideoFrame are all peer connection
related objects. You can't ever ship an implementation without those bits
because you need to be able to receive them from other WebRTC clients (eg in
Chrome).

Best way to check is to see if it's part of the PeerConnection W3C spec.

Mobile-only code however we can ship without in case other people have their own
implementations they would like to use.

[tkchin_webrtc, 2017-05-10 17:16:15]

On 2017/05/10 17:13:19, tkchin_webrtc wrote:
> https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
> File webrtc/sdk/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
> webrtc/sdk/BUILD.gn:102: "objc/Framework/Classes/Video/RTCVideoSource.mm",
> On 2017/05/10 07:08:57, kthelgason wrote:
> > On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> > > RTCVideoSource and RTCVideoTrack belong in PeerConnection, as does
> > RTCVideoFrame
> > > This should only contain additional platform specific capture and render
> code
> > 
> > My thinking here was that this would also make it easier to compile
audio-only
> > or data-only versions of webrtc in the future. For those cases we'd want to
> skip
> > _all_ video stuff right? How do you distinguish between things that you
think
> > belong in PeerConnection vs other targets. I've been using naive string
> matching
> > on the target name :)
> 
> But RTCVideoSource / RTCVideoTrack / RTCVideoFrame are all peer connection
> related objects. You can't ever ship an implementation without those bits
> because you need to be able to receive them from other WebRTC clients (eg in
> Chrome).
> 
> Best way to check is to see if it's part of the PeerConnection W3C spec.
> 
> Mobile-only code however we can ship without in case other people have their
own
> implementations they would like to use.

I think it's still worth splitting up CL at this point - I'm not 100% confident
that I've reached 100% coverage on this CL.
It should be pretty easy to split? Mostly copy/paste.

[kthelgason, 2017-05-12 08:27:39]

On 2017/05/10 17:16:15, tkchin_webrtc wrote:
> On 2017/05/10 17:13:19, tkchin_webrtc wrote:
> > https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn
> > File webrtc/sdk/BUILD.gn (right):
> > 
> >
>
https://codereview.webrtc.org/2862543002/diff/160001/webrtc/sdk/BUILD.gn#newc...
> > webrtc/sdk/BUILD.gn:102: "objc/Framework/Classes/Video/RTCVideoSource.mm",
> > On 2017/05/10 07:08:57, kthelgason wrote:
> > > On 2017/05/09 18:39:25, tkchin_webrtc wrote:
> > > > RTCVideoSource and RTCVideoTrack belong in PeerConnection, as does
> > > RTCVideoFrame
> > > > This should only contain additional platform specific capture and render
> > code
> > > 
> > > My thinking here was that this would also make it easier to compile
> audio-only
> > > or data-only versions of webrtc in the future. For those cases we'd want
to
> > skip
> > > _all_ video stuff right? How do you distinguish between things that you
> think
> > > belong in PeerConnection vs other targets. I've been using naive string
> > matching
> > > on the target name :)
> > 
> > But RTCVideoSource / RTCVideoTrack / RTCVideoFrame are all peer connection
> > related objects. You can't ever ship an implementation without those bits
> > because you need to be able to receive them from other WebRTC clients (eg in
> > Chrome).
> > 
> > Best way to check is to see if it's part of the PeerConnection W3C spec.
> > 
> > Mobile-only code however we can ship without in case other people have their
> own
> > implementations they would like to use.
> 
> I think it's still worth splitting up CL at this point - I'm not 100%
confident
> that I've reached 100% coverage on this CL.
> It should be pretty easy to split? Mostly copy/paste.

I'm sorry but I've taken the morning to try and split the CL and it's quite
difficult. Many of the targets don't make sense in isolation (objc_common for
example) and would require moving the same file around several times in
different CLs. I really don't want to spend the time required to split this up
properly, and we also have two other projects blocked on this.

I realize it's very hard to review like this, and I appreciate that. Most of the
changes you can safely ignore. If this ends up getting reverted I will bite the
bullet and split it, but that will be almost as much work as redoing it
completely.

[kthelgason, 2017-05-12 09:08:28]

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

[commit-bot: I haz the power, 2017-05-12 09:08:37]

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-05-12 09:11:26]

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

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

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/20229)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25543)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24446)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/22465)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16974)

[kthelgason, 2017-05-12 11:13:47]

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

[commit-bot: I haz the power, 2017-05-12 11:13: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-05-12 11:18:41]

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

[commit-bot: I haz the power, 2017-05-12 11:18:43]

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...)

[daniela-webrtc, 2017-05-12 11:43:12]

https://codereview.webrtc.org/2862543002/diff/220001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/220001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:330: rtc_source_set("objc_peerconnection_unittests") {
Suggestion to rename this from `peerconnection_unittests` to something like
`objc_sdk_unitttests`. It has (and in future will have even more) classes that
are outside of peerconnection.

[daniela-webrtc, 2017-05-12 11:47:56]

and also lgtm

[kthelgason, 2017-05-12 12:01:40]

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

[commit-bot: I haz the power, 2017-05-12 12:01:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kthelgason, 2017-05-12 12:19:02]

Patchset #12 (id:240001) has been deleted

[kthelgason, 2017-05-12 12:19:21]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2017-05-12 12:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
denicija@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2862543002/#ps260001 (title:
"fix build for simulator")

[commit-bot: I haz the power, 2017-05-12 12:19:31]

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-05-12 12:26:16]

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

[commit-bot: I haz the power, 2017-05-12 12:26:19]

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

[tkchin_webrtc, 2017-05-15 22:45:11]

lgtm

thanks for doing this work! Looks much cleaner now.

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:293: libs = [ "AVFoundation.framework" ]
Looking forward to your deprecation of AVFoundationVideoSource so we can remove
this dep.

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCDataChannelConfiguration.h
(right):

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCDataChannelConfiguration.h:48:
@property(nonatomic, copy) NSString *protocol;
why is this changed in this CL?

[kthelgason, 2017-05-16 07:57:14]

https://codereview.webrtc.org/2862543002/diff/220001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/220001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:330: rtc_source_set("objc_peerconnection_unittests") {
On 2017/05/12 11:43:12, daniela-webrtc wrote:
> Suggestion to rename this from `peerconnection_unittests` to something like
> `objc_sdk_unitttests`. It has (and in future will have even more) classes that
> are outside of peerconnection.

Thanks, agreed.

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:293: libs = [ "AVFoundation.framework" ]
On 2017/05/15 22:45:10, tkchin_webrtc wrote:
> Looking forward to your deprecation of AVFoundationVideoSource so we can
remove
> this dep.

Acknowledged.

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCDataChannelConfiguration.h
(right):

https://codereview.webrtc.org/2862543002/diff/260001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCDataChannelConfiguration.h:48:
@property(nonatomic, copy) NSString *protocol;
On 2017/05/15 22:45:10, tkchin_webrtc wrote:
> why is this changed in this CL?

I have no idea how this happened, thanks for spotting this!

[kthelgason, 2017-05-16 07:57:56]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2017-05-16 07:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
denicija@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2862543002/#ps280001 (title:
"rebase and minor fixes")

[commit-bot: I haz the power, 2017-05-16 07:58:08]

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-05-16 08:04:18]

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

[commit-bot: I haz the power, 2017-05-16 08:04:20]

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

[kthelgason, 2017-05-16 08:06:19]

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

[kthelgason, 2017-05-16 08:06:21]

+ Henrik

The following files still require owner approval:
    webrtc/BUILD.gn
    webrtc/sdk/BUILD.gn
    webrtc/test/BUILD.gn
    webrtc/test/testsupport/iosfileutils.mm

Please take a look when you have time.

[henrika_webrtc, 2017-05-16 14:00:48]

lgtm

[kthelgason, 2017-05-16 14:03:01]

The CQ bit was checked by kthelgason@webrtc.org

[commit-bot: I haz the power, 2017-05-16 14:03:09]

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-05-16 14:07:01]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1494943381265690,
"parent_rev": "860249ec62d371f84f1a885c4a73c783aac609c2", "commit_rev":
"52c83fe7102f566cf35a7533092873d58b38f426"}

[commit-bot: I haz the power, 2017-05-16 14:07:05]

Description was changed from

==========
Split iOS sdk in to separate targets

This CL splits the iOS sdk into separate static libraries for video,
audio, ui, common, and peerconnection-related code. This will in the
future make it easier to compile WebRTC without unneeded components.

BUG=webrtc:4867
==========

to

==========
Split iOS sdk in to separate targets

This CL splits the iOS sdk into separate static libraries for video,
audio, ui, common, and peerconnection-related code. This will in the
future make it easier to compile WebRTC without unneeded components.

BUG=webrtc:4867

Review-Url: https://codereview.webrtc.org/2862543002
Cr-Commit-Position: refs/heads/master@{#18166}
Committed:
https://chromium.googlesource.com/external/webrtc/+/52c83fe7102f566cf35a75330...
==========

[commit-bot: I haz the power, 2017-05-16 14:07:08]

Committed patchset #13 (id:280001) as
https://chromium.googlesource.com/external/webrtc/+/52c83fe7102f566cf35a75330...

[charujain, 2017-05-16 15:06:54]

A revert of this CL (patchset #13 id:280001) has been created in
https://codereview.webrtc.org/2890513002/ by charujain@webrtc.org.

The reason for reverting is: Breaking downstream projects..

[kthelgason, 2017-05-17 11:14:51]

Patchset #14 (id:300001) has been deleted