[Chromoting] Plugin message in JingleMessage

remoting/protocol/jingle_messages_unittest.cc

Index: remoting/protocol/jingle_messages_unittest.cc
diff --git a/remoting/protocol/jingle_messages_unittest.cc b/remoting/protocol/jingle_messages_unittest.cc
index a72db8636103c769892e8cdfd34a69d434511613..beedc75bc67e1193f64a6a7cdcc6ab4f180898b7 100644
--- a/remoting/protocol/jingle_messages_unittest.cc
+++ b/remoting/protocol/jingle_messages_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/strings/string_util.h"
 #include "remoting/protocol/content_description.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -26,6 +27,39 @@ namespace {
 const char kXmlNsNs[] = "http://www.w3.org/2000/xmlns/";
 const char kXmlNs[] = "xmlns";
 
+bool VerifyXml(const XmlElement* exp,
+               const XmlElement* val,
+               std::string* error);
+
+bool VerifyXmlChildrenUnordered(const XmlElement* exp,
+                                const XmlElement* val,
+                                bool exp_is_expected,
+                                std::string* error) {
+  const XmlElement* exp_child = exp->FirstElement();
+  while (exp_child) {
+    const XmlElement* val_child = val->FirstElement();
+    while (val_child) {
+      if (VerifyXml(exp_child, val_child, error)) {
+        break;
+      }
+      val_child = val_child->NextElement();
+    }
+    if (!val_child) {
+      if (error->empty()) {
+        if (exp_is_expected) {
+          *error = "<" + exp_child->Name().Merged() + "> is expected, "
+                   "but not found";
+        } else {
+          *error = "Unexpected <" + exp_child->Name().Merged() + "> found";
+        }
+      }
+      return false;
+    }
+    exp_child = exp_child->NextElement();
+  }
+  return true;
+}
+
 // Compares two XML blobs and returns true if they are
 // equivalent. Otherwise |error| is set to error message that
 // specifies the first test.
@@ -72,21 +106,8 @@ bool VerifyXml(const XmlElement* exp,
     }
   }
 
-  const XmlElement* exp_child = exp->FirstElement();
-  const XmlElement* val_child = val->FirstElement();
-  while (exp_child && val_child) {
-    if (!VerifyXml(exp_child, val_child, error))
-      return false;
-    exp_child = exp_child->NextElement();
-    val_child = val_child->NextElement();
-  }
-  if (exp_child) {
-    *error = "<" + exp_child->Name().Merged() + "> is expected, but not found";
-    return false;
-  }
-
-  if (val_child) {
-    *error = "Unexpected <" + val_child->Name().Merged() + "> found";
+  if (!VerifyXmlChildrenUnordered(exp, val, true, error) ||
+      !VerifyXmlChildrenUnordered(val, exp, false, error)) {
     return false;
   }
 
@@ -119,6 +140,7 @@ void ParseFormatAndCompare(const char* message_text, JingleMessage* parsed) {
 
   std::unique_ptr<XmlElement> formatted_message(parsed->ToXml());
   ASSERT_TRUE(formatted_message.get());
+
   EXPECT_TRUE(VerifyXml(source_message.get(), formatted_message.get(), &error))
       << error;
 }
@@ -600,5 +622,82 @@ TEST(JingleMessageTest, RemotingErrorCode) {
   }
 }
 
