[Chromoting] Add platform data to heartbeats.

[Chromoting] Add platform data to heartbeats.

BUG=126491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138275

[simonmorris, 2012-05-18 00:32:23]

ptal

[Wez, 2012-05-18 01:27:14]

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender.cc
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:233: scoped_ptr<XmlElement> query(new
XmlElement(
Can you add a comment here to the effect of "Create heartbeat stanza"

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:239: scoped_ptr<XmlElement>
log(ServerLogEntry::MakeLogStanza());
And one here to the effect of "Create heartbeat log stanza", and to note that
the log stanza's not signed?

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:240: scoped_ptr<ServerLogEntry>
log_entry(ServerLogEntry::MakeHeartbeat());
ServerLogEntry::MakeHeartbeat() looks strange here; the

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry.h
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
remoting/host/server_log_entry.h:31: static scoped_ptr<buzz::XmlElement>
MakeLogStanza();
nit: "Log" is a little redundant in the name; how about MakeStanza(), or
NewStanza() to distinguish this method from the Make* methods below?

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
remoting/host/server_log_entry.h:38: static ServerLogEntry* MakeHeartbeat();
MakeHeartbeat unfortunately reads in HeartbeatSender as though this method
creates the heart-beat; consider ServerLogEntry::MakeForHeartbeat() or
ForHeartbeat()?

[simonmorris, 2012-05-18 17:06:57]

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender.cc
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:233: scoped_ptr<XmlElement> query(new
XmlElement(
On 2012/05/18 01:27:14, Wez wrote:
> Can you add a comment here to the effect of "Create heartbeat stanza"

Done.

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:239: scoped_ptr<XmlElement>
log(ServerLogEntry::MakeLogStanza());
On 2012/05/18 01:27:14, Wez wrote:
> And one here to the effect of "Create heartbeat log stanza", and to note that
> the log stanza's not signed?

Done.

http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
remoting/host/heartbeat_sender.cc:240: scoped_ptr<ServerLogEntry>
log_entry(ServerLogEntry::MakeHeartbeat());
On 2012/05/18 01:27:14, Wez wrote:
> ServerLogEntry::MakeHeartbeat() looks strange here; the 

Done.

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry.h
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
remoting/host/server_log_entry.h:31: static scoped_ptr<buzz::XmlElement>
MakeLogStanza();
On 2012/05/18 01:27:14, Wez wrote:
> nit: "Log" is a little redundant in the name; how about MakeStanza(), or
> NewStanza() to distinguish this method from the Make* methods below?

Done.

http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
remoting/host/server_log_entry.h:38: static ServerLogEntry* MakeHeartbeat();
On 2012/05/18 01:27:14, Wez wrote:
> MakeHeartbeat unfortunately reads in HeartbeatSender as though this method
> creates the heart-beat; consider ServerLogEntry::MakeForHeartbeat() or
> ForHeartbeat()?

Done.

[simonmorris, 2012-05-21 16:14:08]

On 2012/05/18 17:06:57, simonmorris wrote:
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender.cc
> File remoting/host/heartbeat_sender.cc (right):
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
> remoting/host/heartbeat_sender.cc:233: scoped_ptr<XmlElement> query(new
> XmlElement(
> On 2012/05/18 01:27:14, Wez wrote:
> > Can you add a comment here to the effect of "Create heartbeat stanza"
> 
> Done.
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
> remoting/host/heartbeat_sender.cc:239: scoped_ptr<XmlElement>
> log(ServerLogEntry::MakeLogStanza());
> On 2012/05/18 01:27:14, Wez wrote:
> > And one here to the effect of "Create heartbeat log stanza", and to note
that
> > the log stanza's not signed?
> 
> Done.
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/heartbeat_sender...
> remoting/host/heartbeat_sender.cc:240: scoped_ptr<ServerLogEntry>
> log_entry(ServerLogEntry::MakeHeartbeat());
> On 2012/05/18 01:27:14, Wez wrote:
> > ServerLogEntry::MakeHeartbeat() looks strange here; the 
> 
> Done.
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry.h
> File remoting/host/server_log_entry.h (right):
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
> remoting/host/server_log_entry.h:31: static scoped_ptr<buzz::XmlElement>
> MakeLogStanza();
> On 2012/05/18 01:27:14, Wez wrote:
> > nit: "Log" is a little redundant in the name; how about MakeStanza(), or
> > NewStanza() to distinguish this method from the Make* methods below?
> 
> Done.
> 
>
http://codereview.chromium.org/10409017/diff/1/remoting/host/server_log_entry...
> remoting/host/server_log_entry.h:38: static ServerLogEntry* MakeHeartbeat();
> On 2012/05/18 01:27:14, Wez wrote:
> > MakeHeartbeat unfortunately reads in HeartbeatSender as though this method
> > creates the heart-beat; consider ServerLogEntry::MakeForHeartbeat() or
> > ForHeartbeat()?
> 
> Done.

ping

[Sergey Ulanov, 2012-05-21 20:06:02]

lgtm with some style nits.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/heartbeat_sen...
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/heartbeat_sen...
remoting/host/heartbeat_sender.cc:240: // Create heartbeat log stanza (which
isn't signed).
The log message is part of the same XMPP stanza, so this comment is confusing. I
suggest "Append log message." or something like that.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.cc:72: ServerLogEntry*
ServerLogEntry::MakeForHeartbeat() {
nit: add "// static" above this line.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.h:35: static ServerLogEntry*
MakeSessionStateChange(bool connection);
scoped_ptr<>, maybe in a separate CL

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.h:38: static ServerLogEntry* MakeForHeartbeat();
scoped_ptr<>?

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry_unittest.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:73:
ASSERT_TRUE(VerifyStanza(key_value_pairs, keys, stanza.get(), &error)) <<
nit: wrap << operator.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:85:
ASSERT_TRUE(VerifyStanza(key_value_pairs, keys, stanza.get(), &error)) <<
nit: wrap << operator.

[simonmorris, 2012-05-21 20:40:22]

http://codereview.chromium.org/10409017/diff/1006/remoting/host/heartbeat_sen...
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/heartbeat_sen...
remoting/host/heartbeat_sender.cc:240: // Create heartbeat log stanza (which
isn't signed).
On 2012/05/21 20:06:02, sergeyu wrote:
> The log message is part of the same XMPP stanza, so this comment is confusing.
I
> suggest "Append log message." or something like that.

Done.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.cc:72: ServerLogEntry*
ServerLogEntry::MakeForHeartbeat() {
On 2012/05/21 20:06:02, sergeyu wrote:
> nit: add "// static" above this line.

Done.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.h:35: static ServerLogEntry*
MakeSessionStateChange(bool connection);
On 2012/05/21 20:06:02, sergeyu wrote:
> scoped_ptr<>, maybe in a separate CL

Follow-up CL (otherwise the CL description is inaccurate).

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry.h:38: static ServerLogEntry* MakeForHeartbeat();
On 2012/05/21 20:06:02, sergeyu wrote:
> scoped_ptr<>?

Follow-up CL, for consistency.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
File remoting/host/server_log_entry_unittest.cc (right):

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:73:
ASSERT_TRUE(VerifyStanza(key_value_pairs, keys, stanza.get(), &error)) <<
On 2012/05/21 20:06:02, sergeyu wrote:
> nit: wrap << operator.

Done.

http://codereview.chromium.org/10409017/diff/1006/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:85:
ASSERT_TRUE(VerifyStanza(key_value_pairs, keys, stanza.get(), &error)) <<
On 2012/05/21 20:06:02, sergeyu wrote:
> nit: wrap << operator.

Done.

[Wez, 2012-05-21 20:43:47]

LGTM

http://codereview.chromium.org/10409017/diff/8002/remoting/host/server_log_en...
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/10409017/diff/8002/remoting/host/server_log_en...
remoting/host/server_log_entry.h:35: static ServerLogEntry*
MakeSessionStateChange(bool connection);
nit: This should be MakeForSessionStateChange() to match MakeForHeartbeat(),
below, now.

[simonmorris, 2012-05-21 20:47:25]

On 2012/05/21 20:43:47, Wez wrote:
> LGTM
> 
>
http://codereview.chromium.org/10409017/diff/8002/remoting/host/server_log_en...
> File remoting/host/server_log_entry.h (right):
> 
>
http://codereview.chromium.org/10409017/diff/8002/remoting/host/server_log_en...
> remoting/host/server_log_entry.h:35: static ServerLogEntry*
> MakeSessionStateChange(bool connection);
> nit: This should be MakeForSessionStateChange() to match MakeForHeartbeat(),
> below, now.

I'll make that change in the same follow-up CL that will make these return a
scoped_ptr.

[commit-bot: I haz the power, 2012-05-21 22:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10409017/6002

[commit-bot: I haz the power, 2012-05-22 02:16:01]

Try job failure for 10409017-6002 (retry) on win_rel for steps "base_unittests,
browser_tests, net_unittests, sync_unit_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 15:21:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10409017/6002

[commit-bot: I haz the power, 2012-05-22 15:47:58]

Try job failure for 10409017-6002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-22 16:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10409017/6002

[commit-bot: I haz the power, 2012-05-22 16:38:16]

Try job failure for 10409017-6002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...