Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567953002:
[Chromoting] Plugin message in JingleMessage (Closed)
Description[Chromoting] Plugin message in JingleMessage
This change adds plugin message in JingleMessage, which is an action indepenent
xml node, placed before or after other messages.
The plugin message will be used to send and receive host attributes and
experiment configuration.
This is part of host experiment framework.
BUG=650926
Committed: https://crrev.com/5443b73b918944af49422accd998d82c4ff8027f
Cr-Commit-Position: refs/heads/master@{#438945}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Resolve review comments #
Total comments: 2
Patch Set 3 : Resolve review comments #
Total comments: 4
Patch Set 4 : Add {} for single line if-statements #Patch Set 5 : Add OWNERS file to work around IPC security check #
Messages
Total messages: 37 (27 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages.h:104: // Content of plugin message. The node is read or written by all plugins, and There can be more than one plugin, so I think this should be a vector. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:627: void TestPluginMessage(bool insert_before_regular_message) { 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. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:640: std::vector<std::string> subst = { subst is not a good name https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { 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.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages.h:104: // Content of plugin message. The node is read or written by all plugins, and On 2016/12/12 23:00:03, Sergey Ulanov wrote: > There can be more than one plugin, so I think this should be a vector. Then we will need extra logic to decide which piece of data should be sent to a certain plugin in JingleSession, which indeed can be placed in plugins themselves. I.e. A plugin get entire plugin data, and decide which subnode it should read from or write to. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:627: void TestPluginMessage(bool insert_before_regular_message) { On 2016/12/12 23:00:03, Sergey Ulanov wrote: > 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. 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. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:640: std::vector<std::string> subst = { On 2016/12/12 23:00:03, Sergey Ulanov wrote: > subst is not a good name Done. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { On 2016/12/12 23:00:03, Sergey Ulanov wrote: > 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. 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.
https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages.h:104: // Content of plugin message. The node is read or written by all plugins, and On 2016/12/13 00:20:51, Hzj_jie wrote: > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > There can be more than one plugin, so I think this should be a vector. > > Then we will need extra logic to decide which piece of data should be sent to a > certain plugin in JingleSession, I don't think it would increase complexity much. The dispatching code could look as follows: for (auto& msg : plugin_messages) { for (auto& plugin : plugins_) { if (plugin->HandleMessage(msg)) break; } } but feel free to keep the current approach (see the comment below). > which indeed can be placed in plugins > themselves. I.e. A plugin get entire plugin data, and decide which subnode it > should read from or write to. I see. Then I think <plugin> is not the best name, as it stores information for multiple plugins. <plugins> would be better, but still not very clear. Maybe call it <attachements>? https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:66: bool VerifyXml(const XmlElement* exp, There are some messages for which order matters. Particularly it matters for session description. So I don't think we want to make this function ignore ordering for all tests. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:627: void TestPluginMessage(bool insert_before_regular_message) { On 2016/12/13 00:20:51, Hzj_jie wrote: > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { On 2016/12/13 00:20:51, Hzj_jie wrote: > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > 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. > > 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. It would make JingleMessage very slightly more complex, but would make it easier to handle these messages in JingleSession. https://codereview.chromium.org/2567953002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2567953002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:350: plugin_message.reset(nullptr); don't need nullptr.
The CQ bit was checked by zijiehe@chromium.org to run a 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages.h:104: // Content of plugin message. The node is read or written by all plugins, and On 2016/12/14 01:31:50, Sergey Ulanov wrote: > On 2016/12/13 00:20:51, Hzj_jie wrote: > > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > > There can be more than one plugin, so I think this should be a vector. > > > > Then we will need extra logic to decide which piece of data should be sent to > a > > certain plugin in JingleSession, > > I don't think it would increase complexity much. The dispatching code could look > as follows: > for (auto& msg : plugin_messages) { > for (auto& plugin : plugins_) { > if (plugin->HandleMessage(msg)) > break; > } > } > > but feel free to keep the current approach (see the comment below). > > > which indeed can be placed in plugins > > themselves. I.e. A plugin get entire plugin data, and decide which subnode it > > should read from or write to. > > I see. Then I think <plugin> is not the best name, as it stores information for > multiple plugins. <plugins> would be better, but still not very clear. Maybe > call it <attachements>? Yes, "attachments" is definitely good. Updated. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:66: bool VerifyXml(const XmlElement* exp, On 2016/12/14 01:31:50, Sergey Ulanov wrote: > There are some messages for which order matters. Particularly it matters for > session description. So I don't think we want to make this function ignore > ordering for all tests. Got you. Then I would prefer to remove the unordered test cases of plugin messages. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:627: void TestPluginMessage(bool insert_before_regular_message) { On 2016/12/14 01:31:50, Sergey Ulanov wrote: > On 2016/12/13 00:20:51, Hzj_jie wrote: > > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > > 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. > > > > 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. > > 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. Updated. https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { On 2016/12/14 01:31:50, Sergey Ulanov wrote: > On 2016/12/13 00:20:51, Hzj_jie wrote: > > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > > 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. > > > > 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. > > It would make JingleMessage very slightly more complex, but would make it easier > to handle these messages in JingleSession. 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. https://codereview.chromium.org/2567953002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2567953002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:350: plugin_message.reset(nullptr); On 2016/12/14 01:31:50, Sergey Ulanov wrote: > don't need nullptr. Done, also several other resets.
lgtm https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { On 2016/12/14 20:03:48, Hzj_jie wrote: > On 2016/12/14 01:31:50, Sergey Ulanov wrote: > > On 2016/12/13 00:20:51, Hzj_jie wrote: > > > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > > > 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. > > > > > > 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. > > > > It would make JingleMessage very slightly more complex, but would make it > easier > > to handle these messages in JingleSession. > > 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. 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. https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:359: if (child == attachments_tag) { nit: remove {} for consistency with other single-line if statements. https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:459: if (attachments) { nit: remove {} for consistency with other single-line if statements.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/2567953002/diff/1/remoting/protocol/jingle_me... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2567953002/diff/1/remoting/protocol/jingle_me... remoting/protocol/jingle_messages_unittest.cc:650: } else if (action_type == JingleMessage::TRANSPORT_INFO) { On 2016/12/15 19:18:25, Sergey Ulanov wrote: > On 2016/12/14 20:03:48, Hzj_jie wrote: > > On 2016/12/14 01:31:50, Sergey Ulanov wrote: > > > On 2016/12/13 00:20:51, Hzj_jie wrote: > > > > On 2016/12/12 23:00:03, Sergey Ulanov wrote: > > > > > 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. > > > > > > > > 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. > > > > > > It would make JingleMessage very slightly more complex, but would make it > > easier > > > to handle these messages in JingleSession. > > > > 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. > > 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. 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. https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:359: if (child == attachments_tag) { On 2016/12/15 19:18:25, Sergey Ulanov wrote: > nit: remove {} for consistency with other single-line if statements. Though in coding style, ignoring {} is allowed for a single line statement, I do not really see lots of examples. I would prefer to add {} back to other single-line if statements. https://codereview.chromium.org/2567953002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:459: if (attachments) { On 2016/12/15 19:18:25, Sergey Ulanov wrote: > nit: remove {} for consistency with other single-line if statements. Ditto.
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/2567953002/#ps80001 (title: "Add OWNERS file to work around IPC security check")
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": 80001, "attempt_start_ts": 1481841731975790,
"parent_rev": "a40638940b8d009ded8ec901dbc2ba311055bf26", "commit_rev":
"bf088112e7e895bd95f4e975df7a6e685b7a23ea"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Plugin message in JingleMessage This change adds plugin message in JingleMessage, which is an action indepenent xml node, placed before or after other messages. The plugin message will be used to send and receive host attributes and experiment configuration. This is part of host experiment framework. BUG=650926 ========== to ========== [Chromoting] Plugin message in JingleMessage This change adds plugin message in JingleMessage, which is an action indepenent xml node, placed before or after other messages. The plugin message will be used to send and receive host attributes and experiment configuration. This is part of host experiment framework. BUG=650926 Review-Url: https://codereview.chromium.org/2567953002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Plugin message in JingleMessage This change adds plugin message in JingleMessage, which is an action indepenent xml node, placed before or after other messages. The plugin message will be used to send and receive host attributes and experiment configuration. This is part of host experiment framework. BUG=650926 Review-Url: https://codereview.chromium.org/2567953002 ========== to ========== [Chromoting] Plugin message in JingleMessage This change adds plugin message in JingleMessage, which is an action indepenent xml node, placed before or after other messages. The plugin message will be used to send and receive host attributes and experiment configuration. This is part of host experiment framework. BUG=650926 Committed: https://crrev.com/5443b73b918944af49422accd998d82c4ff8027f Cr-Commit-Position: refs/heads/master@{#438945} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5443b73b918944af49422accd998d82c4ff8027f Cr-Commit-Position: refs/heads/master@{#438945} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
