Log when the host is started for an account.

Log when the host is started for an account.

BUG=132683
TEST=me2me_virtual_host (manual) and it2me_basic (pyauto) on Linux

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154441

[rmsousa, 2012-08-29 13:16:04]

I thought about adding the OnStart method to the HostStatusObserver instead, to
be more general/consistent, but the object that calls the HostStatusObserver
interface (ChromotingHost) doesn't know the user_id.

[alexeypa (please no reviews), 2012-08-29 15:15:48]

A drive-by.

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
File remoting/host/host_event_logger.h (right):

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
ChromotingHost::Start is called after HostEventLogger is created. I think it is
better to add OnStart to HostStatusObserver and pass uid to ChromotigHost::Start
which in its turn will pass it to HostStatusObserver::OnStart.

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
File remoting/host/host_event_logger_posix.cc (right):

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
remoting/host/host_event_logger_posix.cc:42: //HostEventLogger implementation.
nit: space in front of HostEventLogger

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_...
remoting/host/host_event_logger_win.cc:43: //HostEventLogger implementation.
nit: space in front of HostEventLogger

[Jamie, 2012-08-29 17:24:14]

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger.h (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
On 2012/08/29 15:15:48, alexeypa wrote:
> ChromotingHost::Start is called after HostEventLogger is created. I think it
is
> better to add OnStart to HostStatusObserver and pass uid to
ChromotigHost::Start
> which in its turn will pass it to HostStatusObserver::OnStart.

I agree. HostStatusObserver is the best place to add callbacks of this nature,
assuming the plumbing to get the relevant information to the call site is not
too painful.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger_posix.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_posix.cc:93: void
HostEventLoggerPosix::OnShutdown() {
Much as I dislike "while you're here" type comments, it's very strange to me
that we don't log anything on shutdown. Can you add a shutdown message as well?

[Lambros, 2012-08-29 17:34:25]

I'm inclined to agree that HostStatusObserver is the right place for OnStart(),
even though the plumbing is a bit of a nuisance. alexeypa's suggestion of
passing xmpp_login into ChromotingHost::Start() seems a bit yucky, but is
probably the best we can do for now.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger.h (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
nit: no semicolon needed

[rmsousa, 2012-08-29 19:24:40]

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger.h (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
On 2012/08/29 17:34:25, Lambros wrote:
> nit: no semicolon needed

Done.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
On 2012/08/29 15:15:48, alexeypa wrote:
> ChromotingHost::Start is called after HostEventLogger is created. I think it
is
> better to add OnStart to HostStatusObserver and pass uid to
ChromotigHost::Start
> which in its turn will pass it to HostStatusObserver::OnStart.

Done.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger.h:28: virtual void OnStart(const std::string&
xmpp_login) {};
On 2012/08/29 17:24:14, Jamie wrote:
> On 2012/08/29 15:15:48, alexeypa wrote:
> > ChromotingHost::Start is called after HostEventLogger is created. I think it
> is
> > better to add OnStart to HostStatusObserver and pass uid to
> ChromotigHost::Start
> > which in its turn will pass it to HostStatusObserver::OnStart.
> 
> I agree. HostStatusObserver is the best place to add callbacks of this nature,
> assuming the plumbing to get the relevant information to the call site is not
> too painful.

Done.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger_posix.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_posix.cc:42: //HostEventLogger implementation.
On 2012/08/29 15:15:48, alexeypa wrote:
> nit: space in front of HostEventLogger

Done.

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_posix.cc:93: void
HostEventLoggerPosix::OnShutdown() {
On 2012/08/29 17:24:14, Jamie wrote:
> Much as I dislike "while you're here" type comments, it's very strange to me
> that we don't log anything on shutdown. Can you add a shutdown message as
well?

I added the message, but I think Lambros's comments on
https://chromiumcodereview.appspot.com/10800023#msg4 still apply - at least on
Linux I don't see it being executed. I can try looking at cleaning up our
shutdown next, if nobody else is already looking at it.
(also, at this point the xmpp_login used at start isn't stored anywhere (aside
from privately at signaling_strategy), so we don't have an easy way to log it
here).

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger_win.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_win.cc:43: //HostEventLogger implementation.
On 2012/08/29 15:15:48, alexeypa wrote:
> nit: space in front of HostEventLogger

Done.

[Jamie, 2012-08-30 06:09:33]

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger_posix.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_posix.cc:93: void
HostEventLoggerPosix::OnShutdown() {
On 2012/08/29 19:24:40, rmsousa wrote:
> On 2012/08/29 17:24:14, Jamie wrote:
> > Much as I dislike "while you're here" type comments, it's very strange to me
> > that we don't log anything on shutdown. Can you add a shutdown message as
> well?
> 
> I added the message, but I think Lambros's comments on
> https://chromiumcodereview.appspot.com/10800023#msg4 still apply - at least on
> Linux I don't see it being executed. I can try looking at cleaning up our
> shutdown next, if nobody else is already looking at it.
> (also, at this point the xmpp_login used at start isn't stored anywhere (aside
> from privately at signaling_strategy), so we don't have an easy way to log it
> here).

Ah yes, I remember now. Thanks for adding it, but I think that you were right
the first time and there's not much point having a log message that we know
never gets logged, particularly given the extra method and string resource that
needs to be added on Windows. Sorry about that; could you replace it with a TODO
along the lines of your suggested clean up above.

http://chromiumcodereview.appspot.com/10893009/diff/8003/remoting/host/chromo...
File remoting/host/chromoting_host_unittest.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/8003/remoting/host/chromo...
remoting/host/chromoting_host_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Can you add a unit test for the new callback?

[rmsousa, 2012-08-30 16:29:03]

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
File remoting/host/host_event_logger_posix.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/5001/remoting/host/host_e...
remoting/host/host_event_logger_posix.cc:93: void
HostEventLoggerPosix::OnShutdown() {
On 2012/08/30 06:09:33, Jamie wrote:
> On 2012/08/29 19:24:40, rmsousa wrote:
> > On 2012/08/29 17:24:14, Jamie wrote:
> > > Much as I dislike "while you're here" type comments, it's very strange to
me
> > > that we don't log anything on shutdown. Can you add a shutdown message as
> > well?
> > 
> > I added the message, but I think Lambros's comments on
> > https://chromiumcodereview.appspot.com/10800023#msg4 still apply - at least
on
> > Linux I don't see it being executed. I can try looking at cleaning up our
> > shutdown next, if nobody else is already looking at it.
> > (also, at this point the xmpp_login used at start isn't stored anywhere
(aside
> > from privately at signaling_strategy), so we don't have an easy way to log
it
> > here).
> 
> Ah yes, I remember now. Thanks for adding it, but I think that you were right
> the first time and there's not much point having a log message that we know
> never gets logged, particularly given the extra method and string resource
that
> needs to be added on Windows. Sorry about that; could you replace it with a
TODO
> along the lines of your suggested clean up above.

Done.

http://chromiumcodereview.appspot.com/10893009/diff/8003/remoting/host/chromo...
File remoting/host/chromoting_host_unittest.cc (right):

http://chromiumcodereview.appspot.com/10893009/diff/8003/remoting/host/chromo...
remoting/host/chromoting_host_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/08/30 06:09:33, Jamie wrote:
> Can you add a unit test for the new callback?

Done.

[Jamie, 2012-08-30 17:37:24]

lgtm

[commit-bot: I haz the power, 2012-08-30 17:39:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/10893009/13001

[commit-bot: I haz the power, 2012-08-30 20:26:15]

Try job failure for 10893009-13001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-30 20:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/10893009/13001

[commit-bot: I haz the power, 2012-08-30 23:28:39]

Try job failure for 10893009-13001 (retry) (previous was lost) on linux_rel for
step "interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-31 12:47:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/10893009/13001

[commit-bot: I haz the power, 2012-08-31 16:26:25]

Change committed as 154441