Improve handling of NONE transport in channel configuration.

Improve handling of NONE transport in channel configuration.

Previously the session description parser always expected version and codec
attributes for each channel config. These attributes do not make sense
for NONE transport (i.e. when channel is disabled). Now the parser
accepts configs without these attributes and doesn't add them when formatting
outgoing messages.

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

[Wez, 2012-08-22 00:08:30]

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:69: // crbug.com/144053
The codec case explicitly checks for CODEC_UNDEFINED and doesn't add it in that
case, so does this comment only apply to the version?

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:102: if (codec_required &&
config->transport != ChannelConfig::TRANSPORT_NONE) {
nit: Cleaner to have a single config->transport != ChannelConfig::TRANSPORT_NONE
test, with the version & codec parsing code in the block.

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:173: // crbug.com/144053
This only affects clients which support optional channels; isn't that just M22?

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:174: config.version = 2;
nit: kDefaultVersion?

[Sergey Ulanov, 2012-08-22 00:18:27]

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:69: // crbug.com/144053
On 2012/08/22 00:08:30, Wez wrote:
> The codec case explicitly checks for CODEC_UNDEFINED and doesn't add it in
that
> case, so does this comment only apply to the version?

Yes. Anyway I removed this comment and changed these code to add these
attributes when transport is not NONE

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:102: if (codec_required &&
config->transport != ChannelConfig::TRANSPORT_NONE) {
On 2012/08/22 00:08:30, Wez wrote:
> nit: Cleaner to have a single config->transport !=
ChannelConfig::TRANSPORT_NONE
> test, with the version & codec parsing code in the block.

Done.

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:173: // crbug.com/144053
On 2012/08/22 00:08:30, Wez wrote:
> This only affects clients which support optional channels; isn't that just
M22?

Yeah, Actually I think that M22 should be able to handle it properly as well -
it just skips the configs it cannot parse, so we don't really need this
compatibility hack

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:174: config.version = 2;
On 2012/08/22 00:08:30, Wez wrote:
> nit: kDefaultVersion?
Removed this code.

[Wez, 2012-08-22 00:26:54]

LGTM w/ nits.

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:102: if (codec_required &&
config->transport != ChannelConfig::TRANSPORT_NONE) {
On 2012/08/22 00:18:27, sergeyu wrote:
> On 2012/08/22 00:08:30, Wez wrote:
> > nit: Cleaner to have a single config->transport !=
> ChannelConfig::TRANSPORT_NONE
> > test, with the version & codec parsing code in the block.
> 
> Done.

What I meant was actually to put the version & codec attr parsing inside a
_single_ block that gets executed iff TRANSPORT_NONE is not set.

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:173: // crbug.com/144053
On 2012/08/22 00:18:27, sergeyu wrote:
> On 2012/08/22 00:08:30, Wez wrote:
> > This only affects clients which support optional channels; isn't that just
> M22?
> 
> Yeah, Actually I think that M22 should be able to handle it properly as well -
> it just skips the configs it cannot parse, so we don't really need this
> compatibility hack

And if no configurations match then it'll fall back to the default
TRANSPORT_NONE config?

[Sergey Ulanov, 2012-08-22 00:37:41]

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:102: if (codec_required &&
config->transport != ChannelConfig::TRANSPORT_NONE) {
On 2012/08/22 00:26:54, Wez wrote:
> On 2012/08/22 00:18:27, sergeyu wrote:
> > On 2012/08/22 00:08:30, Wez wrote:
> > > nit: Cleaner to have a single config->transport !=
> > ChannelConfig::TRANSPORT_NONE
> > > test, with the version & codec parsing code in the block.
> > 
> > Done.
> 
> What I meant was actually to put the version & codec attr parsing inside a
> _single_ block that gets executed iff TRANSPORT_NONE is not set.

Done.

http://codereview.chromium.org/10834446/diff/2001/remoting/protocol/content_d...
remoting/protocol/content_description.cc:173: // crbug.com/144053
On 2012/08/22 00:26:54, Wez wrote:
> On 2012/08/22 00:18:27, sergeyu wrote:
> > On 2012/08/22 00:08:30, Wez wrote:
> > > This only affects clients which support optional channels; isn't that just
> > M22?
> > 
> > Yeah, Actually I think that M22 should be able to handle it properly as well
-
> > it just skips the configs it cannot parse, so we don't really need this
> > compatibility hack
> 
> And if no configurations match then it'll fall back to the default
> TRANSPORT_NONE config?
Yes

[commit-bot: I haz the power, 2012-08-22 01:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10834446/1008

[commit-bot: I haz the power, 2012-08-22 03:00:52]

Try job failure for 10834446-1008 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...