Created define to switch on/off audio.

Created define to switch on/off audio.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148223

[kxing, 2012-07-23 21:12:52]

Could someone PTAL?

[Sergey Ulanov, 2012-07-23 22:17:48]

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:178: #if ENABLE_AUDIO
#if defined(ENABLE_REMOTING_AUDIO)

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:187: #else
#else  // !defined(ENABLE_REMOTING_AUDIO)

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:192:
result->mutable_audio_configs()->push_back(
I think it's better to remove this config for now until our protocol is stable.
Otherwise in the future we may get into situation when an old client connects to
a newer host, the host chooses this config, but then they try to communicate
using incompatible protocol.

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:196: #endif
#endig  // !defined(ENABLE_REMOTING_AUDIO)

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp#newcode1511
remoting/remoting.gyp:1511: 'ENABLE_AUDIO=0',
ENABLE_REMOTING_AUDIO

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp#newcode1511
remoting/remoting.gyp:1511: 'ENABLE_AUDIO=0',
It's preferred to define ENABLE_AUDIO=1 when it's enabled and don't define
anything at all when it is disabled. Add remoting_audio variable in the
variables section and then add condition that would define
ENABLE_REMOTING_AUDIO=1 in case remoting_audio=1. That way you would be able to
define that value in GYP_DEFINES without changing gyp file. Take a look at
use_pulseaudio in media/media.gyp for an example.

[kxing, 2012-07-24 15:02:16]

Could someone PTAL again?

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:178: #if ENABLE_AUDIO
On 2012/07/23 22:17:48, sergeyu wrote:
> #if defined(ENABLE_REMOTING_AUDIO)

Done.

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:187: #else
On 2012/07/23 22:17:48, sergeyu wrote:
> #else  // !defined(ENABLE_REMOTING_AUDIO)

Done.

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:192:
result->mutable_audio_configs()->push_back(
On 2012/07/23 22:17:48, sergeyu wrote:
> I think it's better to remove this config for now until our protocol is
stable.
> Otherwise in the future we may get into situation when an old client connects
to
> a newer host, the host chooses this config, but then they try to communicate
> using incompatible protocol.

Done.

http://codereview.chromium.org/10795075/diff/1/remoting/protocol/session_conf...
remoting/protocol/session_config.cc:196: #endif
On 2012/07/23 22:17:48, sergeyu wrote:
> #endig  // !defined(ENABLE_REMOTING_AUDIO)

Done.

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp#newcode1511
remoting/remoting.gyp:1511: 'ENABLE_AUDIO=0',
On 2012/07/23 22:17:48, sergeyu wrote:
> ENABLE_REMOTING_AUDIO

Done.

http://codereview.chromium.org/10795075/diff/1/remoting/remoting.gyp#newcode1511
remoting/remoting.gyp:1511: 'ENABLE_AUDIO=0',
On 2012/07/23 22:17:48, sergeyu wrote:
> It's preferred to define ENABLE_AUDIO=1 when it's enabled and don't define
> anything at all when it is disabled. Add remoting_audio variable in the
> variables section and then add condition that would define
> ENABLE_REMOTING_AUDIO=1 in case remoting_audio=1. That way you would be able
to
> define that value in GYP_DEFINES without changing gyp file. Take a look at
> use_pulseaudio in media/media.gyp for an example.

Done.

[Sergey Ulanov, 2012-07-24 19:49:59]

LGTM when my nit is addressed.

http://codereview.chromium.org/10795075/diff/6001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/6001/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1511: 'conditions': [
Please move this condition to target_defaults section so it applies to all
targets, not only remoting_protocol.

[kxing, 2012-07-24 20:11:52]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10795075/diff/6001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/6001/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1511: 'conditions': [
On 2012/07/24 19:49:59, sergeyu wrote:
> Please move this condition to target_defaults section so it applies to all
> targets, not only remoting_protocol.

Done.

[Sergey Ulanov, 2012-07-24 20:26:21]

http://codereview.chromium.org/10795075/diff/10001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/10001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:196: 'conditions': [
nit: it's more common to put conditions at then end of a tartget instead of
beginning. Move it after include_dirs

[kxing, 2012-07-24 20:29:22]

Moved 'conditions' after 'include_dirs'.

http://codereview.chromium.org/10795075/diff/10001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10795075/diff/10001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:196: 'conditions': [
On 2012/07/24 20:26:21, sergeyu wrote:
> nit: it's more common to put conditions at then end of a tartget instead of
> beginning. Move it after include_dirs

Done.

[commit-bot: I haz the power, 2012-07-24 20:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10795075/8002

[commit-bot: I haz the power, 2012-07-24 22:14:44]

Change committed as 148223