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

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

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_;

remoting/protocol/content_description_unittest.cc

+// 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
+

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;

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',