Convert Chromoting plugin logging to appear in client UI

Convert Chromoting logging to appear in client debug-info div.

BUG=none
TEST=none

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

[Sergey Ulanov, 2011-03-04 22:24:45]

I added copule new messages in my last CL. Can you please convert those too?

[garykac, 2011-03-04 22:27:02]

will do

On Fri, Mar 4, 2011 at 2:24 PM, <sergeyu@chromium.org> wrote:

> I added copule new messages in my last CL. Can you please convert those
> too?
>
> http://codereview.chromium.org/6621018/
>

[Sergey Ulanov, 2011-03-04 22:28:45]

LGTM one more nit.

http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_instance.h:80: void LogDebugWarning(const
std::string& info);
Looks like these three methods can be replaced with one, with LogSeverity
parameter.

[dmac, 2011-03-05 00:52:01]

http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_instance.h:80: void LogDebugWarning(const
std::string& info);
On 2011/03/04 22:28:45, sergeyu wrote:
> Looks like these three methods can be replaced with one, with LogSeverity
> parameter.

and can we make them void LogDebug(Severity, format, ...) this will fix up all
the places where you are doing StringPrintf

[dmac, 2011-04-05 18:34:43]

On 2011/03/05 00:52:01, dmac wrote:
>
http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
> File remoting/client/plugin/chromoting_instance.h (right):
> 
>
http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
> remoting/client/plugin/chromoting_instance.h:80: void LogDebugWarning(const
> std::string& info);
> On 2011/03/04 22:28:45, sergeyu wrote:
> > Looks like these three methods can be replaced with one, with LogSeverity
> > parameter.
> 
> and can we make them void LogDebug(Severity, format, ...) this will fix up all
> the places where you are doing StringPrintf

Anything happening with this?

[dmac, 2011-04-20 23:34:35]

On 2011/04/05 18:34:43, dmac wrote:
> On 2011/03/05 00:52:01, dmac wrote:
> >
>
http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
> > File remoting/client/plugin/chromoting_instance.h (right):
> > 
> >
>
http://codereview.chromium.org/6621018/diff/1/remoting/client/plugin/chromoti...
> > remoting/client/plugin/chromoting_instance.h:80: void LogDebugWarning(const
> > std::string& info);
> > On 2011/03/04 22:28:45, sergeyu wrote:
> > > Looks like these three methods can be replaced with one, with LogSeverity
> > > parameter.
> > 
> > and can we make them void LogDebug(Severity, format, ...) this will fix up
all
> > the places where you are doing StringPrintf
> 
> Anything happening with this?

Gary, can you clean this issue up? Either submit, or delete?

[garykac, 2011-05-13 23:25:12]

OK. I have:

* revived this CL (my machine died 1.5 months ago - so I needed to pull this
from the old HD and bring it up to date)
* applied Sergey's suggestions (LogSeverity)
* applied DMac's suggestions (stdargs)
* added VLog support
* fixed the threading crashes

<whew>!

[dmac, 2011-05-13 23:34:10]

ran out of time to finish review. I'll look at it later again, but thought I'd
send my initial thoughts.

http://codereview.chromium.org/6621018/diff/11001/remoting/client/client_logg...
File remoting/client/client_logger.h (right):

http://codereview.chromium.org/6621018/diff/11001/remoting/client/client_logg...
remoting/client/client_logger.h:8: #include <string>
we do you need string?

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

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:53: logger_(new
PepperClientLogger(this)) {
does this need to be a ptr?

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:295:
DCHECK(CurrentlyOnPluginThread());
do you want to do the thread check at the end or beginning of the method?

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:303:
DCHECK(CurrentlyOnPluginThread());
same comment as above

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

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:123: ClientLogger* logger_;
does this need to be a scoped_ptr? it looks like it owns logger_... or could it
just be a PepperClientLogger here (not a ptr).

[garykac, 2011-05-14 01:09:16]

http://codereview.chromium.org/6621018/diff/11001/remoting/client/client_logg...
File remoting/client/client_logger.h (right):

http://codereview.chromium.org/6621018/diff/11001/remoting/client/client_logg...
remoting/client/client_logger.h:8: #include <string>
On 2011/05/13 23:34:10, dmac wrote:
> we do you need string?

Leftover from old version. Removed.

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

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:53: logger_(new
PepperClientLogger(this)) {
On 2011/05/13 23:34:10, dmac wrote:
> does this need to be a ptr?

Done.

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:295:
DCHECK(CurrentlyOnPluginThread());
On 2011/05/13 23:34:10, dmac wrote:
> do you want to do the thread check at the end or beginning of the method?

Oops. That's leftover from when I was debugging the thread issue (I wanted the
message displayed before the DCHECK).

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:303:
DCHECK(CurrentlyOnPluginThread());
On 2011/05/13 23:34:10, dmac wrote:
> same comment as above

Done.

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

http://codereview.chromium.org/6621018/diff/11001/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:123: ClientLogger* logger_;
On 2011/05/13 23:34:10, dmac wrote:
> does this need to be a scoped_ptr? it looks like it owns logger_... or could
it
> just be a PepperClientLogger here (not a ptr).

Done.

[garykac, 2011-05-18 23:46:38]

ping!

[Sergey Ulanov, 2011-05-18 23:56:26]

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.h:89: void Log(int severity, const
char* format, ...);
Do we need this methods? Maybe it's better to add logger() that would return
pointer to |logger_|?

[garykac, 2011-05-19 21:14:36]

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.h:89: void Log(int severity, const
char* format, ...);
On 2011/05/18 23:56:26, sergeyu wrote:
> Do we need this methods? Maybe it's better to add logger() that would return
> pointer to |logger_|?

I originally had that, but found that:

   instance_->getLogger()->Log(...)

was a lot more irritating to deal with than simply:

   instance_->Log(...)

We type these log lines fairly frequently in the code so the convenience was
nice.

[Sergey Ulanov, 2011-05-20 00:58:19]

Just two nits for the copyright headers. LGTM otherwise.

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_client_logger.cc (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_client_logger.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_client_logger.h (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_client_logger.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011

[garykac, 2011-05-20 22:33:16]

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_client_logger.cc (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_client_logger.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/05/20 00:58:19, sergeyu wrote:
> 2011

Done.

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_client_logger.h (right):

http://codereview.chromium.org/6621018/diff/9009/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_client_logger.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/05/20 00:58:19, sergeyu wrote:
> 2011

Done.

[commit-bot: I haz the power, 2011-05-20 22:54:00]

Try job failure for 6621018-19001 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-23 18:22:18]

Change committed as 86304