make it possible to disable libflac and libspeex in content

libflac and libspeex are only used if input_speech==1, so add the dependencies as needed.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192653

[Paweł Hajdan Jr., 2013-04-03 23:53:10]

Mostyn, let's go back a bit here. I'd like to help you - could you explain what
are you trying to do? If you're getting something to compile, could you post the
error messages?

https://codereview.chromium.org/13431003/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:1183: 'enable_libflac%': 0,
This does not look like valid gyp code for me (it's outside of variables).

https://codereview.chromium.org/13431003/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:1186: ['enable_libflac==1', {
This won't work correctly either - I don't see anything that would change the
_actual_ dependency - this code only changes the _declared_ dependency.

[Mostyn Bramley-Moore, 2013-04-04 00:09:08]

On 2013/04/03 23:53:10, Paweł Hajdan Jr. wrote:
> Mostyn, let's go back a bit here. I'd like to help you - could you explain
what
> are you trying to do? If you're getting something to compile, could you post
the
> error messages?

I'm trying to make a build for an embedded linux system, which does not have
system version of libflac or libspeex, and I'm also trying to minimise binary
footprint.

If I simply comment out the flac.gyp:libflac and speex.gyp:libspeex dependencies
then everything builds fine, which led me to believe that these are not really
needed.  

However, on closer inspection it seems that this only works because I have
input_speech==0 - instead I guess we should move the flac/speex dependences into
a condition where input_speech==1 ?

[Paweł Hajdan Jr., 2013-04-04 00:25:30]

On 2013/04/04 00:09:08, Mostyn Bramley-Moore wrote:
> I'm trying to make a build for an embedded linux system, which does not have
> system version of libflac or libspeex, and I'm also trying to minimise binary
> footprint.

Oh, I see.

Well, first thing is don't worry about the use_system_foo switches. These are
mostly for Linux distros and not Google Chrome (although some of them are used
by Chrome).

Looks like you're also trying to remove dependency on bundled libspeex and
libflac. See comments below.

> If I simply comment out the flac.gyp:libflac and speex.gyp:libspeex
dependencies
> then everything builds fine, which led me to believe that these are not really
> needed.  

These libraries are used by content/browser/speech/audio_encoder.cc

> However, on closer inspection it seems that this only works because I have
> input_speech==0 - instead I guess we should move the flac/speex dependences
into
> a condition where input_speech==1 ?

That makes sense to me - looks like you got quite far in working with gyp!

I recommend testing with input_speech=1 (or just default settings).

Please let me know if I can help more - and you can always ask on #chromium or
chromium-dev ML if needed.

[Mostyn Bramley-Moore, 2013-04-04 01:20:27]

On 2013/04/04 00:25:30, Paweł Hajdan Jr. wrote:
> > However, on closer inspection it seems that this only works because I have
> > input_speech==0 - instead I guess we should move the flac/speex dependences
> into
> > a condition where input_speech==1 ?
> 
> That makes sense to me - looks like you got quite far in working with gyp!
> 
> I recommend testing with input_speech=1 (or just default settings).

I tried content_shell builds with patch set 2, with and without input_speech
enabled- libflac/libspeex were built when input_speech was enabled, and not
built when it was disabled.  So it seems to work as expected now.

> Please let me know if I can help more - and you can always ask on #chromium or
> chromium-dev ML if needed.

Thanks :)

