Enabling 'gn check' on webrtc/sdk

Enabling 'gn check' on webrtc/sdk

BUG=webrtc:7499
Review-Url: https://codereview.webrtc.org/2818433003
Cr-Commit-Position: refs/heads/master@{#17805}
Committed: https://chromium.googlesource.com/external/webrtc/+/1e060c6b0cf72c805209820330968ea7d64208b7

[mbonadei, 2017-04-12 14:26:24]

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

[kjellander_webrtc, 2017-04-12 14:37:03]

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

[kjellander_webrtc, 2017-04-12 14:37:07]

Most of it looks good but I'd like Magnus' input on the default metrics/field
trial implementations that are pulled in. I suspect they might break things
downstream

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:33: "../system_wrappers:field_trial_default",
I suspect this might cause problems for consumers of the target that wants to
provide their own implementations. Which files are pulling these in? Can we
refactor out what they're referencing so we don't have to depend on the
implementations?

I'd like Magnus' input on this.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:431: "../common_video:common_video",
Leave out :common_video here for improved readability. Same earlier in the file.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:435: "../system_wrappers:system_wrappers",
You can leave out the :system_wrappers part.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD.gn
File webrtc/sdk/android/BUILD.gn (right):

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD....
webrtc/sdk/android/BUILD.gn:81: "../../system_wrappers:field_trial_default",
Similar question here as before. I don't think we should have default field
trial implementation here. Can you share details on what's pulling it in?

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD....
webrtc/sdk/android/BUILD.gn:107: "../../system_wrappers:system_wrappers",
Leave out :system_wrappers

[mbonadei, 2017-04-13 08:12:03]

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:33: "../system_wrappers:field_trial_default",
On 2017/04/12 14:37:04, kjellander_webrtc wrote:
> I suspect this might cause problems for consumers of the target that wants to
> provide their own implementations. Which files are pulling these in? Can we
> refactor out what they're referencing so we don't have to depend on the
> implementations?
> 
> I'd like Magnus' input on this.

The header 'system_wrappers/include/field_trial_default.h' is pulled in by
'webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm' and
'webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h'.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:431: "../common_video:common_video",
On 2017/04/12 14:37:04, kjellander_webrtc wrote:
> Leave out :common_video here for improved readability. Same earlier in the
file.

Done.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:435: "../system_wrappers:system_wrappers",
On 2017/04/12 14:37:04, kjellander_webrtc wrote:
> You can leave out the :system_wrappers part.

Done.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD.gn
File webrtc/sdk/android/BUILD.gn (right):

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD....
webrtc/sdk/android/BUILD.gn:81: "../../system_wrappers:field_trial_default",
On 2017/04/12 14:37:04, kjellander_webrtc wrote:
> Similar question here as before. I don't think we should have default field
> trial implementation here. Can you share details on what's pulling it in?

Answered on the other question.

https://codereview.webrtc.org/2818433003/diff/60001/webrtc/sdk/android/BUILD....
webrtc/sdk/android/BUILD.gn:107: "../../system_wrappers:system_wrappers",
On 2017/04/12 14:37:04, kjellander_webrtc wrote:
> Leave out :system_wrappers

Done.

[mbonadei, 2017-04-13 08:34:30]

In patch set #6 I am only fixing some Gn coding standards (basically
moving deps at the bottom and cflags, defines, configs after sources).

[magjed_webrtc, 2017-04-13 09:30:38]

Like kjellander@ says, consumers of webrtc/sdk/objc should preferably be able to
provide their own implementation of field_trial and metrics, instead of being
forced to use the default implementation.

You can see how we did this on android in CL
https://codereview.webrtc.org/2518293002/. We need to split out the default
stuff into separate targets, e.g. rtc_sdk_objc_metrics_default and
rtc_sdk_objc_field_trial_default and consumers can choose to include them or
provide their own.

That said, we are already forcing consumers to use the default implementation on
ObjC, and this CL just makes it more explicit, right? Then I don't have a
problem with landing this CL and fixing the dependency on default in a future
CL.

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

https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:214: "../common_video:common_video",
Remove unnecessary common_video.

https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:217: "../system_wrappers:system_wrappers",
Remove unnecessary system_wrappers.

[mbonadei, 2017-04-19 09:41:43]

I have excluded the two system_wrappers headers from 'gn check' so we can avoid
to compile 'system_wrappers:field_trial_default' and
'system_wrappers:metrics_default'
when we want to build 'rtc_sdk_common_objc'.

