Improve handling of NONE transport in channel configuration.

remoting/protocol/content_description.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 "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "remoting/base/constants.h"
 #include "remoting/protocol/authenticator.h"
@@ skipping 44 matching lines @@
 // e.g. for video channel:
 //    <video transport="stream" version="1" codec="vp8" />
 XmlElement* FormatChannelConfig(const ChannelConfig& config,
                                 const std::string& tag_name) {
   XmlElement* result = new XmlElement(
       QName(kChromotingXmlNamespace, tag_name));
 
   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));
 
   if (config.codec != ChannelConfig::CODEC_UNDEFINED) {
     result->AddAttr(QName(kDefaultNs, kCodecAttr),
                     ValueToName(kCodecs, config.codec));
   }
 
   return result;
 }
 
 // Returns false if the element is invalid.
 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;
     }
   } else {
     config->codec = ChannelConfig::CODEC_UNDEFINED;
   }
 
   return true;
 }
@@ skipping 45 matching lines @@
     root->AddElement(FormatChannelConfig(*it, kEventTag));
   }
 
   for (it = config()->video_configs().begin();
        it != config()->video_configs().end(); ++it) {
     root->AddElement(FormatChannelConfig(*it, kVideoTag));
   }
 
   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.
   XmlElement* resolution_tag = new XmlElement(
       QName(kChromotingXmlNamespace, kDeprecatedResolutionTag));
   resolution_tag->AddAttr(QName(kDefaultNs, kDeprecatedWidthAttr), "640");
   resolution_tag->AddAttr(QName(kDefaultNs, kDeprecatedHeightAttr), "480");
   root->AddElement(resolution_tag);
 
   if (authenticator_message_.get()) {
@@ skipping 19 matching lines @@
   while (child) {
     ChannelConfig channel_config;
     if (ParseChannelConfig(child, codec_required, &channel_config)) {
       configs->push_back(channel_config);
     }
     child = child->NextNamed(tag);
   }
   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;
 }
 
 // static
 scoped_ptr<ContentDescription> ContentDescription::ParseXml(
     const XmlElement* element) {
   if (element->Name() != QName(kChromotingXmlNamespace, kDescriptionTag)) {
     LOG(ERROR) << "Invalid description: " << element->Str();
     return scoped_ptr<ContentDescription>();
@@ skipping 15 matching lines @@
   const XmlElement* child = Authenticator::FindAuthenticatorMessage(element);
   if (child)
     authenticator_message.reset(new XmlElement(*child));
 
   return scoped_ptr<ContentDescription>(
       new ContentDescription(config.Pass(), authenticator_message.Pass()));
 }
 
 }  // namespace protocol
 }  // namespace remoting