[Torne, 2013-04-04 09:49:09]

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gy...
content/content_browser.gypi:1300: 'conditions': [
I generally prefer not to use a nested condition for something this simple; it's
usually clearer to just have a separate 'input_speech==1 and OS!='android'" or
similar rather than nesting another condition in the else branch.

[Mostyn Bramley-Moore, 2013-04-04 10:17:17]

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gy...
content/content_browser.gypi:1300: 'conditions': [
On 2013/04/04 09:49:09, Torne wrote:
> I generally prefer not to use a nested condition for something this simple;
it's
> usually clearer to just have a separate 'input_speech==1 and OS!='android'" or
> similar rather than nesting another condition in the else branch.

Done in patch set 3.

[Mostyn Bramley-Moore, 2013-04-04 10:17:39]

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/6001/content/content_browser.gy...
content/content_browser.gypi:1300: 'conditions': [
On 2013/04/04 10:17:17, Mostyn Bramley-Moore wrote:
> On 2013/04/04 09:49:09, Torne wrote:
> > I generally prefer not to use a nested condition for something this simple;
> it's
> > usually clearer to just have a separate 'input_speech==1 and OS!='android'"
or
> > similar rather than nesting another condition in the else branch.
> 
> Done in patch set 3.

Done.

[Torne, 2013-04-04 10:23:23]

lgtm

https://codereview.chromium.org/13431003/diff/11001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/11001/content/content_browser.g...
content/content_browser.gypi:1302: '../third_party/flac/flac.gyp:libflac',
nit: indentation is wrong here. possibly your editor is using tabs (we use
spaces).

[Mostyn Bramley-Moore, 2013-04-04 10:26:59]

https://codereview.chromium.org/13431003/diff/11001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/11001/content/content_browser.g...
content/content_browser.gypi:1302: '../third_party/flac/flac.gyp:libflac',
On 2013/04/04 10:23:23, Torne wrote:
> nit: indentation is wrong here. possibly your editor is using tabs (we use
> spaces).

Done.

[Paweł Hajdan Jr., 2013-04-04 18:28:36]

Mostyn, just one thing to even further improve it.

Let me know if you'd like me to commit this for you - I think the commit queue
(CQ) should also work.

Just make sure to fix my comment below.

https://codereview.chromium.org/13431003/diff/17001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/17001/content/content_browser.g...
content/content_browser.gypi:1300: ['input_speech==1 and OS!="android"', {
I think for OS==android input_speech is 0 anyway (see build/common.gypi, look
for input_speech).

Please test that, and ideally convert this to just an "else" branch of above
"if".

[Mostyn Bramley-Moore, 2013-04-04 19:21:07]

> Let me know if you'd like me to commit this for you - I think the commit queue

> (CQ) should also work.

Does this go through more testing if we use the CQ?  If so I don't mind using
that method.

https://codereview.chromium.org/13431003/diff/17001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/13431003/diff/17001/content/content_browser.g...
content/content_browser.gypi:1300: ['input_speech==1 and OS!="android"', {
On 2013/04/04 18:28:36, Paweł Hajdan Jr. wrote:
> I think for OS==android input_speech is 0 anyway (see build/common.gypi, look
> for input_speech).
> 
> Please test that, and ideally convert this to just an "else" branch of above
> "if".

Done in patch set 5.

I don't have the android build setup available so I can't test this directly,
but I agree with your analysis of the android setup in build/common.gypi.  I
also looked through some android build logs and it doesn't appear to build
content/browser/speech/audio_encoder.cc.

[Paweł Hajdan Jr., 2013-04-04 19:32:16]

LGTM

CQ is just more automated solution that integrates the trybots and committing.
It's not really more nor less testing than the manual process - just more
convenient. You can read more here:
http://www.chromium.org/developers/testing/commit-queue

[commit-bot: I haz the power, 2013-04-04 19:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/13431003/9002

[commit-bot: I haz the power, 2013-04-04 19:32:33]

Presubmit check for 13431003-9002 failed and returned exit status 1.

INFO:root:Found 1 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/content_browser.gypi

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Paweł Hajdan Jr., 2013-04-04 19:34:40]

John, could you take a look (content/OWNERS) ?

[jam, 2013-04-05 17:58:26]

lgtm

[commit-bot: I haz the power, 2013-04-05 18:57:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/13431003/9002

[commit-bot: I haz the power, 2013-04-05 23:54:31]

Change committed as 192653