[Chromoting] Add SessionPlugin in JingleSession

remoting/protocol/jingle_session_unittest.cc

Index: remoting/protocol/jingle_session_unittest.cc
diff --git a/remoting/protocol/jingle_session_unittest.cc b/remoting/protocol/jingle_session_unittest.cc
index 64808e3ab88bda396505469bc05e8c5d281c74d2..da3c6759d3aaedc8fd273c569ce5fb61aa6eb932 100644
--- a/remoting/protocol/jingle_session_unittest.cc
+++ b/remoting/protocol/jingle_session_unittest.cc
@@ -25,6 +25,7 @@
 #include "remoting/protocol/fake_authenticator.h"
 #include "remoting/protocol/jingle_session_manager.h"
 #include "remoting/protocol/network_settings.h"
+#include "remoting/protocol/session_plugin.h"
 #include "remoting/protocol/transport.h"
 #include "remoting/protocol/transport_context.h"
 #include "remoting/signaling/fake_signal_strategy.h"
@@ -101,6 +102,34 @@ class FakeTransport : public Transport {
   base::Closure on_message_callback_;
 };
 
+class FakePlugin : public SessionPlugin {
+ public:
+   std::unique_ptr<buzz::XmlElement> OnOutgoing(
+       Session::State state,
+       JingleMessage::ActionType action) override {
+    on_outgoing_count_++;
+    return nullptr;
+  }
+
+  void OnIncoming(Session::State state,
+                  JingleMessage::ActionType action,
+                  const buzz::XmlElement* attachments) override {
+    on_incoming_count_++;
+  }
+
+  int on_outgoing_count() const {
+    return on_outgoing_count_;
+  }
+
+  int on_incoming_count() const {
+    return on_incoming_count_;
+  }
+
+ private:
+  int on_outgoing_count_ = 0;
+  int on_incoming_count_ = 0;
+};
+
 std::unique_ptr<buzz::XmlElement> CreateTransportInfo(const std::string& id) {
   std::unique_ptr<buzz::XmlElement> result(
       buzz::XmlElement::ForStr("<transport xmlns='google:remoting:ice'/>"));
@@ -124,6 +153,9 @@ class JingleSessionTest : public testing::Test {
     host_session_.reset(session);
     host_session_->SetEventHandler(&host_session_event_handler_);
     host_session_->SetTransport(&host_transport_);
+    std::unique_ptr<FakePlugin> plugin(new FakePlugin());
+    host_plugin_ = plugin.get();
+    host_session_->Attach(std::move(plugin));
   }
 
   void DeleteHostSession() { host_session_.reset(); }
@@ -237,6 +269,9 @@ class JingleSessionTest : public testing::Test {
         client_server_->Connect(host_jid_, std::move(authenticator));
     client_session_->SetEventHandler(&client_session_event_handler_);
     client_session_->SetTransport(&client_transport_);
+    std::unique_ptr<FakePlugin> plugin(new FakePlugin());
+    client_plugin_ = plugin.get();
+    client_session_->Attach(std::move(plugin));
     base::RunLoop().RunUntilIdle();
   }
 
@@ -277,6 +312,9 @@ class JingleSessionTest : public testing::Test {
   std::unique_ptr<Session> client_session_;
   MockSessionEventHandler client_session_event_handler_;
   FakeTransport client_transport_;
+
+  FakePlugin* host_plugin_;
+  FakePlugin* client_plugin_;
 };
 
 
@@ -555,5 +593,18 @@ TEST_F(JingleSessionTest, TransportInfoDuringAuthentication) {
   EXPECT_EQ("1", client_transport_.received_messages()[0]->Attr(buzz::QN_ID));
 }
 
+TEST_F(JingleSessionTest, TestSessionPlugin) {

[Sergey Ulanov, 2016/12/21 01:38:34]

This test doesn't verify that the messages are being passed between the plugins,
and I think it's the most important part.

[Hzj_jie, 2016/12/22 00:27:11]

Done.

+  CreateSessionManagers(3, FakeAuthenticator::ACCEPT);
+  ASSERT_NO_FATAL_FAILURE(
+      InitiateConnection(3, FakeAuthenticator::ACCEPT, false));
+  // It's expected the plugins won't be able to be attached to the Session
+  // before the first connect message has been sent, which is controlled by
+  // JingleSessionManager.

[Sergey Ulanov, 2016/12/21 01:38:34]

I think this limitation needs to be mentioned somewhere beside the test. Maybe
in the comments for Session::AddPlugin()?
FWIW it should be easy to fix that, e.g. by sending session-initiate message
asynchronously after SessionManager::Connect() returns.

[Hzj_jie, 2016/12/22 00:27:10]

Done.

+  ASSERT_EQ(client_plugin_->on_outgoing_count(), 2);

[Sergey Ulanov, 2016/12/21 01:38:34]

This essentially verifies how many messages are sent in each direction, but this
isn't related to plugins in any way. It would break if there are any changes
that affect number of messages and this looks wrong to me. The test should
verify that the plugins can send messages to each other and that the session can
still be connected. 

[Hzj_jie, 2016/12/22 00:27:10]

I have updated the test case to compare the xml elements from both side.
I think whether the session is possible to be connected can be covered by other
test cases, since plugins are always attached to Session in SetHostSession() and
ConnectClient().

[Sergey Ulanov, 2016/12/23 00:14:51]

Currently the test doesn't verify that JingleSession attaches messages and can
process them on the receiving end. That's the main problem here. It doesn't test
all plugin-related functionality.

[Hzj_jie, 2016/12/24 00:09:54]

The test has been updated in the last version already.

+  ASSERT_EQ(client_plugin_->on_incoming_count(), 3);
+  ASSERT_EQ(host_plugin_->on_outgoing_count(), 3);
+  ASSERT_EQ(host_plugin_->on_incoming_count(), 3);
+}
+
 }  // namespace protocol
 }  // namespace remoting