|
|
Description[Chromoting] Add SessionPlugin in JingleSession
This change adds SessionPlugin interface, and an Session::AddPlugin() function
to attach SessionPlugin instances into Session and its derived classes.
In JingleSession, the plugins will be executed after receiving a message or
before sending a message.
So a SessionPlugin implementation can read fields from and write to a message.
Refer to change https://codereview.chromium.org/2586133002/, it shows how
HostExperimentSessionPlugin works as a SessionPlugin.
This is part of host experiment framework.
BUG=650926
Committed: https://crrev.com/cf233ee72108ce3e3d35ecc7ba83a2e7d3726704
Cr-Commit-Position: refs/heads/master@{#441003}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 32
Patch Set 3 : Resolve review comments #
Total comments: 12
Patch Set 4 : Resolve review comments #
Total comments: 2
Patch Set 5 : Resolve review comments #
Total comments: 12
Patch Set 6 : Resolve review comments #
Messages
Total messages: 64 (50 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
Mostly looks good. I'll let Sergey give you the final sign-off, since he's more knowledgeable both in this area of the code and in C++ current best practises than I am. https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:26: // Attachs messages in |attachments|. This function will be called after other s/Attachs/Attaches/ https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:27: // components have finished generating messages, and set the state and action. s/and set/and have set/ https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:30: virtual void OnSending(Session::State state, I think On[Incoming|Outgoing]Message would be better names for these, as you've used elsewhere. https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:32: std::unique_ptr<buzz::XmlElement>* attachments) = 0; Passing a pointer to a unique_ptr is a bit strange; why not a raw pointer like you used in ExecutePluginsOnOutgoingMessage? Or, if you believe that this is only useful for adding attributes to the "attachments" field, maybe a better design would be to return the new attributes, which would also mean you could get rid of the NewAttachments method, and avoid duplicating the (admittedly minor) code duplication where every implementation has to create that field if it's null.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/fake... File remoting/protocol/fake_session.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/fake... remoting/protocol/fake_session.cc:107: void FakeSession::Attach(std::unique_ptr<SessionPlugin> plugin) {} NOTIMPLEMENTED() https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_messages.cc:516: if (!attachment) { I don't think we want to allow attachment to be null here. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_messages.h:96: void AttachAttachment(std::unique_ptr<buzz::XmlElement> attachment); AddAttachment() https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:495: DCHECK(message); Don't really need this. unique_ptr<>::operator* called in the next line will assert that the pointer is not null. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:762: void JingleSession::ExecutePluginsOnIncomingMessage( I don't think you really need this function. It's executed in only one place and it is 3 lines long. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:596: TEST_F(JingleSessionTest, TestSessionPlugin) { This test doesn't verify that the messages are being passed between the plugins, and I think it's the most important part. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:602: // JingleSessionManager. 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. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:603: ASSERT_EQ(client_plugin_->on_outgoing_count(), 2); 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. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session.h:91: // empty std::unique_ptr. nit: Don't really need to mention that plugin=null is not allowed. It's the default for any pointer argument. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session.h:92: virtual void Attach(std::unique_ptr<SessionPlugin> plugin) = 0; call it AddPlugin()? https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:21: class SessionPlugin { 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. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, 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.
https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:26: // Attachs messages in |attachments|. This function will be called after other On 2016/12/20 23:29:21, Jamie wrote: > s/Attachs/Attaches/ Done. https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:27: // components have finished generating messages, and set the state and action. On 2016/12/20 23:29:21, Jamie wrote: > s/and set/and have set/ Done. https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:30: virtual void OnSending(Session::State state, On 2016/12/20 23:29:21, Jamie wrote: > I think On[Incoming|Outgoing]Message would be better names for these, as you've > used elsewhere. Done. https://codereview.chromium.org/2586403003/diff/40001/remoting/protocol/sessi... remoting/protocol/session_plugin.h:32: std::unique_ptr<buzz::XmlElement>* attachments) = 0; On 2016/12/20 23:29:21, Jamie wrote: > Passing a pointer to a unique_ptr is a bit strange; why not a raw pointer like > you used in ExecutePluginsOnOutgoingMessage? Or, if you believe that this is > only useful for adding attributes to the "attachments" field, maybe a better > design would be to return the new attributes, which would also mean you could > get rid of the NewAttachments method, and avoid duplicating the (admittedly > minor) code duplication where every implementation has to create that field if > it's null. This function is a little bit different than ExecutePluginsOnOutgoingMessage(), which accepts JingleMessage as parameter. This class is a SessionPlugin, it should not depend on a JingleMessage. And in JingleMessage, if the attachments field is an empty unique_ptr, the entire node will be ignored in ToXml() function. So here, the scenario becomes a little bit complex, we should not create an attachments node before one of the plugins decides to insert something into the attachments node. Unfortunately XmlElement has no IsEmpty() or similar function to indicate whether this node contains something really useful. Though FirstChild() / FirstAttr() can be use, it looks a little bit weird to me to check whether an XmlElement is empty by checking the first child or attribute. Returning an XmlElement is a workable solution. I have updated the code. I used to think by returning an XmlElement, one SessionPlugin cannot depend on the data coming from another SessionPlugin. But we can still pass the data between two SessionPlugins through other mechanism.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::Attach() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 ========== to ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::AddPlugin() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/fake... File remoting/protocol/fake_session.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/fake... remoting/protocol/fake_session.cc:107: void FakeSession::Attach(std::unique_ptr<SessionPlugin> plugin) {} On 2016/12/21 01:38:34, Sergey Ulanov wrote: > NOTIMPLEMENTED() Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_messages.cc:516: if (!attachment) { On 2016/12/21 01:38:34, Sergey Ulanov wrote: > I don't think we want to allow attachment to be null here. Done. The null check needs to be placed either here or in JingleSession. IMO, here is better. Logically speaking it's a little bit weird, what's the point to attach an empty attachment. But it can save an std::move in JingleSession. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_messages.h:96: void AttachAttachment(std::unique_ptr<buzz::XmlElement> attachment); On 2016/12/21 01:38:34, Sergey Ulanov wrote: > AddAttachment() Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:495: DCHECK(message); On 2016/12/21 01:38:34, Sergey Ulanov wrote: > Don't really need this. unique_ptr<>::operator* called in the next line will > assert that the pointer is not null. Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:762: void JingleSession::ExecutePluginsOnIncomingMessage( On 2016/12/21 01:38:34, Sergey Ulanov wrote: > I don't think you really need this function. It's executed in only one place and > it is 3 lines long. Not really, this function is called by both AcceptIncomingConnection() and OnIncomingMessage(). https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:596: TEST_F(JingleSessionTest, TestSessionPlugin) { On 2016/12/21 01:38:34, Sergey Ulanov wrote: > This test doesn't verify that the messages are being passed between the plugins, > and I think it's the most important part. Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:602: // JingleSessionManager. On 2016/12/21 01:38:34, Sergey Ulanov wrote: > 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. Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:603: ASSERT_EQ(client_plugin_->on_outgoing_count(), 2); On 2016/12/21 01:38:34, Sergey Ulanov wrote: > 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. 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(). https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session.h:91: // empty std::unique_ptr. On 2016/12/21 01:38:34, Sergey Ulanov wrote: > nit: Don't really need to mention that plugin=null is not allowed. It's the > default for any pointer argument. Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session.h:92: virtual void Attach(std::unique_ptr<SessionPlugin> plugin) = 0; On 2016/12/21 01:38:34, Sergey Ulanov wrote: > call it AddPlugin()? Done. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:21: class SessionPlugin { On 2016/12/21 01:38:34, Sergey Ulanov wrote: > 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. 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. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/21 01:38:34, Sergey Ulanov wrote: > 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. 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?
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:603: ASSERT_EQ(client_plugin_->on_outgoing_count(), 2); On 2016/12/22 00:27:10, Hzj_jie wrote: > On 2016/12/21 01:38:34, Sergey Ulanov wrote: > > 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. > > 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(). 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. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/22 00:27:11, Hzj_jie wrote: > On 2016/12/21 01:38:34, Sergey Ulanov wrote: > > 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. > > 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.) 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. > > 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? 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. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:762: plugin->OnIncoming(state_, message.action, message.attachments.get()); I think it would be cleaner to call this function only when message.attachments != nullptr, i.e. when there are actual attachments to be handled. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session.h:91: virtual void AddPlugin(std::unique_ptr<SessionPlugin> plugin) = 0; Looking at HostExperimentSessionPlugin I don't think we want to pass ownership of the plugin here. The caller would have to keep reference to the plugin somewhere (to get results), which is harder if it gives up ownership. Instead it's probably better to pass raw pointer here. This approach is also more consistent with SetTransport(). https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session_plugin.cc (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.cc:13: SessionPlugin::~SessionPlugin() = default; constructor/destructor for interfaces can be inlined in the header. Then you wouldn't need this file. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:20: // impact a session generation. Suggest rewording: // Interface for Session plugins. Plugins allow to send and receive // optional information that is not essential for session handshake. // Messages generated by the plugins on one end of a connection are // attached to the session handshake messages and passed to the plugins // on the other end. Plugins are optional, i.e. Session doesn't need // any plugins to connect successfully. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:31: virtual std::unique_ptr<buzz::XmlElement> OnOutgoing( Maybe call it GetNextAttachment() or GetNextMessage()? https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:38: virtual void OnIncoming(Session::State state, OnIncomingMessage
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:603: ASSERT_EQ(client_plugin_->on_outgoing_count(), 2); On 2016/12/23 00:14:51, Sergey Ulanov wrote: > On 2016/12/22 00:27:10, Hzj_jie wrote: > > On 2016/12/21 01:38:34, Sergey Ulanov wrote: > > > 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. > > > > 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(). > > 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. > The test has been updated in the last version already. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/23 00:14:51, Sergey Ulanov wrote: > On 2016/12/22 00:27:11, Hzj_jie wrote: > > On 2016/12/21 01:38:34, Sergey Ulanov wrote: > > > 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. > > > > 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.) > > 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 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. > > > > 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? > > 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. I will remove it. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:762: plugin->OnIncoming(state_, message.action, message.attachments.get()); On 2016/12/23 00:14:51, Sergey Ulanov wrote: > I think it would be cleaner to call this function only when message.attachments > != nullptr, i.e. when there are actual attachments to be handled. Done. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session.h:91: virtual void AddPlugin(std::unique_ptr<SessionPlugin> plugin) = 0; On 2016/12/23 00:14:52, Sergey Ulanov wrote: > Looking at HostExperimentSessionPlugin I don't think we want to pass ownership > of the plugin here. The caller would have to keep reference to the plugin > somewhere (to get results), which is harder if it gives up ownership. Instead > it's probably better to pass raw pointer here. This approach is also more > consistent with SetTransport(). Yes, updated. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session_plugin.cc (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.cc:13: SessionPlugin::~SessionPlugin() = default; On 2016/12/23 00:14:52, Sergey Ulanov wrote: > constructor/destructor for interfaces can be inlined in the header. Then you > wouldn't need this file. Done. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:20: // impact a session generation. On 2016/12/23 00:14:52, Sergey Ulanov wrote: > Suggest rewording: > // Interface for Session plugins. Plugins allow to send and receive > // optional information that is not essential for session handshake. > // Messages generated by the plugins on one end of a connection are > // attached to the session handshake messages and passed to the plugins > // on the other end. Plugins are optional, i.e. Session doesn't need > // any plugins to connect successfully. Done. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:31: virtual std::unique_ptr<buzz::XmlElement> OnOutgoing( On 2016/12/23 00:14:52, Sergey Ulanov wrote: > Maybe call it GetNextAttachment() or GetNextMessage()? Done. https://codereview.chromium.org/2586403003/diff/140001/remoting/protocol/sess... remoting/protocol/session_plugin.h:38: virtual void OnIncoming(Session::State state, On 2016/12/23 00:14:52, Sergey Ulanov wrote: > OnIncomingMessage Done.
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/24 00:09:54, Hzj_jie wrote: > On 2016/12/23 00:14:51, Sergey Ulanov wrote: > > 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 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. 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. https://codereview.chromium.org/2586403003/diff/160001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/160001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:399: ExecutePluginsOnOutgoingMessage(message.get()); When the host accepts session-initiate message from a client JID it doesn't recognize it sends session-terminate without session-accept. Attaching plugin information to this session-terminate message may lead to privacy issues (e.g. leaking windows version to someone who doesn't own the host). It's a minor issue right now, but it's better to avoid future risks here. That's one of the reasons I think we shouldn't allow attachments in session-terminate. If you think we do need attachments in session-terminate then it should be allowed only after session is accepted.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/27 22:05:18, Sergey Ulanov wrote: > On 2016/12/24 00:09:54, Hzj_jie wrote: > > On 2016/12/23 00:14:51, Sergey Ulanov wrote: > > > 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 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. > > 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. 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. https://codereview.chromium.org/2586403003/diff/160001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/160001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:399: ExecutePluginsOnOutgoingMessage(message.get()); On 2016/12/27 22:05:18, Sergey Ulanov wrote: > When the host accepts session-initiate message from a client JID it doesn't > recognize it sends session-terminate without session-accept. Attaching plugin > information to this session-terminate message may lead to privacy issues (e.g. > leaking windows version to someone who doesn't own the host). It's a minor issue > right now, but it's better to avoid future risks here. That's one of the reasons > I think we shouldn't allow attachments in session-terminate. If you think we do > need attachments in session-terminate then it should be allowed only after > session is accepted. I see. Thank you for the explanation. I exclude plugins from session-terminate message, and added your comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few more style-nits, but LGTM otherwise. https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/27 23:21:08, Hzj_jie wrote: > On 2016/12/27 22:05:18, Sergey Ulanov wrote: > > On 2016/12/24 00:09:54, Hzj_jie wrote: > > > On 2016/12/23 00:14:51, Sergey Ulanov wrote: > > > > 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 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. > > > > 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. > > 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. 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. > > 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. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:767: void JingleSession::ExecutePluginsOnIncomingMessage( Maybe call it ProcessIncomingAttachments() or ProcessIncomingPluginMessage()? https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:769: if (message.attachments) { if (!messeage.attachments) { return; } https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:776: void JingleSession::ExecutePluginsOnOutgoingMessage(JingleMessage* message) { Maybe AddPluginAttachments() or something like that? The current name doesn't make it clear that this function may change the |message|. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:787: std::unique_ptr<JingleMessage> message(new JingleMessage( Need to check here that the state_ is still CONNECTING, in case the session was closed. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:627: TEST_F(JingleSessionTest, SessionPluginShouldNotBeInvolvedInSESSION_TERMINATE) { s/SESSION_TERMINATE/SessionTerminate/ https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/sess... remoting/protocol/session.h:90: // Adds a SessionPlugin to handle attachments. Please also mention that to ensure that plugin attachments are processed correctly for session-initiate message this function must be called immediately after SessionManager::Connect() for outgoing connections or in the IncomingSessionCallback handler for incoming connections.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... File remoting/protocol/session_plugin.h (right): https://codereview.chromium.org/2586403003/diff/120001/remoting/protocol/sess... remoting/protocol/session_plugin.h:39: JingleMessage::ActionType action, On 2016/12/29 00:37:57, Sergey Ulanov wrote: > On 2016/12/27 23:21:08, Hzj_jie wrote: > > On 2016/12/27 22:05:18, Sergey Ulanov wrote: > > > On 2016/12/24 00:09:54, Hzj_jie wrote: > > > > On 2016/12/23 00:14:51, Sergey Ulanov wrote: > > > > > 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 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. > > > > > > 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. > > > > 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. > > 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. > > > > > 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. > Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:767: void JingleSession::ExecutePluginsOnIncomingMessage( On 2016/12/29 00:37:58, Sergey Ulanov wrote: > Maybe call it ProcessIncomingAttachments() or ProcessIncomingPluginMessage()? Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:769: if (message.attachments) { On 2016/12/29 00:37:58, Sergey Ulanov wrote: > if (!messeage.attachments) { > return; > } Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:776: void JingleSession::ExecutePluginsOnOutgoingMessage(JingleMessage* message) { On 2016/12/29 00:37:58, Sergey Ulanov wrote: > Maybe AddPluginAttachments() or something like that? The current name doesn't > make it clear that this function may change the |message|. Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session.cc:787: std::unique_ptr<JingleMessage> message(new JingleMessage( On 2016/12/29 00:37:58, Sergey Ulanov wrote: > Need to check here that the state_ is still CONNECTING, in case the session was > closed. Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/jing... remoting/protocol/jingle_session_unittest.cc:627: TEST_F(JingleSessionTest, SessionPluginShouldNotBeInvolvedInSESSION_TERMINATE) { On 2016/12/29 00:37:58, Sergey Ulanov wrote: > s/SESSION_TERMINATE/SessionTerminate/ Done. https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/sess... File remoting/protocol/session.h (right): https://codereview.chromium.org/2586403003/diff/180001/remoting/protocol/sess... remoting/protocol/session.h:90: // Adds a SessionPlugin to handle attachments. On 2016/12/29 00:37:58, Sergey Ulanov wrote: > Please also mention that to ensure that plugin attachments are processed > correctly for session-initiate message this function must be called immediately > after SessionManager::Connect() for outgoing connections or in the > IncomingSessionCallback handler for incoming connections. Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2586403003/#ps200001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483053100093460, "parent_rev": "79cc165e14939c01afa34adbb4c044713e03291c", "commit_rev": "953b557607d6e53998c94ea7f20be9f876c8be97"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::AddPlugin() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 ========== to ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::AddPlugin() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 Review-Url: https://codereview.chromium.org/2586403003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::AddPlugin() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 Review-Url: https://codereview.chromium.org/2586403003 ========== to ========== [Chromoting] Add SessionPlugin in JingleSession This change adds SessionPlugin interface, and an Session::AddPlugin() function to attach SessionPlugin instances into Session and its derived classes. In JingleSession, the plugins will be executed after receiving a message or before sending a message. So a SessionPlugin implementation can read fields from and write to a message. Refer to change https://codereview.chromium.org/2586133002/, it shows how HostExperimentSessionPlugin works as a SessionPlugin. This is part of host experiment framework. BUG=650926 Committed: https://crrev.com/cf233ee72108ce3e3d35ecc7ba83a2e7d3726704 Cr-Commit-Position: refs/heads/master@{#441003} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cf233ee72108ce3e3d35ecc7ba83a2e7d3726704 Cr-Commit-Position: refs/heads/master@{#441003} |