Implement browser-side logging to WebRtc log

chrome/browser/media/webrtc_logging_handler_host.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/webrtc_logging_handler_host.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 185 matching lines @@
                             kLogNotStoppedOrNoLogOpen);
     return;
   }
   g_browser_process->webrtc_log_uploader()->LoggingStoppedDontUpload();
   circular_buffer_.reset();
   log_buffer_.reset();
   logging_state_ = CLOSED;
   FireGenericDoneCallback(&discard_callback, true, "");
 }
 
+void WebRtcLoggingHandlerHost::LogMessage(const std::string& message) {
+  BrowserThread::PostTask(

[Henrik Grunell, 2014/01/22 07:05:24]

I don't think we should log if in STOPPING state, which is allowed in
OnAddLogMessage. It must be allowed from the renderer side though since messages
are buffered there. It's not a big deal however.

And no need to post a task if already on IO thread.

So I suggest post to same function if not on IO thread, and keep
if (logging_state_ == STARTED)
  LogToCircularBuffer(message);

[vrk (LEFT CHROMIUM), 2014/01/23 21:46:32]

Done.
FYI this pattern of

  void foo() {
    // The first time this method is called, we won't be
    // on the correct thread, so repost this same method 
    // to the desired thread. (this pattern is bad!)
    if (current != desired_thread) {
      desired_thread->PostTask(foo);
      return;
    }
    ... continue on desired_thread.
  }

is not a good pattern, as it makes it difficult to reason about threading (and
therefore easy to introduce threading bugs/hard to debug them). This pattern
used to be rampant in the <video>/<audio> code, but there was a big clean-up to
get rid of it. Talk to fischman@ or scherkus@ about it for more info. We should
do the same cleanup in the WebRTC code, and we shouldn't add more of it.

Also, for the same reason, the following is not a good pattern either:

   // Can be called on any thread
   void foo() {
     // If we're coincidentally on the thread we want to 
     // be on, continue without repost. (this pattern is bad!)
     if (current == desired_thread) {
        bar();
     } else {
        desired_thread->Post(bar);
     }
   }

The time saved by this check is not worth the difficulty in reasoning about the
threading story. There are other things we can do in follow up CLs too to make
the threading story here clearer. Can chat about this offline - fischman@ had
some suggestions.

+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&WebRtcLoggingHandlerHost::OnAddLogMessage, this, message));
+}
+
 void WebRtcLoggingHandlerHost::OnChannelClosing() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (logging_state_ == STARTED || logging_state_ == STOPPED) {
     if (upload_log_on_render_close_) {
       logging_state_ = STOPPED;
       TriggerUploadLog();
     } else {
       g_browser_process->webrtc_log_uploader()->LoggingStoppedDontUpload();
     }
   }
@@ skipping 172 matching lines @@
 void WebRtcLoggingHandlerHost::FireGenericDoneCallback(
     GenericDoneCallback* callback, bool success,
     const std::string& error_message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!(*callback).is_null());
   content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
                                    base::Bind(*callback, success,
                                               error_message));
   (*callback).Reset();
 }