Conenct Chromoting plugin debug log to JS UI.

Conenct Chromoting plugin debug log to JS UI.

BUG=none
TEST=none

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

[dmac, 2011-06-26 14:34:44]

http://codereview.chromium.org/7262015/diff/2001/remoting/base/logger.h
File remoting/base/logger.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/base/logger.h#newco...
remoting/base/logger.h:13: extern const char* const log_severity_names[];
maybe I missed it. Where are we using these outside of Logger? Just verifying we
need to expose them.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:268: logger_->Log(logging::LOG_INFO, "Host
disconnected from talk network.");
why did we change this from a vlog?

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:331: logger_->Log(logging::LOG_INFO, "Client
connected: %s",
we did we change from a vlog?

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:224: Logger* logger_;
Chromoting host takes ownership of everything else it gets assigned. Why is
logger different?

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_logger.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.cc:37: // client UI.
fix up comment.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.cc:56: // client UI.
fix up comment

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_logger.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.h:20:
HostPluginLogger(HostNPScriptObject* scriptable);
explicit?

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

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:483: LOG_IF(ERROR, !is_good) <<
"LogDebugInfo failed";
did you want to fix the LOG_IF here?

http://codereview.chromium.org/7262015/diff/2001/remoting/host/simple_host_pr...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/simple_host_pr...
remoting/host/simple_host_process.cc:198: access_verifier.release(),
logger_.get());
do you actually need to call get here and below? C++ doesn't automatically call
through for you?

[garykac, 2011-06-27 21:24:20]

http://codereview.chromium.org/7262015/diff/2001/remoting/base/logger.h
File remoting/base/logger.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/base/logger.h#newco...
remoting/base/logger.h:13: extern const char* const log_severity_names[];
On 2011/06/26 14:34:44, dmac wrote:
> maybe I missed it. Where are we using these outside of Logger? Just verifying
we
> need to expose them.

We're not so I made them protected statics (since subclasses need to use them).

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:268: logger_->Log(logging::LOG_INFO, "Host
disconnected from talk network.");
On 2011/06/26 14:34:44, dmac wrote:
> why did we change this from a vlog?

Because this is info that I want to see in the UI debug log (like the "host
connected" message above).

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:331: logger_->Log(logging::LOG_INFO, "Client
connected: %s",
On 2011/06/26 14:34:44, dmac wrote:
> we did we change from a vlog?

As above.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:224: Logger* logger_;
On 2011/06/26 14:34:44, dmac wrote:
> Chromoting host takes ownership of everything else it gets assigned. Why is
> logger different?

Unlike the other params, the logger is part of the scriptable object, and the SO
needs to keep using it. So ownership seems to fit better in the SO.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_logger.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.cc:37: // client UI.
On 2011/06/26 14:34:44, dmac wrote:
> fix up comment.

Done.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.cc:56: // client UI.
On 2011/06/26 14:34:44, dmac wrote:
> fix up comment

Done.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_logger.h (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_logger.h:20:
HostPluginLogger(HostNPScriptObject* scriptable);
On 2011/06/26 14:34:44, dmac wrote:
> explicit?

Absolutely. Good catch.

Fixed here and for the client plugin logger

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

http://codereview.chromium.org/7262015/diff/2001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:483: LOG_IF(ERROR, !is_good) <<
"LogDebugInfo failed";
On 2011/06/26 14:34:44, dmac wrote:
> did you want to fix the LOG_IF here?

Calling logger_->Log will just redirect back here and (most likely) repeat the
failure.

http://codereview.chromium.org/7262015/diff/2001/remoting/host/simple_host_pr...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/7262015/diff/2001/remoting/host/simple_host_pr...
remoting/host/simple_host_process.cc:198: access_verifier.release(),
logger_.get());
On 2011/06/26 14:34:44, dmac wrote:
> do you actually need to call get here and below? C++ doesn't automatically
call
> through for you?

When using it, yes. But here is tries to pass scoped_ptr<HostPluginLogger>& and
fails.

[garykac, 2011-07-06 17:55:45]

ping

[dmac, 2011-07-06 19:51:49]

LGTM

http://codereview.chromium.org/7262015/diff/8001/remoting/base/logger.cc
File remoting/base/logger.cc (right):

http://codereview.chromium.org/7262015/diff/8001/remoting/base/logger.cc#newc...
remoting/base/logger.cc:15: "INFO", "WARNING", "ERROR", "ERROR_REPORT", "FATAL"
};
I would probably put the close brace on it's own line.