The host sends simple log entries to the server.

The host sends simple log entries to the server.

BUG=
TEST=


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

[simonmorris, 2011-11-15 23:00:04]

PTAL

[Sergey Ulanov, 2011-11-17 01:16:33]

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:24: }
  // namespace

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:67: if (!message_loop_->BelongsToCurrentThread())
{
Currently this method is called only on network thread. So I don't think we
really need this trampoline, and probably won't need it in the future.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:73: pending_entries_.push_back(entry);
do we really need |pedning_entries_|? SendPendingEntries() is going to send it
immediately anyway. Or is it so that we don't lose messages while signalling is
disconnected?

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:97: base::Bind(&LogToServer::ProcessResponse,
this)));
you can pass null callback here (i.e. IqSender::ReplyCallback() ).

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:102: void LogToServer::ProcessResponse(const
XmlElement* response) {
Don't think we need this.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:27: class LogToServer :
nit: move : to the next line.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:29: public base::RefCountedThreadSafe<LogToServer>
{
given that this object outlives the host (it must to because it is a
HostStatusObserver), I don't think it needs to be refcounted.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:31: LogToServer(base::MessageLoopProxy*
message_loop);
explicit

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server_...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server_...
remoting/host/log_to_server_unittest.cc:26: class LogToServerTest : public
testing::Test {
Thanks for writing unittests!

http://codereview.chromium.org/8468015/diff/3001/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:515: log_to_server_ = new
LogToServer(host_context_.network_message_loop());
Can you please add flag to disable/enable logging?

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:41: }
// namespace

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:59: Set(kKeyOsName, kValueOsNameWindows);
Can we use base::SysInfo::OperatingSystemName() here?

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:82: Set(kKeyCpu,
SysInfo::CPUArchitecture().c_str());
It would be also useful to log NumberOfProcessors()

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.h:38: typedef std::map<std::string, std::string>
Dict;
types should be defined after "private:"
Maybe call it ValuesDictionary?

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.h:39: Dict dict_;
dict_ is ambiguous name. Maybe values_.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry_unittest.cc:17: static int countAttrs(XmlElement*
elem) {
CountAttrs?

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry_unittest.cc:47: ASSERT_EQ(expected,
ServerLogEntryTest::countAttrs(stanza.get()));
In protocol/jingle_messages_unittests.cc there is VerifyXml() function that I
implemented to compare XML blobs - it may be also useful here.

[simonmorris, 2011-11-18 19:23:04]

PTAL

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:24: }
On 2011/11/17 01:16:33, sergeyu wrote:
>   // namespace

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:67: if (!message_loop_->BelongsToCurrentThread())
{
On 2011/11/17 01:16:33, sergeyu wrote:
> Currently this method is called only on network thread. So I don't think we
> really need this trampoline, and probably won't need it in the future.

Is it very unlikely that we'll ever want to log an event from another thread?
If not, I think the cost of the trampoline is very low compared to the cost
of getting threading wrong.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:73: pending_entries_.push_back(entry);
On 2011/11/17 01:16:33, sergeyu wrote:
> do we really need |pedning_entries_|? SendPendingEntries() is going to send it
> immediately anyway. Or is it so that we don't lose messages while signalling
is
> disconnected?

Yes, the idea was to avoid losing messages while signalling is disconnected.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:97: base::Bind(&LogToServer::ProcessResponse,
this)));
On 2011/11/17 01:16:33, sergeyu wrote:
> you can pass null callback here (i.e. IqSender::ReplyCallback() ).

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:102: void LogToServer::ProcessResponse(const
XmlElement* response) {
On 2011/11/17 01:16:33, sergeyu wrote:
> Don't think we need this.

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:27: class LogToServer :
On 2011/11/17 01:16:33, sergeyu wrote:
> nit: move : to the next line.

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:29: public base::RefCountedThreadSafe<LogToServer>
{
On 2011/11/17 01:16:33, sergeyu wrote:
> given that this object outlives the host (it must to because it is a
> HostStatusObserver), I don't think it needs to be refcounted.

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.h:31: LogToServer(base::MessageLoopProxy*
message_loop);
On 2011/11/17 01:16:33, sergeyu wrote:
> explicit

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server_...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server_...
remoting/host/log_to_server_unittest.cc:26: class LogToServerTest : public
testing::Test {
On 2011/11/17 01:16:33, sergeyu wrote:
> Thanks for writing unittests!

You're welcome!

http://codereview.chromium.org/8468015/diff/3001/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:515: log_to_server_ = new
LogToServer(host_context_.network_message_loop());
On 2011/11/17 01:16:33, sergeyu wrote:
> Can you please add flag to disable/enable logging?

Done.

It's just disabled for now - enabling it can go in
a separate CL.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:41: }
On 2011/11/17 01:16:33, sergeyu wrote:
> // namespace

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:59: Set(kKeyOsName, kValueOsNameWindows);
On 2011/11/17 01:16:33, sergeyu wrote:
> Can we use base::SysInfo::OperatingSystemName() here?

This field will be an enum in the backend storage, so that
it's simple to query at this granularity (which is the
intended granularity, for now). But we could add an
"OS long name" field, for more granularity. The backend
would need updating first, though.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.cc:82: Set(kKeyCpu,
SysInfo::CPUArchitecture().c_str());
On 2011/11/17 01:16:33, sergeyu wrote:
> It would be also useful to log NumberOfProcessors()

Yes. The client webapp gets these fields from
navigator.userAgent, which doesn't export the number of
processors - not that the lack of symmetry is any
particular problem.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.h:38: typedef std::map<std::string, std::string>
Dict;
On 2011/11/17 01:16:33, sergeyu wrote:
> types should be defined after "private:"
> Maybe call it ValuesDictionary?

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry.h:39: Dict dict_;
On 2011/11/17 01:16:33, sergeyu wrote:
> dict_ is ambiguous name. Maybe values_.

Done.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
File remoting/host/server_log_entry_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry_unittest.cc:17: static int countAttrs(XmlElement*
elem) {
On 2011/11/17 01:16:33, sergeyu wrote:
> CountAttrs?

Removed altogether.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/server_log_ent...
remoting/host/server_log_entry_unittest.cc:47: ASSERT_EQ(expected,
ServerLogEntryTest::countAttrs(stanza.get()));
On 2011/11/17 01:16:33, sergeyu wrote:
> In protocol/jingle_messages_unittests.cc there is VerifyXml() function that I
> implemented to compare XML blobs - it may be also useful here.

Done (something similar).

[Sergey Ulanov, 2011-11-22 02:01:43]

LGTM, but please see my nits.

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:67: if (!message_loop_->BelongsToCurrentThread())
{
On 2011/11/18 19:23:04, simonmorris wrote:
> On 2011/11/17 01:16:33, sergeyu wrote:
> > Currently this method is called only on network thread. So I don't think we
> > really need this trampoline, and probably won't need it in the future.
> 
> Is it very unlikely that we'll ever want to log an event from another thread?
> If not, I think the cost of the trampoline is very low compared to the cost
> of getting threading wrong.

There is no risk in not having this trampoline as long as you add DCHECK to
verify that this method runs on correct thread, e.g. as in
OnSignallingConnected().
We get most of events on the network thread, so probably we will not need to log
anything from other threads.  In any case, calling code can jump to network
thread if it needs to.
Code gets hard to maintain when threading is mixed with other logic, so when it
comes to threading, I'd like each class falls in one of the following
categories:
 1. Non ref-counted classes that always run on the same thread (preferably
marked as NonThreadSafe).
 2. Ref-counted, thread-safe objects that may jump from one thread to another,
but don't contain much logic beside that.
This approach allows to separate threading details from everything else.
This particular class falls in the first category. If we ever need to log
something from other threads we can add LogToServerProxy that would know how to
switch threads.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/log_to_server...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/11001/remoting/host/log_to_server...
remoting/host/log_to_server_unittest.cc:24: }
// namespace

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:516: if (enable_log_to_server_) {
nit: please remove brackets for consistency with other code in this file.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:527: if (log_to_server_.get()) {
nit: I think you can merge this if and the previous one together.

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

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry.h:33: typedef std::map<std::string, std::string>
KeyValuePairs;
nit: ValuesMap?

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry.h:36: void Set(const char* key, const char*
value);
nit: Should this be public

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

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:21: static bool verifyStanza(
nit:VerifyStanza

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:22: const std::map<std::string,
std::string>& keyValuePairs,
nit:key_value_pair

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:31: + attr->Name().Namespace();
nit: + must be on the previous line. We have different rules for operator
wrapping in C++ and Java :(

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:46: + ": expected " + value;
nit: + must be on the previous line.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:51: int attrCountExpected =
keyValuePairs.size() + keys.size();
attr_count_expected

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:68: std::map<std::string,
std::string> keyValuePairs;
not: key_value_pairs

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:82: std::map<std::string,
std::string> keyValuePairs;
same here

[commit-bot: I haz the power, 2011-11-22 21:00:14]

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

[commit-bot: I haz the power, 2011-11-22 22:48:58]

Change committed as 111239

[simonmorris, 2011-11-22 23:27:35]

FYI

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/8468015/diff/3001/remoting/host/log_to_server....
remoting/host/log_to_server.cc:67: if (!message_loop_->BelongsToCurrentThread())
{
On 2011/11/22 02:01:43, sergeyu wrote:
> On 2011/11/18 19:23:04, simonmorris wrote:
> > On 2011/11/17 01:16:33, sergeyu wrote:
> > > Currently this method is called only on network thread. So I don't think
we
> > > really need this trampoline, and probably won't need it in the future.
> > 
> > Is it very unlikely that we'll ever want to log an event from another
thread?
> > If not, I think the cost of the trampoline is very low compared to the cost
> > of getting threading wrong.
> 
> There is no risk in not having this trampoline as long as you add DCHECK to
> verify that this method runs on correct thread, e.g. as in
> OnSignallingConnected().
> We get most of events on the network thread, so probably we will not need to
log
> anything from other threads.  In any case, calling code can jump to network
> thread if it needs to.
> Code gets hard to maintain when threading is mixed with other logic, so when
it
> comes to threading, I'd like each class falls in one of the following
> categories:
>  1. Non ref-counted classes that always run on the same thread (preferably
> marked as NonThreadSafe).
>  2. Ref-counted, thread-safe objects that may jump from one thread to another,
> but don't contain much logic beside that.
> This approach allows to separate threading details from everything else.
> This particular class falls in the first category. If we ever need to log
> something from other threads we can add LogToServerProxy that would know how
to
> switch threads.

Sounds like a good pattern.

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/log_to_server...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/8468015/diff/11001/remoting/host/log_to_server...
remoting/host/log_to_server_unittest.cc:24: }
On 2011/11/22 02:01:43, sergeyu wrote:
> // namespace 

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:516: if (enable_log_to_server_) {
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: please remove brackets for consistency with other code in this file.

Merged with another if, so braces are now needed.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:527: if (log_to_server_.get()) {
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: I think you can merge this if and the previous one together.

Done.

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

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry.h:33: typedef std::map<std::string, std::string>
KeyValuePairs;
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: ValuesMap?

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry.h:36: void Set(const char* key, const char*
value);
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: Should this be public

No. This way the ServerLogEntry class controls the set of
keys and values that can be used, which makes it easier to
verify that only keys and values compatible with the
backend will be used.

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

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:21: static bool verifyStanza(
On 2011/11/22 02:01:43, sergeyu wrote:
> nit:VerifyStanza

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:22: const std::map<std::string,
std::string>& keyValuePairs,
On 2011/11/22 02:01:43, sergeyu wrote:
> nit:key_value_pair

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:31: + attr->Name().Namespace();
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: + must be on the previous line. We have different rules for operator
> wrapping in C++ and Java :(

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:46: + ": expected " + value;
On 2011/11/22 02:01:43, sergeyu wrote:
> nit: + must be on the previous line.

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:51: int attrCountExpected =
keyValuePairs.size() + keys.size();
On 2011/11/22 02:01:43, sergeyu wrote:
> attr_count_expected

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:68: std::map<std::string,
std::string> keyValuePairs;
On 2011/11/22 02:01:43, sergeyu wrote:
> not: key_value_pairs

Done.

http://codereview.chromium.org/8468015/diff/11001/remoting/host/server_log_en...
remoting/host/server_log_entry_unittest.cc:82: std::map<std::string,
std::string> keyValuePairs;
On 2011/11/22 02:01:43, sergeyu wrote:
> same here

Done.