Host offline status reporting.

Host offline status reporting.

This is change #1 of the Host offline status reporting work: 
1. Define the structure of the offline status message.
2. The host sends the offline status message if the underlying XMPP connection is live.


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

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

[rmsousa, 2013-07-02 02:09:24]

The general approach seems good, but i have some comments about the
implementation details.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
File remoting/host/offline_status_sender.cc (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:24: const char kOfflineStatusTag[] =
"offline-status";
Can you make the tag "host-status", and make "offline" a separate "status"
attribute?

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:28: const char kStatusAttr[] = "status";
if we add the separate online/offline "status" attribute, this can be renamed
"exit-code".

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:33:
OfflineStatusSender::OfflineStatusSender(
Please rename to HostStatusSender (see above)

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:96: // The error code is currently unused
and always contains a value of 0.
We should add this only when we know where it'll come from, and what we'll do
with it. Otherwise, it's complicated:

Without defining what will go in this field, we can't even know what type it
should be (Unix errnos are int, HRESULTs are longs, and GetLastErrors are
DWORDs).

And then, this stanza doesn't currently have the host's OS, so it's impossible
to know how to interpret this number from this stanza (i.e. is it an HRESULT, a
GetLastError() code, a UNIX errno?).

This might be something we will want to log in the future, but I'd prefer to
wait until we know the scenario before we design it. In general, adding
attributes to an existing structure is easy (be it an XML message, a protobuf, a
JSON structure, etc), but removing superfluous attributes is much harder (often
near-impossible, e.g. with protobufs)

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:97: // In the future we may use it for
things like windows error code.
Nit: Comments describing the arguments should be in the header.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:106:
offline_status->AddElement(version_tag.release());
please piggyback a log message, like heartbeat sender does.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:112: QName(kChromotingXmlNamespace,
kHeartbeatSignatureTag)));
s/Heartbeat/OfflineStatus

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.cc:114: std::string message =
signal_strategy_->GetLocalJid();
I'm not sure we need to sign this message, but if we do, we should sign the
actual payload (status) along with the JID.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
File remoting/host/offline_status_sender.h (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:43: // "user@gmail.com/chromoting123123
456".
Nit: please update the example and/or the description.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:59: void SendOfflineStatus(int status,
uint errorcode = 0);
Do not use default arguments:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:59: void SendOfflineStatus(int status,
uint errorcode = 0);
Please add a comment explaining the function and its parameters.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
File remoting/host/offline_status_sender_unittest.cc (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Nit: remove (c), change to 2013 in all new files

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:1031: // If the timer is running that means
a shutdown is already in progress.
Can this happen? This is only called on the HOST_INITIALIZING/HOST_STARTED to
HOST_STOPPING transition.

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:1035: // Delay the shutdown by 2 second to
allow SendOfflineStatus to complete.
Nit: For the simple "perform this action later" scenario, we don't need timer,
PostDelayedTask
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h...
should be enough.

Timer is a wrapper around postdelayedtask, that extends it with some
timer-specific cases, such as resetting, which we don't need here.

[weitao, 2013-07-03 19:23:30]

Major changes:
1. Rename OfflineStatusSender to HostStatusSender
2. Add online/offline attribute
3. Rename various tags and remove errorcode
4. Add timestamp to signature

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
File remoting/host/offline_status_sender.h (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:43: // "user@gmail.com/chromoting123123
456".
On 2013/07/02 02:09:24, rmsousa wrote:
> Nit: please update the example and/or the description.

Done.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:59: void SendOfflineStatus(int status,
uint errorcode = 0);
On 2013/07/02 02:09:24, rmsousa wrote:
> Do not use default arguments:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

Done.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender.h:59: void SendOfflineStatus(int status,
uint errorcode = 0);
On 2013/07/02 02:09:24, rmsousa wrote:
> Do not use default arguments:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

Done.

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
File remoting/host/offline_status_sender_unittest.cc (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/offline_status_...
remoting/host/offline_status_sender_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/07/02 02:09:24, rmsousa wrote:
> Nit: remove (c), change to 2013 in all new files

Done.

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:1031: // If the timer is running that means
a shutdown is already in progress.
Done.

https://codereview.chromium.org/18075003/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:1035: // Delay the shutdown by 2 second to
allow SendOfflineStatus to complete.
On 2013/07/02 02:09:24, rmsousa wrote:
> Nit: For the simple "perform this action later" scenario, we don't need timer,
> PostDelayedTask
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h...
> should be enough.
> 
> Timer is a wrapper around postdelayedtask, that extends it with some
> timer-specific cases, such as resetting, which we don't need here.

Done.

[rmsousa, 2013-07-04 01:20:14]

this looks generally good, I've just some comments on how to encode the values.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:102: QName(kChromotingXmlNamespace,
kStatusAttr), base::IntToString(status));
we typically don't send/receive enums as raw numbers, we send them as strings.
(i.e. 'rem:status="ONLINE"' rather than 'rem:status="0"')

(for other enums [that are only used for logs] we use lowercase, but we already
use all caps for the status string in the JSON (See
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/ho...),
so we should be consistent with that)

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:106: QName(kChromotingXmlNamespace,
kExitCodeAttr),
For this one, I'm inclined to say that we should also treat it as an enum (and
send a stringized version of the enum names, such as
"INVALID_HOST_CONFIGURATION", or "invalid-host-configuration"), rather than an
integer.

Exit codes do make sense as pure integers (saying that "a [Unix] program's exit
code is '103'" is an acceptable sentence, it's not like saying that "a host's
status is '1'"), but the codes that we'll actually see here are defined as
values of an enum
(https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/host...).
Treating them as enums and stringizing them requires some more work (including
server-side) to handle it, but it does make the attribute more semantic, and has
the advantage that if we later add a new exit code, an older webapp that doesn't
recognize that exit code can show it as, for example, "Unknown host exit code:
'INVALID_HOST_CONFIGURATION'" as opposed to "Unknown host exit code: '137'".

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:122:
log->AddElement(log_entry->ToStanza().release());
this log message should include the status and exit code as well.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender.h (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.h:43: // "user@gmail.com/chromoting123123".
Nit: please update

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender_unittest.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender_unittest.cc:183: // For a unit test
SendHostStatus and the validation should complete in
You shouldn't depend on real wallclock timing in unit tests, it introduces
flakiness.

For these purposes, I think it's ok to simply not test the time at all (we do
need to test that the signature includes *some number* as "time", and that the
signature matches, which we do above, but we don't need to test that this value
is a meaningful time, that's the purpose of the unit tests for base::Time).

FYI, if we did need/want tests that depend on time, the appropriate way to do
that is to have the tested object have a base::Clock attribute, that gets
initialized to a base::DefaultClock() in the default constructor, and to have a
separate, VisibleForTesting constructor that takes a custom Clock (to which the
unit test would typically pass a SimpleTestClock).

https://codereview.chromium.org/18075003/diff/6001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:294: 
Unused

[weitao, 2013-07-09 00:37:40]

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:102: QName(kChromotingXmlNamespace,
kStatusAttr), base::IntToString(status));
On 2013/07/04 01:20:14, rmsousa wrote:
> we typically don't send/receive enums as raw numbers, we send them as strings.
> (i.e. 'rem:status="ONLINE"' rather than 'rem:status="0"')
> 
> (for other enums [that are only used for logs] we use lowercase, but we
already
> use all caps for the status string in the JSON (See
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/webapp/ho...),
> so we should be consistent with that)

Done.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:106: QName(kChromotingXmlNamespace,
kExitCodeAttr),
On 2013/07/04 01:20:14, rmsousa wrote:
> For this one, I'm inclined to say that we should also treat it as an enum (and
> send a stringized version of the enum names, such as
> "INVALID_HOST_CONFIGURATION", or "invalid-host-configuration"), rather than an
> integer.
> 
> Exit codes do make sense as pure integers (saying that "a [Unix] program's
exit
> code is '103'" is an acceptable sentence, it's not like saying that "a host's
> status is '1'"), but the codes that we'll actually see here are defined as
> values of an enum
>
(https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/host...).
> Treating them as enums and stringizing them requires some more work (including
> server-side) to handle it, but it does make the attribute more semantic, and
has
> the advantage that if we later add a new exit code, an older webapp that
doesn't
> recognize that exit code can show it as, for example, "Unknown host exit code:
> 'INVALID_HOST_CONFIGURATION'" as opposed to "Unknown host exit code: '137'".

Done.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.cc:122:
log->AddElement(log_entry->ToStanza().release());
On 2013/07/04 01:20:14, rmsousa wrote:
> this log message should include the status and exit code as well.

Done.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender.h (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender.h:43: // "user@gmail.com/chromoting123123".
On 2013/07/04 01:20:14, rmsousa wrote:
> Nit: please update

Done.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
File remoting/host/host_status_sender_unittest.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/host_status_...
remoting/host/host_status_sender_unittest.cc:183: // For a unit test
SendHostStatus and the validation should complete in
On 2013/07/04 01:20:14, rmsousa wrote:
> You shouldn't depend on real wallclock timing in unit tests, it introduces
> flakiness.
> 
> For these purposes, I think it's ok to simply not test the time at all (we do
> need to test that the signature includes *some number* as "time", and that the
> signature matches, which we do above, but we don't need to test that this
value
> is a meaningful time, that's the purpose of the unit tests for base::Time).
> 
> FYI, if we did need/want tests that depend on time, the appropriate way to do
> that is to have the tested object have a base::Clock attribute, that gets
> initialized to a base::DefaultClock() in the default constructor, and to have
a
> separate, VisibleForTesting constructor that takes a custom Clock (to which
the
> unit test would typically pass a SimpleTestClock).

Done.

https://codereview.chromium.org/18075003/diff/6001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/18075003/diff/6001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:294: 
On 2013/07/04 01:20:14, rmsousa wrote:
> Unused

Done.

[rmsousa, 2013-07-09 03:13:41]

This looks generally good, just some nits and a comment on the logging.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
File remoting/host/host_exit_codes.h (right):

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:39: const std::string HostExitCodeStrings[] = {
Nit: Please create a .cc file for the string definitions and the translation
function.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:48: inline std::string ExitCodeToString(int
exit_code) {
Nit: Please take a HostExitCode (rather than int) explicitly, all the way to
HostProcess::ShutdownHost. The values won't be static checked, but this at least
makes it clears to callers that they can't pass arbitrary ints into that
function without defining them first.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:49: DCHECK(exit_code >=
kMinPermanentErrorExitCode &&
This will DCHECK for exit_code kSuccessExitCode (which is a valid ShutdownHost
parameter).

This
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...
seems like it could be useful for a sparse enum-string map.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/server_log_...
File remoting/host/server_log_entry.cc (right):

https://codereview.chromium.org/18075003/diff/19001/remoting/host/server_log_...
remoting/host/server_log_entry.cc:147: void ServerLogEntry::AddCustomField(const
std::string& name,
Could you pass the host_status and exit_code values as enums in the
MakeForHostStatus() constructor (and translate inside it), rather than adding a
generic key/value setter?

[weitao, 2013-07-09 19:02:17]

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
File remoting/host/host_exit_codes.h (right):

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:39: const std::string HostExitCodeStrings[] = {
On 2013/07/09 03:13:41, rmsousa wrote:
> Nit: Please create a .cc file for the string definitions and the translation
> function.

Done.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:48: inline std::string ExitCodeToString(int
exit_code) {
On 2013/07/09 03:13:41, rmsousa wrote:
> Nit: Please take a HostExitCode (rather than int) explicitly, all the way to
> HostProcess::ShutdownHost. The values won't be static checked, but this at
least
> makes it clears to callers that they can't pass arbitrary ints into that
> function without defining them first.

Done.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/host_exit_c...
remoting/host/host_exit_codes.h:49: DCHECK(exit_code >=
kMinPermanentErrorExitCode &&
On 2013/07/09 03:13:41, rmsousa wrote:
> This will DCHECK for exit_code kSuccessExitCode (which is a valid ShutdownHost
> parameter).
> 
> This
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...
> seems like it could be useful for a sparse enum-string map.

Done.

https://codereview.chromium.org/18075003/diff/19001/remoting/host/server_log_...
File remoting/host/server_log_entry.cc (right):

https://codereview.chromium.org/18075003/diff/19001/remoting/host/server_log_...
remoting/host/server_log_entry.cc:147: void ServerLogEntry::AddCustomField(const
std::string& name,
I think the ServerLogEntry class should be general purpose and shouldn't need to
know the details (attribute names, etc) of individual log entries created for
different kind of messages. It should be up to the consumers of ServerLogEntry
to add these name/value pairs. 

I am actually planning to submit a separate change to clean this up: hoisting
all the session state and heartbeat specific attribute references up to the
caller.

On 2013/07/09 03:13:41, rmsousa wrote:
> Could you pass the host_status and exit_code values as enums in the
> MakeForHostStatus() constructor (and translate inside it), rather than adding
a
> generic key/value setter?

[rmsousa, 2013-07-09 21:07:09]

lgtm

[commit-bot: I haz the power, 2013-07-09 21:13:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/18075003/34001

[commit-bot: I haz the power, 2013-07-09 21:40:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-09 23:13:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/18075003/61001

[commit-bot: I haz the power, 2013-07-10 03:57:18]

Change committed as 210731

[commit-bot: I haz the power, 2013-07-10 19:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/18075003/93001

[commit-bot: I haz the power, 2013-07-11 01:01:55]

Change committed as 210995