+namespace {
+
+void TestPluginMessage(bool insert_before_regular_message) {

[Sergey Ulanov, 2016/12/12 23:00:03]

I doubt it's useful to test for the case when the plugins are inserted int a
wrong place - it's unlikely to be broken, but increases complexity.

[Hzj_jie, 2016/12/13 00:20:51]

I have not found there is a strong sequence requirement in jingle schema, so
both before or after regular message are valid. Though we always put the tag
before other tags in c++ implementation.

[Sergey Ulanov, 2016/12/14 01:31:50]

Yes, both formats are valid. My point was that it's not necessary to test both
versions. Other tests don't verify ordering. With XML there are many other
things we could test for, but we don't (e.g. <foo xmlns="bar"/> and <n:foo
xmlns:n="bar"> are the same, but we don't test that they are handled the same
way). In general I don't think we should aim to cover every possible test case.
That increases tests complexity without much benefit. It's more important to
have tests that are easy to maintain. 

If you believe that we really need to test this scenario then it would be
cleaner to move that in a separate test, which would test only one message type.
That would make this test simpler.

[Hzj_jie, 2016/12/14 20:03:48]

Updated.

+  const char* kMessageWithPluginTag =
+      "<cli:iq from='user@gmail.com/chromoting016DBB07' "
+      "to='user@gmail.com/chromiumsy5C6A652D' type='set' "
+      "xmlns:cli='jabber:client'><jingle action='$1' "
+      "sid='2227053353' xmlns='urn:xmpp:jingle:1'>$2"
+      "<gr:plugin xmlns:gr='google:remoting'>"
+      "<gr:sometag>some-message</gr:sometag>"
+      "</gr:plugin>$3</jingle></cli:iq>";
+  for (int i = JingleMessage::SESSION_INITIATE;
+       i <= JingleMessage::TRANSPORT_INFO; i++) {
+    JingleMessage::ActionType action_type =
+        static_cast<JingleMessage::ActionType>(i);
+    std::vector<std::string> subst = {

[Sergey Ulanov, 2016/12/12 23:00:03]

subst is not a good name

[Hzj_jie, 2016/12/13 00:20:51]

Done.

+        JingleMessage::GetActionName(action_type)
+    };
+    if (insert_before_regular_message) {
+      subst.emplace_back();
+    }
+    if (action_type == JingleMessage::SESSION_INFO) {
+      subst.push_back("<test-info>test-message</test-info>");
+    } else if (action_type == JingleMessage::SESSION_TERMINATE) {
+      subst.emplace_back();
+    } else if (action_type == JingleMessage::TRANSPORT_INFO) {

[Sergey Ulanov, 2016/12/12 23:00:03]

I don't think we want to allow plugins for transport-info and session-terminate
messages messages. Only session-info, session-initiate and session-accept.

[Hzj_jie, 2016/12/13 00:20:51]

I thought this before, but it would make JingleMessage more complex without a
significant benefit. I.e. we need to check action in both ParseXml() and
ToXml(). Each plugin can decide whether it should put a certain piece of data
into plugin tag according to the action type in their logic.

[Sergey Ulanov, 2016/12/14 01:31:50]

It would make JingleMessage very slightly more complex, but would make it easier
to handle these messages in JingleSession.

[Hzj_jie, 2016/12/14 20:03:48]

I think it can make both JingleMessage and JingleSession simpler. i.e.
JingleMessage can always read the field, while JingleSession can blindly send
this field to plugins without caring about current action type.

[Sergey Ulanov, 2016/12/15 19:18:25]

I think complexity comes not only from amount of code, but also difficulty to
reason about the behavior. E.g. if we allow <attachments> for session-terminate
message how should they be handled? Can the receiver just ignore them? Also it
makes it harder to maintain this code in the future. E.g. if we need to make
attachments handled asynchronously then the way they would need to be handled is
likely to depend on type of message. It's also not likely that we will ever need
to use this feature for any messages other than session-info, session-accept and
session-initiate.
OTOH if will not be hard to revere this decision in the future if it causes any
problems, so I don't mind keeping it this way for now if you feel strongly about
it.

[Hzj_jie, 2016/12/15 22:41:19]

IMO how to handle the message is not part of the functionality of JingleMessage
class itself. So except for some very basic validations to fulfill the
requirement of Jingle message specification, adding more action based logic in
JingleMessage seems not a good idea.

Anyway, since you do not have a very strong opinion, I would prefer to keep
current solution.

+      subst.push_back(
+          "<content name='chromoting' creator='initiator'>"
+            "<transport xmlns='google:remoting:webrtc'>"
+              "<credentials channel='event' ufrag='tPUyEAmQrEw3y7hi' "
+                           "password='2iRdhLfawKZC5ydJ'/>"
+              "<credentials channel='video' ufrag='EPK3CXo5sTLJSez0' "
+                           "password='eM0VUfUkZ+1Pyi0M'/>"
+              "<candidate name='event' foundation='725747215' "
+                         "address='172.23.164.186' port='59089' type='local' "
+                         "protocol='udp' priority='2122194688' generation='0'/>"
+              "<candidate name='video' foundation='3623806809' "
+                         "address='172.23.164.186' port='57040' type='local' "
+                         "protocol='udp' priority='2122194688' generation='0'/>"
+            "</transport>"
+          "</content>");
+    } else {
+      subst.push_back("<content name='chromoting' creator='initiator'>"
+                        "<description xmlns='google:remoting'>"
+                          "<authentication><auth-token>"
+                            "j7whCMii0Z0AAPwj7whCM/j7whCMii0Z0AAPw="
+                          "</auth-token></authentication>"
+                        "</description>"
+                        "<transport xmlns='google:remoting:webrtc' />"
+                      "</content>");
+    }
+    if (!insert_before_regular_message) {
+      subst.emplace_back();
+    }
+    std::string message_str = base::ReplaceStringPlaceholders(
+        kMessageWithPluginTag, subst, nullptr);
+
+    JingleMessage message;
+    ParseFormatAndCompare(message_str.c_str(), &message);
+
+    EXPECT_TRUE(message.plugin_message);
+    XmlElement expected(QName("google:remoting", "plugin"));
+    expected.AddElement(new XmlElement(QName("google:remoting", "sometag")));
+    expected.FirstElement()->SetBodyText("some-message");
+    std::string error;
+    EXPECT_TRUE(VerifyXml(&expected, message.plugin_message.get(), &error))
+        << error;
+  }
+}
+
+}  // namespace
+
+TEST(JingleMessageTest, PluginMessage) {
+  TestPluginMessage(false);
+  TestPluginMessage(true);
+}
+
 }  // namespace protocol
 }  // namespace remoting