Include version information in heartbeats.

remoting/host/heartbeat_sender.cc

Index: remoting/host/heartbeat_sender.cc
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index 0a5a1815342e89d142b47f2becc0bf59af54d14a..95e954931e1244f27b3d24cf7017082637bfc76f 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -11,6 +11,7 @@
 #include "base/message_loop_proxy.h"
 #include "base/rand_util.h"
 #include "base/string_number_conversions.h"
+#include "base/strings/stringize_macros.h"
 #include "base/time.h"
 #include "remoting/base/constants.h"
 #include "remoting/host/server_log_entry.h"
@@ -28,6 +29,7 @@ namespace {
 
 const char kHeartbeatQueryTag[] = "heartbeat";
 const char kHostIdAttr[] = "hostid";
+const char kHostVersionAttr[] = "host-version";
 const char kHeartbeatSignatureTag[] = "signature";
 const char kSequenceIdAttr[] = "sequence-id";
 
@@ -241,6 +243,8 @@ scoped_ptr<XmlElement> HeartbeatSender::CreateHeartbeatMessage() {
   query->AddAttr(QName(kChromotingXmlNamespace, kHostIdAttr), host_id_);
   query->AddAttr(QName(kChromotingXmlNamespace, kSequenceIdAttr),
                  base::IntToString(sequence_id_));
+  query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr),

[Sergey Ulanov, 2013/02/11 20:29:11]

nit: Does this need to be an attribute? It may be better add it as a separate
element, so the heartbeat looks as follows:
 <heartbeat hostid=".." sequence-id="123">
   <signature>..Signature..</signature>
   <host-version>..version..</host-version>
 </heartbeat>

[Jamie, 2013/02/11 20:41:25]

Either works for me. I'd be interested to know what the pros and cons are of
your proposed change though.

[Sergey Ulanov, 2013/02/11 21:40:49]

There are no hard rules about it, but in general elements should be preferred,
particularly because they are easier to extend. In this case putting
host-version in an element would be better because in the future we may want to
add new fields, e.g. we may need to include OS version.

Here is a good article on the topic of attributes vs elements in XML:
http://www.ibm.com/developerworks/xml/library/x-eleatt/index.html
E.g. it says
"- If the information in question could be itself marked up with elements, put
it in an element."

[Jamie, 2013/02/13 18:25:26]

Thanks for the summary. Done.

+                 STRINGIZE(VERSION));
   query->AddElement(CreateSignature().release());
   // Append log message (which isn't signed).
   scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza());

[Sergey Ulanov, 2013/02/11 20:29:11]

The log entry should already include information about the host version. Can we
just reuse it instead of duplicating it in the same message?

[Jamie, 2013/02/11 20:41:25]

Renato suggested that in the bug discussion, but described it as a hack. You
guys know the server side a lot better than I do, so I'll defer to your
judgement. One advantage of that approach is that this CL isn't needed and we
have the information we need for older hosts.

Renato, do you agree with Sergey's preference to reuse the information from the
log stanza?

[Sergey Ulanov, 2013/02/11 21:40:49]

Yes. I agree. Given that we want to store this information in the directory,
reusing logs would be a hack.

[Jamie, 2013/02/13 18:25:26]

So it sounds like you're in favour of the approach taken in this CL, correct?

[Sergey Ulanov, 2013/02/13 18:53:15]

Yes, I'm in favor of this approach. Sorry for not being clear.