Skip unknown channel configurations when parsing session config.

We may need to add new transport types in the future. Current config
parsing code fails to parse configs with transport types it doesn't 
understand which will make it hard to add new transport type without
breaking backward compatibility.

BUG=137135


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

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

5 files changed, 92 insertions, 35 deletions

diff --git a/remoting/protocol/content_description.cc b/remoting/protocol/content_description.cc
index 0cd7ce4..70ba05b 100644
--- a/remoting/protocol/content_description.cc
+++ b/remoting/protocol/content_description.cc
@@ -235,9 +235,9 @@ bool ContentDescription::ParseChannelConfigs(
   const XmlElement* child = element->FirstNamed(tag);
   while (child) {
     ChannelConfig channel_config;
-    if (!ParseChannelConfig(child, codec_required, &channel_config))
-      return false;
-    configs->push_back(channel_config);
+    if (ParseChannelConfig(child, codec_required, &channel_config)) {
+      configs->push_back(channel_config);
+    }
     child = child->NextNamed(tag);
   }
   if (optional && configs->empty()) {
@@ -251,39 +251,32 @@ bool ContentDescription::ParseChannelConfigs(
 }
 
 // static
-ContentDescription* ContentDescription::ParseXml(
+scoped_ptr<ContentDescription> ContentDescription::ParseXml(
     const XmlElement* element) {
-  if (element->Name() == QName(kChromotingXmlNamespace, kDescriptionTag)) {
-    scoped_ptr<CandidateSessionConfig> config(
-        CandidateSessionConfig::CreateEmpty());
-    const XmlElement* child = NULL;
-
-    if (!ParseChannelConfigs(element, kControlTag, false, false,
-                             config->mutable_control_configs())) {
-      return NULL;
-    }
-    if (!ParseChannelConfigs(element, kEventTag, false, false,
-                             config->mutable_event_configs())) {
-      return NULL;
-    }
-    if (!ParseChannelConfigs(element, kVideoTag, true, false,
-                             config->mutable_video_configs())) {
-      return NULL;
-    }
-    if (!ParseChannelConfigs(element, kAudioTag, true, true,
-                             config->mutable_audio_configs())) {
-      return NULL;
-    }
+  if (element->Name() != QName(kChromotingXmlNamespace, kDescriptionTag)) {
+    LOG(ERROR) << "Invalid description: " << element->Str();
+    return scoped_ptr<ContentDescription>();
+  }
+  scoped_ptr<CandidateSessionConfig> config(
+      CandidateSessionConfig::CreateEmpty());
+  if (!ParseChannelConfigs(element, kControlTag, false, false,
+                           config->mutable_control_configs()) ||
+      !ParseChannelConfigs(element, kEventTag, false, false,
+                           config->mutable_event_configs()) ||
+      !ParseChannelConfigs(element, kVideoTag, true, false,
+                           config->mutable_video_configs()) ||
+      !ParseChannelConfigs(element, kAudioTag, true, true,
+                           config->mutable_audio_configs())) {
+    return scoped_ptr<ContentDescription>();
+  }
 
-    scoped_ptr<XmlElement> authenticator_message;
-    child = Authenticator::FindAuthenticatorMessage(element);
-    if (child)
-      authenticator_message.reset(new XmlElement(*child));
+  scoped_ptr<XmlElement> authenticator_message;
+  const XmlElement* child = Authenticator::FindAuthenticatorMessage(element);
+  if (child)
+    authenticator_message.reset(new XmlElement(*child));
 
-    return new ContentDescription(config.Pass(), authenticator_message.Pass());
-  }
-  LOG(ERROR) << "Invalid description: " << element->Str();
-  return NULL;
+  return scoped_ptr<ContentDescription>(
+      new ContentDescription(config.Pass(), authenticator_message.Pass()));
 }
 
 }  // namespace protocol
diff --git a/remoting/protocol/content_description.h b/remoting/protocol/content_description.h
index fe4ad3a..d345507 100644
--- a/remoting/protocol/content_description.h
+++ b/remoting/protocol/content_description.h
@@ -44,7 +44,8 @@ class ContentDescription : public cricket::ContentDescription {
 
   buzz::XmlElement* ToXml() const;
 
-  static ContentDescription* ParseXml(const buzz::XmlElement* element);
+  static scoped_ptr<ContentDescription> ParseXml(
+      const buzz::XmlElement* element);
 
  private:
   scoped_ptr<const CandidateSessionConfig> candidate_config_;
diff --git a/remoting/protocol/content_description_unittest.cc b/remoting/protocol/content_description_unittest.cc
new file mode 100644
index 0000000..a3ef1d5
--- /dev/null
+++ b/remoting/protocol/content_description_unittest.cc
@@ -0,0 +1,62 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/content_description.h"
+
+#include <string>
+
+#include "base/logging.h"
+#include "remoting/protocol/authenticator.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+namespace remoting {
+namespace protocol {
+
+TEST(ContentDescriptionTest, FormatAndParse) {
+  scoped_ptr<CandidateSessionConfig> config =
+      CandidateSessionConfig::CreateDefault();
+  ContentDescription description(
+      config.Pass(), Authenticator::CreateEmptyAuthenticatorMessage());
+  scoped_ptr<buzz::XmlElement> xml(description.ToXml());
+  LOG(ERROR) << xml->Str();
+  scoped_ptr<ContentDescription> parsed(
+      ContentDescription::ParseXml(xml.get()));
+  ASSERT_TRUE(parsed.get());
+  EXPECT_TRUE(description.config()->control_configs() ==
+              parsed->config()->control_configs());
+  EXPECT_TRUE(description.config()->video_configs() ==
+              parsed->config()->video_configs());
+  EXPECT_TRUE(description.config()->event_configs() ==
+              parsed->config()->event_configs());
+  EXPECT_TRUE(description.config()->audio_configs() ==
+              parsed->config()->audio_configs());
+}
+
+// Verify that we can still parse configs with transports that we don't
+// recognize.
+TEST(ContentDescription, ParseUnknown) {
+  std::string kTestDescription =
+      "<description xmlns=\"google:remoting\">"
+      "  <control transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"stream\" version=\"2\"/>"
+      "  <event transport=\"new_awesome_transport\" version=\"3\"/>"
+      "  <video transport=\"stream\" version=\"2\" codec=\"vp8\"/>"
+      "  <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()->event_configs().size());
+  EXPECT_TRUE(parsed->config()->event_configs()[0] ==
+              ChannelConfig(ChannelConfig::TRANSPORT_STREAM,
+                            kDefaultStreamVersion,
+                            ChannelConfig::CODEC_UNDEFINED));
+}
+
+}  // namespace protocol
+}  // namespace remoting
+
diff --git a/remoting/protocol/jingle_messages.cc b/remoting/protocol/jingle_messages.cc
index 044e183..cc55c76 100644
--- a/remoting/protocol/jingle_messages.cc
+++ b/remoting/protocol/jingle_messages.cc
@@ -259,7 +259,7 @@ bool JingleMessage::ParseXml(const buzz::XmlElement* stanza,
       return false;
     }
 
-    description.reset(ContentDescription::ParseXml(description_tag));
+    description = ContentDescription::ParseXml(description_tag);
     if (!description.get()) {
       *error = "Failed to parse content description";
       return false;
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index 1ffb4cc..691d7d3 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -1738,6 +1738,7 @@
         'protocol/connection_tester.cc',
         'protocol/connection_tester.h',
         'protocol/connection_to_client_unittest.cc',
+        'protocol/content_description_unittest.cc',
         'protocol/fake_authenticator.cc',
         'protocol/fake_authenticator.h',
         'protocol/fake_session.cc',