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

Review URL: https://chromiumcodereview.appspot.com/10834446

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@152720 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 78 insertions, 32 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index d81d440..4ef9de1 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -88,9 +88,7 @@ ChromotingHost::ChromotingHost(
     // with the NONE config.
     protocol_config_->mutable_audio_configs()->clear();
     protocol_config_->mutable_audio_configs()->push_back(
-        protocol::ChannelConfig(protocol::ChannelConfig::TRANSPORT_NONE,
-                                protocol::kDefaultStreamVersion,
-                                protocol::ChannelConfig::CODEC_VERBATIM));
+        protocol::ChannelConfig());
   }
 }
 
diff --git a/remoting/protocol/content_description.cc b/remoting/protocol/content_description.cc
index eb2a892..61c3ff8 100644
--- a/remoting/protocol/content_description.cc
+++ b/remoting/protocol/content_description.cc
@@ -63,12 +63,14 @@ XmlElement* FormatChannelConfig(const ChannelConfig& config,
   result->AddAttr(QName(kDefaultNs, kTransportAttr),
                   ValueToName(kTransports, config.transport));
 
-  result->AddAttr(QName(kDefaultNs, kVersionAttr),
-                  base::IntToString(config.version));
+  if (config.transport != ChannelConfig::TRANSPORT_NONE) {
+    result->AddAttr(QName(kDefaultNs, kVersionAttr),
+                    base::IntToString(config.version));
 
-  if (config.codec != ChannelConfig::CODEC_UNDEFINED) {
-    result->AddAttr(QName(kDefaultNs, kCodecAttr),
-                    ValueToName(kCodecs, config.codec));
+    if (config.codec != ChannelConfig::CODEC_UNDEFINED) {
+      result->AddAttr(QName(kDefaultNs, kCodecAttr),
+                      ValueToName(kCodecs, config.codec));
+    }
   }
 
   return result;
@@ -79,18 +81,28 @@ 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) {
-    if (!NameToValue(kCodecs, element->Attr(QName(kDefaultNs, kCodecAttr)),
-                     &config->codec)) {
+  // 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;
     }
+
+    // Codec is not required when transport="none".
+    if (codec_required) {
+      if (!NameToValue(kCodecs, element->Attr(QName(kDefaultNs, kCodecAttr)),
+                       &config->codec)) {
+        return false;
+      }
+    } else {
+      config->codec = ChannelConfig::CODEC_UNDEFINED;
+    }
   } else {
+    config->version = 0;
     config->codec = ChannelConfig::CODEC_UNDEFINED;
   }
 
@@ -191,9 +203,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;
 }
diff --git a/remoting/protocol/content_description_unittest.cc b/remoting/protocol/content_description_unittest.cc
index a3ef1d5..25aa8b6 100644
--- a/remoting/protocol/content_description_unittest.cc
+++ b/remoting/protocol/content_description_unittest.cc
@@ -37,7 +37,7 @@ TEST(ContentDescriptionTest, FormatAndParse) {
 
 // Verify that we can still parse configs with transports that we don't
 // recognize.
-TEST(ContentDescription, ParseUnknown) {
+TEST(ContentDescriptionTest, ParseUnknown) {
   std::string kTestDescription =
       "<description xmlns=\"google:remoting\">"
       "  <control transport=\"stream\" version=\"2\"/>"
@@ -57,6 +57,46 @@ TEST(ContentDescription, ParseUnknown) {
                             ChannelConfig::CODEC_UNDEFINED));
 }
 
+// Verify that we can parse configs with none transport without version and
+// codec.
+TEST(ContentDescriptionTest, NoneTransport) {
+  std::string kTestDescription =
+      "<description xmlns=\"google:remoting\">"
+      "  <control transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"stream\" version=\"2\"/>"
+      "  <video transport=\"stream\" version=\"2\" codec=\"vp8\"/>"
+      "  <audio transport=\"none\"/>"
+      "  <authentication/>"
+      "</description>";
+  scoped_ptr<buzz::XmlElement> xml(buzz::XmlElement::ForStr(kTestDescription));
+  scoped_ptr<ContentDescription> parsed(
+      ContentDescription::ParseXml(xml.get()));
+  ASSERT_TRUE(parsed.get());
+  EXPECT_EQ(1U, parsed->config()->audio_configs().size());
+  EXPECT_TRUE(parsed->config()->audio_configs()[0] == ChannelConfig());
+}
+
+// Verify that we can parse configs with none transport with version and
+// codec.
+TEST(ContentDescriptionTest, NoneTransportWithCodec) {
+  std::string kTestDescription =
+      "<description xmlns=\"google:remoting\">"
+      "  <control transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"stream\" version=\"2\"/>"
+      "  <video transport=\"stream\" version=\"2\" codec=\"vp8\"/>"
+      "  <audio transport=\"none\" version=\"2\" codec=\"verbatim\"/>"
+      "  <authentication/>"
+      "</description>";
+  scoped_ptr<buzz::XmlElement> xml(buzz::XmlElement::ForStr(kTestDescription));
+  scoped_ptr<ContentDescription> parsed(
+      ContentDescription::ParseXml(xml.get()));
+  ASSERT_TRUE(parsed.get());
+  EXPECT_EQ(1U, parsed->config()->audio_configs().size());
+  EXPECT_TRUE(parsed->config()->audio_configs()[0] == ChannelConfig());
+}
+
 }  // namespace protocol
 }  // namespace remoting
 
