[Chromoting] Add SessionPlugin in JingleSession

remoting/protocol/session_plugin.h

Index: remoting/protocol/session_plugin.h
diff --git a/remoting/protocol/session_plugin.h b/remoting/protocol/session_plugin.h
new file mode 100644
index 0000000000000000000000000000000000000000..185210d19fb707df67bfba8dfcbbcbedace23ea6
--- /dev/null
+++ b/remoting/protocol/session_plugin.h
@@ -0,0 +1,46 @@
+// Copyright 2016 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.
+
+#ifndef REMOTING_PROTOCOL_SESSION_PLUGIN_H_
+#define REMOTING_PROTOCOL_SESSION_PLUGIN_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "remoting/protocol/jingle_messages.h"
+#include "remoting/protocol/session.h"
+#include "third_party/webrtc/libjingle/xmllite/xmlelement.h"
+
+namespace remoting {
+namespace protocol {
+
+// An interface to attach data on an outgoing message or handle an incoming of
+// message. The plugin data are designed to be optional, so this class won't
+// impact a session generation.
+class SessionPlugin {

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

This interface allow plugins to send outgoing messages only when the are other
messages being sent in the same direction. I think it would be simpler if
plugins were getting a callback that they can use to send messages to the peer
when they need to, similar to how it works in Transport, e.g.

class SessionPlugin {
  typedef
     base::Callback<void(std::unique_ptr<buzz::XmlElement> message)> 
     SendCallback;
  void Start(SendCallback send_callback);
  void OnIncomingMessage();
}

That would also make plugin implementations simpler as they won't need to track
the intermediate state when a message is received but the response isn't sent.

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

Currently we do not have such scenario, HostExperimentSessionPlugin sends
messages before receiving. And IMO, allowing SessionPlugin to send standalone
messages is not a right thing to do. So we can achieve your suggestion by
wrapping SessionPlugin with a DelaySendingSessionPlugin once we need it. A
prototype is like,
class DelaySendingSessinPlugin : public SessionPlugin {
 public:
  std::unique_ptr<XmlElement> OnSending(...) final {
    return std::move(pending_);
  }

 protected:
  void SendMessage(std::unique_ptr<buzz::XmlElement> message) {
    pending_ = std::move(message);
  }

 private:
  std::unique_ptr<XmlElement> pending_;
};

Since OnReceiving() is the only place the DelaySendingSessionPlugin is involved,
a DelaySendingSessionPlugin should only be able to send one xml element during
each round-trip.

And this wrapper can help us to reduce the complexity in JingleSession or other
Session implementation. i.e. Each Session implementation does not need to track
the internal state of each SessionPlugin.

+ public:
+  SessionPlugin();
+  virtual ~SessionPlugin();
+
+  // Returns an XmlElement if the SessionPlugin requires to attach some data
+  // into the outgoing message. This function will be called after other
+  // components have finished generating messages, and have set the state and
+  // action. So |state| is the current session state, |action| is the current
+  // ActionType in message.
+  virtual std::unique_ptr<buzz::XmlElement> OnOutgoing(
+      Session::State state,
+      JingleMessage::ActionType action) = 0;
+
+  // Handles messages in |attachments|. This function will be called before
+  // other components have handled the incoming message. So |state| is the last
+  // session state, |action| is the current ActionType in message.
+  virtual void OnIncoming(Session::State state,
+                          JingleMessage::ActionType action,

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

Do the implementation of this class really need to know |state| or |action|?
Particularly |action| looks out of place here. We should avoid any dependency on
Jingle-specific logic in Session interface: Session is not always a jingle
session, it may be implemented using a different protocol.

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

Session::State should be a must-have, it is needed by host experiment session
plugin to decide when the host-attributes should be sent, and whether the
host-configuration is still useful. (The field received after authenticated is
simply useless.)

For JingleMessage::ActionType, I cannot quite tell. It's used in
HostExperimentSessionPlugin (change 2586133002) as a secondary check. But IMO,
some of the ActionTypes do not really depend on JingleSession, such as
SESSION_ACCEPT, SESSION_INITIATE, SESSION_TERMINATE. They are common actions for
a connection-based communication.
I can surely remove ActionType, which I hope, does not really impact current
scenarios. But I think moving some action types out of JingleMessage may be
better. What do you think?

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

The current version of that plugin makes assumptions about messages that are
sent by the Session implementations in each state. It might not work correctly
if we try to replace JingleSession with something else. I think it's better to
avoid that dependency. E.g. FakeSession doesn't actually send any messages in
either direction, so the code would be rather awkward and fragile if you try to
add support for plugins there. It should be easy to avoid dependency on state in
plugins. In HostExperimentSessionPlugin in particular you could use the
following approach:
 1. Attach host attributes to the first message sent to the client, i.e. the
first time OnOutgoing() is called.
 2. Listen for configuration message in OnIncoming(), ignoring state. The case
when the response is received too late can be detected with help of the upper
layer that uses the plugin.
I still think it's better to remove it. |action| doesn't belong here because it
Jingle-specific concept. Session interface should not depend on Jingle.

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

I agree for HostExperimentSessionPlugin, this should be able to work correctly.
I still think Session::State is a valuable parameter for SessionPlugin.
Otherwise a SessionPlugin can only send the message at very beginning then
receive messages, or receive-then-send.
I will remove it.

[Sergey Ulanov, 2016/12/27 22:05:18]

Not sure what's the problem here. SessionPlugin implementations can be stateful,
so they can keep track of how many messages they have sent/received, so it's
possible to implement more complex patterns than send-receive or receive-send.
It doesn't need to know about session state to implement this correctly and I'm
arguing that it shouldn't have that dependency. If there is ever a plugin that
needs to be aware of session state it would be easy to pass the state from the
connection layer that created the plugin.
Also note that it's hard to make the state value that the plugin gets here
consistent. State often changes after the current message is sent, so it's hard
for the plugin to interpret it correctly.

[Hzj_jie, 2016/12/27 23:21:08]

My concern is the plugin may not only depend on its internal state, but also the
session state to decide the following actions. Using experiment framework as an
example, if the session state switched to FAILED / CLOSED for any reason, the
client does not need to talk with external experiment system to decide the
participation of current session anymore.

I buy in the way to pass session state machine from the SessionPlugin
constructor, which is also a reasonable approach.

I have removed the Session::State parameter.

[Sergey Ulanov, 2016/12/29 00:37:57]

In the current implementation OnIncoming() and OnOutgoing() will not be called
in FAILED/CLOSED state (because no outgoing messages are sent in that state and
Session is normally destroyed as soon as it gets to FAILED/CLOSED state), so I
don't think passing state here makes any difference in that case. The plugin
will normally be destroyed as soon as the client connection is closed. That way
it will know it can cancel any async work.

[Hzj_jie, 2016/12/29 03:07:56]

Done.

+                          const buzz::XmlElement* attachments) = 0;
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_SESSION_PLUGIN_H_