Improve handling of NONE transport in channel configuration.

remoting/protocol/content_description.cc

Index: remoting/protocol/content_description.cc
diff --git a/remoting/protocol/content_description.cc b/remoting/protocol/content_description.cc
index 9f7d5d54f0f30c2285bc4dbf25125838461f67ee..ae78dbc5d5d1ec3ec5d4cf20f07b54594e7a5621 100644
--- a/remoting/protocol/content_description.cc
+++ b/remoting/protocol/content_description.cc
@@ -62,6 +62,12 @@ XmlElement* FormatChannelConfig(const ChannelConfig& config,
   result->AddAttr(QName(kDefaultNs, kTransportAttr),
                   ValueToName(kTransports, config.transport));
 
+  // TODO(sergeyu): Here we add version and codec attributes even when transport
+  // is set to NONE. This is needed for backward compatibility only. Fix this
+  // code once the current stable version is able to cope with missing version
+  // and codec attributes.
+  // crbug.com/144053

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

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?

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

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

+
   result->AddAttr(QName(kDefaultNs, kVersionAttr),
                   base::IntToString(config.version));
 
@@ -78,13 +84,22 @@ bool ParseChannelConfig(const XmlElement* element, bool codec_required,
                         ChannelConfig* config) {
   if (!NameToValue(
           kTransports, element->Attr(QName(kDefaultNs, kTransportAttr)),
-          &config->transport) ||
-      !base::StringToInt(element->Attr(QName(kDefaultNs, kVersionAttr)),
-                         &config->version)) {
+          &config->transport)) {
     return false;
   }
 
-  if (codec_required) {
+  // Version is not required when transport="none".
+  if (config->transport != ChannelConfig::TRANSPORT_NONE) {
+    if (!base::StringToInt(element->Attr(QName(kDefaultNs, kVersionAttr)),
+                           &config->version)) {
+      return false;
+    }
+  } else {
+    config->version = 0;
+  }
+
+  // Codec is not required when transport="none".
+  if (codec_required && config->transport != ChannelConfig::TRANSPORT_NONE) {

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

nit: Cleaner to have a single config->transport != ChannelConfig::TRANSPORT_NONE
test, with the version & codec parsing code in the block.

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

Done.

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

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.

[Sergey Ulanov, 2012/08/22 00:37:42]

Done.

     if (!NameToValue(kCodecs, element->Attr(QName(kDefaultNs, kCodecAttr)),
                      &config->codec)) {
       return false;
@@ -150,7 +165,16 @@ XmlElement* ContentDescription::ToXml() const {
 
   for (it = config()->audio_configs().begin();
        it != config()->audio_configs().end(); ++it) {
-    root->AddElement(FormatChannelConfig(*it, kAudioTag));
+    ChannelConfig config = *it;
+    if (config.transport == ChannelConfig::TRANSPORT_NONE) {
+      // Older client and host may expect the following values for for the
+      // version and codec for the NONE audio config.
+      // TODO(sergeyu): Remove this code once the current version supports.
+      // crbug.com/144053

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

This only affects clients which support optional channels; isn't that just M22?

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

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

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

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

[Sergey Ulanov, 2012/08/22 00:37:42]

Yes

+      config.version = 2;

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

nit: kDefaultVersion?

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

Removed this code.

+      config.codec = ChannelConfig::CODEC_VERBATIM;
+    }
+    root->AddElement(FormatChannelConfig(config, kAudioTag));
   }
 
   // Older endpoints require an initial-resolution tag, but otherwise ignore it.
@@ -190,9 +214,7 @@ bool ContentDescription::ParseChannelConfigs(
   if (optional && configs->empty()) {
       // If there's no mention of the tag, implicitly assume
       // TRANSPORT_NONE for the channel.
-      configs->push_back(ChannelConfig(ChannelConfig::TRANSPORT_NONE,
-                                       kDefaultStreamVersion,
-                                       ChannelConfig::CODEC_VERBATIM));
+      configs->push_back(ChannelConfig());
   }
   return true;
 }