diff --git a/remoting/protocol/session_config.cc b/remoting/protocol/session_config.cc
index f3b2e03..6113552 100644
--- a/remoting/protocol/session_config.cc
+++ b/remoting/protocol/session_config.cc
@@ -11,8 +11,10 @@ namespace protocol {
 
 const int kDefaultStreamVersion = 2;
 
-ChannelConfig::ChannelConfig() {
-  Reset();
+ChannelConfig::ChannelConfig()
+    : transport(TRANSPORT_NONE),
+      version(0),
+      codec(CODEC_UNDEFINED) {
 }
 
 ChannelConfig::ChannelConfig(TransportType transport, int version, Codec codec)
@@ -22,15 +24,12 @@ ChannelConfig::ChannelConfig(TransportType transport, int version, Codec codec)
 }
 
 bool ChannelConfig::operator==(const ChannelConfig& b) const {
+  // If the transport field is set to NONE then all other fields are irrelevant.
+  if (transport == ChannelConfig::TRANSPORT_NONE)
+    return transport == b.transport;
   return transport == b.transport && version == b.version && codec == b.codec;
 }
 
-void ChannelConfig::Reset() {
-  transport = TRANSPORT_STREAM;
-  version = kDefaultStreamVersion;
-  codec = CODEC_UNDEFINED;
-}
-
 SessionConfig::SessionConfig() {
 }
 
@@ -205,10 +204,7 @@ scoped_ptr<CandidateSessionConfig> CandidateSessionConfig::CreateDefault() {
                     kDefaultStreamVersion,
                     ChannelConfig::CODEC_VERBATIM));
 #endif  // defined(ENABLE_REMOTING_AUDIO)
-  result->mutable_audio_configs()->push_back(
-      ChannelConfig(ChannelConfig::TRANSPORT_NONE,
-                    kDefaultStreamVersion,
-                    ChannelConfig::CODEC_VERBATIM));
+  result->mutable_audio_configs()->push_back(ChannelConfig());
   return result.Pass();
 }
 
diff --git a/remoting/protocol/session_config.h b/remoting/protocol/session_config.h
index 58d3d78c..5d89d67 100644
--- a/remoting/protocol/session_config.h
+++ b/remoting/protocol/session_config.h
@@ -36,15 +36,17 @@ struct ChannelConfig {
     CODEC_SPEEX,
   };
 
+  // The constructor that creates a config with transport field set to
+  // TRANSPORT_NONE which indicates that corresponding channel is disabled.
   ChannelConfig();
+
+  // Creates a channel config with the specified parameters.
   ChannelConfig(TransportType transport, int version, Codec codec);
 
   // operator== is overloaded so that std::find() works with
   // std::vector<ChannelConfig>.
   bool operator==(const ChannelConfig& b) const;
 
-  void Reset();
-
   TransportType transport;
   int version;
   Codec codec;