This should give us the benefit of enabling 'gn check' on the 'webrtc/sdk'
directory 
while we postpone the refactoring.

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

https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:214: "../common_video:common_video",
On 2017/04/13 09:30:37, magjed_webrtc wrote:
> Remove unnecessary common_video.

Done.

https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
webrtc/sdk/BUILD.gn:217: "../system_wrappers:system_wrappers",
On 2017/04/13 09:30:37, magjed_webrtc wrote:
> Remove unnecessary system_wrappers.

Done.

[kjellander_webrtc, 2017-04-19 10:02:56]

On 2017/04/19 09:41:43, mbonadei wrote:
> I have excluded the two system_wrappers headers from 'gn check' so we can
avoid
> to compile 'system_wrappers:field_trial_default' and
> 'system_wrappers:metrics_default'
> when we want to build 'rtc_sdk_common_objc'.
> 
> This should give us the benefit of enabling 'gn check' on the 'webrtc/sdk'
> directory 
> while we postpone the refactoring.

Please reference https://bugs.chromium.org/p/webrtc/issues/detail?id=7499 which
I just filed.

> 
> https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn
> File webrtc/sdk/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
> webrtc/sdk/BUILD.gn:214: "../common_video:common_video",
> On 2017/04/13 09:30:37, magjed_webrtc wrote:
> > Remove unnecessary common_video.
> 
> Done.
> 
>
https://codereview.webrtc.org/2818433003/diff/100001/webrtc/sdk/BUILD.gn#newc...
> webrtc/sdk/BUILD.gn:217: "../system_wrappers:system_wrappers",
> On 2017/04/13 09:30:37, magjed_webrtc wrote:
> > Remove unnecessary system_wrappers.
> 
> Done.

[kjellander_webrtc, 2017-04-19 10:04:39]

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

https://codereview.webrtc.org/2818433003/diff/140001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:83: "../../system_wrappers:field_trial_default",
I don't think we should have this.

[mbonadei, 2017-04-19 11:00:15]

Description was changed from

==========
Enabling 'gn check' on webrtc/sdk

BUG=webrtc:6828
==========

to

==========
Enabling 'gn check' on webrtc/sdk

BUG=webrtc:7499
==========

[kjellander_webrtc, 2017-04-19 13:41:33]

https://codereview.webrtc.org/2818433003/diff/160001/webrtc/sdk/android/BUILD.gn
File webrtc/sdk/android/BUILD.gn (left):

https://codereview.webrtc.org/2818433003/diff/160001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:95: "//webrtc/system_wrappers:field_trial_default",
This needs to stay, removed by accident? Even if the name says
"metrics_default", I think it's meant to include field trial as well.

[mbonadei, 2017-04-19 14:06:35]

https://codereview.webrtc.org/2818433003/diff/160001/webrtc/sdk/android/BUILD.gn
File webrtc/sdk/android/BUILD.gn (left):

https://codereview.webrtc.org/2818433003/diff/160001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:95: "//webrtc/system_wrappers:field_trial_default",
On 2017/04/19 13:41:33, kjellander_webrtc wrote:
> This needs to stay, removed by accident? Even if the name says
> "metrics_default", I think it's meant to include field trial as well.

Yes, this is an error. Thanks.

[kjellander_webrtc, 2017-04-19 15:11:30]

lgtm

[magjed_webrtc, 2017-04-20 12:22:52]

lgtm

[mbonadei, 2017-04-21 06:38:25]

The CQ bit was checked by mbonadei@webrtc.org

[commit-bot: I haz the power, 2017-04-21 06:38: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-04-21 07:02:03]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492756705591090,
"parent_rev": "bf34b23ab577b73db62005d814bf535696fc524c", "commit_rev":
"1e060c6b0cf72c805209820330968ea7d64208b7"}

[commit-bot: I haz the power, 2017-04-21 07:02:07]

Description was changed from

==========
Enabling 'gn check' on webrtc/sdk

BUG=webrtc:7499
==========

to

==========
Enabling 'gn check' on webrtc/sdk

BUG=webrtc:7499

Review-Url: https://codereview.webrtc.org/2818433003
Cr-Commit-Position: refs/heads/master@{#17805}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1e060c6b0cf72c80520982033...
==========

[commit-bot: I haz the power, 2017-04-21 07:02:07]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/1e060c6b0cf72c80520982033...