Add a gyp/gn variable for whether to use iLBC or not

Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
Committed: https://crrev.com/f8c2baca4e7d61d90a73ef1145a359a4b6166b62
Cr-Commit-Position: refs/heads/master@{#11291}

[kwiberg-webrtc, 2016-01-12 15:31:35]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-01-12 15:48:59]

Description was changed from

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
==========

to

==========
Add a gyp/gn variable for whether to use iLBC or not

Notably, it defaults to false, and is overridden to true for
standalone WebRTC builds (which need it for testing). This means that
third parties that use WebRTC won't get iLBC anymore unless they start
explicitly asking for it.

BUG=webrtc:5415
==========

[kwiberg-webrtc, 2016-01-12 15:48:59]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kjellander@webrtc.org

[kwiberg-webrtc, 2016-01-12 15:52:08]

Please review. Note that this is slightly different than what bug 5415 asks for,
since it also changes the default to not include iLBC.

[kjellander_webrtc, 2016-01-12 16:14:51]

lgtm

[hlundin-webrtc, 2016-01-12 16:15:41]

LG, but see my comment.

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/build/common.gypi#...
webrtc/build/common.gypi:44: 'include_ilbc%': 0,
I'm not sure we want this to be default off. This does change the behavior for
non-chrome non-firefox integrations of WebRTC. We should probably announce that,
at the very least.

[kjellander_webrtc, 2016-01-12 16:23:27]

changed my mind, this does not lgtm until it's possible to also disable it for
the standalone build.

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:20: 'include_ilbc': 1,
ah, I missed this. Please make this (note the %):
'include_ilbc%': 1,

so it is possible to override it, even for the standalone build.
Please also test that it works to disable for the standalone build by:
webrtc/build/gyp_webrtc -Dinclude_iblc=0

Same for GN: please verify it's possible to turn it off with:
gn gen out/Default --args=rtc_include_ilbc=false

[kwiberg-webrtc, 2016-01-14 10:59:31]

Yo! New patch set! Check it out!

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/build/common.gypi#...
webrtc/build/common.gypi:44: 'include_ilbc%': 0,
On 2016/01/12 16:15:41, hlundin-webrtc wrote:
> I'm not sure we want this to be default off. This does change the behavior for
> non-chrome non-firefox integrations of WebRTC. We should probably announce
that,
> at the very least.

I changed it to the same default as before. See patch set #3.

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/supplement.gypi
File webrtc/supplement.gypi (right):

https://codereview.webrtc.org/1578953003/diff/20001/webrtc/supplement.gypi#ne...
webrtc/supplement.gypi:20: 'include_ilbc': 1,
On 2016/01/12 16:23:27, kjellander (webrtc) wrote:
> ah, I missed this. Please make this (note the %):
> 'include_ilbc%': 1,
> 
> so it is possible to override it, even for the standalone build.
> Please also test that it works to disable for the standalone build by:
> webrtc/build/gyp_webrtc -Dinclude_iblc=0

This typo cost me at least half an hour. I couldn't figure out why setting the
variable had no effect... :-)

> Same for GN: please verify it's possible to turn it off with:
> gn gen out/Default --args=rtc_include_ilbc=false

Patch set #3 should address all your concerns. Among other things, it no longer
modifies supplement.gypi at all.

[kwiberg-webrtc, 2016-01-14 12:12:13]

Description was changed from

==========
Add a gyp/gn variable for whether to use iLBC or not

Notably, it defaults to false, and is overridden to true for
standalone WebRTC builds (which need it for testing). This means that
third parties that use WebRTC won't get iLBC anymore unless they start
explicitly asking for it.

BUG=webrtc:5415
==========

to

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
==========

[kwiberg-webrtc, 2016-01-14 13:29:51]

(Hmm, what's with the libfuzzer bot failures? I can't see how they could be my
fault, but they reproduce, so it isn't just a random glitch...)

[hlundin-webrtc, 2016-01-18 11:48:51]

lgtm

[kjellander_webrtc, 2016-01-18 13:03:59]

lgtm

[kwiberg-webrtc, 2016-01-18 13:05:38]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-01-18 13:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578953003/60001

[commit-bot: I haz the power, 2016-01-18 14:38:35]

Description was changed from

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
==========

to

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
==========

[commit-bot: I haz the power, 2016-01-18 14:38:36]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-01-18 14:38:46]

Description was changed from

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415
==========

to

==========
Add a gyp/gn variable for whether to use iLBC or not

BUG=webrtc:5415

Committed: https://crrev.com/f8c2baca4e7d61d90a73ef1145a359a4b6166b62
Cr-Commit-Position: refs/heads/master@{#11291}
==========

[commit-bot: I haz the power, 2016-01-18 14:38:47]

Patchset 3 (id:??) landed as
https://crrev.com/f8c2baca4e7d61d90a73ef1145a359a4b6166b62
Cr-Commit-Position: refs/heads/master@{#11291}