Modify Chromoting logging to hook into base logging.

Modify Chromoting logging to hook into base logging.

BUG=none
TEST=none

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

[dmac, 2011-07-21 23:37:06]

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc#newco...
remoting/base/util.cc:18: #if _MSC_VER >= 1400
add a comment about this?

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc#newco...
remoting/base/util.cc:23: struct tm* tm_time = &local_time;
why not just use local_time. instead of tm_time?

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:55: static bool logging_to_plugin_
= false;
should these be marked with a g in front of them? At the very least they aren't
member vars so they shouldn't have a _ on the end.

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:287: logging_to_plugin_ = true;
logging_to_plugin_ race conditions about the global var on different threads?

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:90: static bool LogToUI(int
severity, const char* file, int line,
add a comment to the function please?

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:130:
DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::ChromotingInstance);
are we sure about this?

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.cc:305: LOG(WARNING) <<
"Exception when invoking JS callback"
Should this be LOG(ERROR) to match the others?

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

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:69: static bool logging_to_plugin_ =
false;
again, should these be prefixed with a g? and they shouldn't have a _

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:82: logging_old_handler_ =
logging::GetLogMessageHandler();
Is this not assuming that HostNPScriptObject is only going to be created once?

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:491: if (logging_scriptable_object_
&& !logging_to_plugin_) {
accessing logging_to_plugin_ from multiple thread problems?

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

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:70: static bool LogToUI(int severity,
const char* file, int line,
comment method please.

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:70: static bool LogToUI(int severity,
const char* file, int line,
severity is not an enum?

[garykac, 2011-08-02 00:15:37]

+sergeyu for the thread proxy

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc#newco...
remoting/base/util.cc:18: #if _MSC_VER >= 1400
On 2011/07/21 23:37:07, dmac wrote:
> add a comment about this?

Added a comment to the block of code since this was modified from
base/logging.cc's timestamp code.

http://codereview.chromium.org/7355011/diff/11001/remoting/base/util.cc#newco...
remoting/base/util.cc:23: struct tm* tm_time = &local_time;
On 2011/07/21 23:37:07, dmac wrote:
> why not just use local_time. instead of tm_time?

To duplicate behavior from LogMessage

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:55: static bool logging_to_plugin_
= false;
On 2011/07/21 23:37:07, dmac wrote:
> should these be marked with a g in front of them? At the very least they
aren't
> member vars so they shouldn't have a _ on the end.

Done.

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:287: logging_to_plugin_ = true;
On 2011/07/21 23:37:07, dmac wrote:
> logging_to_plugin_ race conditions about the global var on different threads?

True. I pushed the check down into the "CorrectThread" code below.

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:90: static bool LogToUI(int
severity, const char* file, int line,
On 2011/07/21 23:37:07, dmac wrote:
> add a comment to the function please?

Done.

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:130:
DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::ChromotingInstance);
On 2011/07/21 23:37:07, dmac wrote:
> are we sure about this?

I added a thread proxy class (stolen from Sergey's code) to manage the logging
tasks.

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/7355011/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_scriptable_object.cc:305: LOG(WARNING) <<
"Exception when invoking JS callback"
On 2011/07/21 23:37:07, dmac wrote:
> Should this be LOG(ERROR) to match the others?

Done.

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

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:69: static bool logging_to_plugin_ =
false;
On 2011/07/21 23:37:07, dmac wrote:
> again, should these be prefixed with a g? and they shouldn't have a _

Done.

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:82: logging_old_handler_ =
logging::GetLogMessageHandler();
On 2011/07/21 23:37:07, dmac wrote:
> Is this not assuming that HostNPScriptObject is only going to be created once?

No, er.. yes. It's not not assuming...  I mean it *is* assuming.

Anyway, I fixed it up a bit and added a comment.  It doesn't seem worth the dev
effort to make the debug logging work perfectly for multiple Chromoting tabs.

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:491: if (logging_scriptable_object_
&& !logging_to_plugin_) {
On 2011/07/21 23:37:07, dmac wrote:
> accessing logging_to_plugin_ from multiple thread problems?

Pushed this check down into the UIThread code below (in LogDebugInfo).

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

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:70: static bool LogToUI(int severity,
const char* file, int line,
On 2011/07/21 23:37:07, dmac wrote:
> severity is not an enum?

defined in base/logging.h as an int.

http://codereview.chromium.org/7355011/diff/11001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:70: static bool LogToUI(int severity,
const char* file, int line,
On 2011/07/21 23:37:07, dmac wrote:
> comment method please.

Done.

[Sergey Ulanov, 2011-08-02 01:08:32]

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
File remoting/base/task_thread_proxy.h (right):

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
year

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:12: class TaskThreadProxy : public
base::RefCountedThreadSafe<TaskThreadProxy> {
Please add some comments that explain when this class should be used.

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:15: : message_loop_(loop) {
Move implementation to task_thread_proxy.cc, otherwise clang bots will not like
this

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:18: // Detach is called when the message loop
is about to be destroyed.
This comment is not correct. This method should be called when the target of
posted tasks is being destroyed. We don't really care about message loop being
destroyed, as long as the message loop outlives this object.

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:58: static TaskThreadProxy*
g_logging_thread_proxy = NULL;
Don't need this.

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:331: if (g_logging_instance &&
g_logging_thread_proxy) {
don't need to verify g_logging_thread_proxy

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:335: g_logging_thread_proxy->Call(
g_logging_instance->log_proxy->Call(...

[garykac, 2011-08-02 02:13:31]

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
File remoting/base/task_thread_proxy.h (right):

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2011/08/02 01:08:32, sergeyu wrote:
> year

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:12: class TaskThreadProxy : public
base::RefCountedThreadSafe<TaskThreadProxy> {
On 2011/08/02 01:08:32, sergeyu wrote:
> Please add some comments that explain when this class should be used.

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:15: : message_loop_(loop) {
On 2011/08/02 01:08:32, sergeyu wrote:
> Move implementation to task_thread_proxy.cc, otherwise clang bots will not
like
> this

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/base/task_thread_p...
remoting/base/task_thread_proxy.h:18: // Detach is called when the message loop
is about to be destroyed.
On 2011/08/02 01:08:32, sergeyu wrote:
> This comment is not correct. This method should be called when the target of
> posted tasks is being destroyed. We don't really care about message loop being
> destroyed, as long as the message loop outlives this object.

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:58: static TaskThreadProxy*
g_logging_thread_proxy = NULL;
On 2011/08/02 01:08:32, sergeyu wrote:
> Don't need this.

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:331: if (g_logging_instance &&
g_logging_thread_proxy) {
On 2011/08/02 01:08:32, sergeyu wrote:
> don't need to verify g_logging_thread_proxy

Done.

http://codereview.chromium.org/7355011/diff/19001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:335: g_logging_thread_proxy->Call(
On 2011/08/02 01:08:32, sergeyu wrote:
> g_logging_instance->log_proxy->Call(...

Done.

[Sergey Ulanov, 2011-08-02 02:21:56]

http://codereview.chromium.org/7355011/diff/22003/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/7355011/diff/22003/remoting/base/util.cc#newco...
remoting/base/util.cc:18: time_t t = time(NULL);
It's better to use base::Time here.

[garykac, 2011-08-02 18:07:01]

http://codereview.chromium.org/7355011/diff/22003/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/7355011/diff/22003/remoting/base/util.cc#newco...
remoting/base/util.cc:18: time_t t = time(NULL);
On 2011/08/02 02:21:56, sergeyu wrote:
> It's better to use base::Time here.

Done.

[Sergey Ulanov, 2011-08-02 18:15:52]

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

http://codereview.chromium.org/7355011/diff/27001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:539: if (log_debug_info_func_ &&
!g_logging_to_plugin) {
Not sure why we need |g_logging_to_plugin| here. Can you please add comments?
Does it need to be global?

[garykac, 2011-08-02 23:09:50]

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

http://codereview.chromium.org/7355011/diff/27001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:539: if (log_debug_info_func_ &&
!g_logging_to_plugin) {
On 2011/08/02 18:15:52, sergeyu wrote:
> Not sure why we need |g_logging_to_plugin| here. Can you please add comments?
> Does it need to be global?

Added comments and moved it into the static LogToUI. It's global because we have
only 1 global LogMessage callback eve if there are multiple chromoting
instances.

Notes it stands, the code doesn't 100% properly handle multiple plugins (see
comment near top of file). But spending time on that case is not a priority.

[Wez, 2011-08-02 23:45:26]

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

http://codereview.chromium.org/7355011/diff/27001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:110: g_logging_old_handler_ = NULL;
This code isn't symmetric with the constructor; if multiple instances are
created and destroyed then we'll tear down our logging handler as soon as the
first is destroyed, and only reinstate it if a new instance is started.

Given that hooking logging has global effect, it might make more sense to hook
in NPP_Initialize, and un-hook in NPP_Shutdown?

[Wez, 2011-08-04 02:21:11]

On 2011/08/02 23:45:26, Wez wrote:
>
http://codereview.chromium.org/7355011/diff/27001/remoting/host/plugin/host_s...
> File remoting/host/plugin/host_script_object.cc (right):
> 
>
http://codereview.chromium.org/7355011/diff/27001/remoting/host/plugin/host_s...
> remoting/host/plugin/host_script_object.cc:110: g_logging_old_handler_ = NULL;
> This code isn't symmetric with the constructor; if multiple instances are
> created and destroyed then we'll tear down our logging handler as soon as the
> first is destroyed, and only reinstate it if a new instance is started.
> 
> Given that hooking logging has global effect, it might make more sense to hook
> in NPP_Initialize, and un-hook in NPP_Shutdown?

LGTM.  Let's revisit this later; at your discretion whether to add a TODO in
this CL. :)