Move the call to enable the WebRTC event log from PeerConnectionFactory to PeerConnection.

chrome/browser/media/webrtc_event_log_handler.cc

Index: chrome/browser/media/webrtc_event_log_handler.cc
diff --git a/chrome/browser/media/webrtc_event_log_handler.cc b/chrome/browser/media/webrtc_event_log_handler.cc
index 0d9baccce4be9ff2de0f7586f9bd9508f533285f..fd8ea942e2b26caae53defd368c94ead96e7f93f 100644
--- a/chrome/browser/media/webrtc_event_log_handler.cc
+++ b/chrome/browser/media/webrtc_event_log_handler.cc
@@ -15,8 +15,11 @@
 #include "chrome/browser/media/webrtc_log_list.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
+#include "content/common/media/peer_connection_tracker_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/webrtc_callback_interface.h"
+#include "ipc/ipc_platform_file.h"
 
 using content::BrowserThread;
 
@@ -27,21 +30,77 @@ const char WebRtcEventLogHandler::kWebRtcEventLogHandlerKey[] =
 namespace {
 
 // Returns a path name to be used as prefix for RTC event log files.
-base::FilePath GetWebRtcEventLogPrefixPath(const base::FilePath& directory,
-                                           uint64_t rtc_event_log_id) {
+base::FilePath GetWebRtcEventLogPrefixPath(const base::FilePath& directory) {
   static const char kWebRtcEventLogFilePrefix[] = "WebRtcEventLog.";
-  return directory.AppendASCII(kWebRtcEventLogFilePrefix +
-                               base::Int64ToString(rtc_event_log_id));
+  return directory.AppendASCII(kWebRtcEventLogFilePrefix);
 }
 
+// Appends the IDs to the RTC event log file name.
+base::FilePath GetWebRtcEventLogPath(const base::FilePath& base_file,
+                                     int render_process_id,
+                                     uint64_t rtc_event_log_id) {
+  return base_file.AddExtension(base::IntToString(render_process_id))
+      .AddExtension(base::Int64ToString(rtc_event_log_id));

[dcheng, 2016/05/13 06:50:11]

Uint64ToString, since the input type is unsigned.

[Ivo-OOO until feb 6, 2016/05/18 16:26:54]

Done.

+}
+
+// Opens a logfile to pass on to the renderer.
+IPC::PlatformFileForTransit CreateFileForProcess(
+    const base::FilePath& base_path,
+    int render_process_id,
+    uint64_t rtc_event_log_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::FilePath file_path =
+      GetWebRtcEventLogPath(base_path, render_process_id, rtc_event_log_id);
+  base::File event_log_file(
+      file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND);
+  if (!event_log_file.IsValid()) {
+    VLOG(1) << "Could not open WebRTC event log file, error="
+            << event_log_file.error_details();
+    return IPC::InvalidPlatformFileForTransit();
+  }
+  return IPC::TakePlatformFileForTransit(std::move(event_log_file));
+}
+
+void SendEventLogFileToRenderer(content::RenderProcessHost* rph,
+                                int local_id,
+                                IPC::PlatformFileForTransit file_for_transit) {
+  if (file_for_transit == IPC::InvalidPlatformFileForTransit()) {
+    return;
+  }
+#if defined(OS_ANDROID)
+  const int64_t max_filesize_bytes = 10000000;
+#else
+  const int64_t max_filesize_bytes = 30000000;

[dcheng, 2016/05/13 06:50:11]

Out of curiosity... why do we need to send this over IPC? Couldn't it just be
hardcoded in the renderer?

[Henrik Grunell, 2016/05/13 09:05:31]

Agree.

[Ivo-OOO until feb 6, 2016/05/18 16:26:54]

Ok, I will move it there, good point.

+#endif
+
+  rph->Send(new PeerConnectionTracker_StartEventLog(local_id, file_for_transit,
+                                                    max_filesize_bytes));
+}
 }  // namespace
 
-WebRtcEventLogHandler::WebRtcEventLogHandler(Profile* profile)
+WebRtcEventLogHandler::WebRtcEventLogHandler(int process_id, Profile* profile)
     : profile_(profile),
       is_rtc_event_logging_in_progress_(false),
+      number_log_files_(0),
       current_rtc_event_log_id_(0) {
   DCHECK(profile_);
   thread_checker_.DetachFromThread();
+  content::RenderProcessHost* host =
+      content::RenderProcessHost::FromID(process_id);
+  using StartEventLogFn = void (WebRtcEventLogHandler::*)(
+      content::RenderProcessHost* host, const base::FilePath& file_path);

[dcheng, 2016/05/13 06:50:11]

I think it's probably shorter (and clearer) to just write:

content::WebRTCCallbackInterface::GetInstance()->RegisterEventLogHandler(
    process_id,
    base::Bind(&WebRtcEventLogHandler::StartWebRtcEventLogging, this, host),
    base::Bind(&WebRtcEventLogHandler::StopWebRtcEventLogging, this, host)));

[Ivo-OOO until feb 6, 2016/05/18 16:26:54]

You're right that it's both shorter and clearer, however it doesn't work in this
case. I think it's because there are two functions with the same name, and the
compiler cannot figure out which one to use. Changing the name of the start/stop
functions would probably fix this, but I think function names are more
important.

+  using StopEventLogFn =
+      void (WebRtcEventLogHandler::*)(content::RenderProcessHost* host);
+  StartEventLogFn start_fn = &WebRtcEventLogHandler::StartWebRtcEventLogging;
+  StopEventLogFn stop_fn = &WebRtcEventLogHandler::StopWebRtcEventLogging;
+  content::WebRTCCallbackInterface::GetInstance()->RegisterEventLogHandler(
+      process_id, base::Bind(start_fn, this, host),
+      base::Bind(stop_fn, this, host));
+  content::WebRTCCallbackInterface::GetInstance()
+      ->RegisterPeerConnectionCallbacks(
+          process_id,
+          base::Bind(&WebRtcEventLogHandler::OnPeerConnectionAdded, this),
+          base::Bind(&WebRtcEventLogHandler::OnPeerConnectionRemoved, this));
 }
 
 WebRtcEventLogHandler::~WebRtcEventLogHandler() {}
@@ -60,6 +119,28 @@ void WebRtcEventLogHandler::StartWebRtcEventLogging(
                  delay, callback, error_callback));
 }
 
+void WebRtcEventLogHandler::StartWebRtcEventLogging(
+    content::RenderProcessHost* host,
+    const base::FilePath& file_path) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  is_rtc_event_logging_in_progress_ = true;

[dcheng, 2016/05/13 06:50:11]

This is triggered by a renderer IPC, right? What if a renderer sends this IPC
multiple times? Seems like it might let the renderer exceed the
number_log_files_ limit.

[Ivo-OOO until feb 6, 2016/05/18 16:26:54]

This is not triggered by renderer IPC (it is called from WebRTCInternals via
WebRTCCallbackInterface in the browser process), but you're right that calling
multiple times could cause problems, so I'll add some code to prevent that,
thanks.

+  base_file_path_ = file_path;
+#if defined(OS_ANDROID)
+  number_log_files_ = 3;

[Henrik Grunell, 2016/05/13 09:05:31]

Are these number based on something particular?

[Ivo-OOO until feb 6, 2016/05/18 16:26:54]

Not really, just on what seems like a reasonably upper limit for most situations
and on the intuition that desktops/laptops are faster than mobile phones. 

+#else
+  number_log_files_ = 10;
+#endif
+  for (int local_id : active_peer_connection_local_id_) {
+    if (number_log_files_ > 0) {
+      BrowserThread::PostTaskAndReplyWithResult(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&CreateFileForProcess, file_path, host->GetID(), local_id),
+          base::Bind(&SendEventLogFileToRenderer, host, local_id));
+      number_log_files_--;
+    }
+  }
+}
+
 void WebRtcEventLogHandler::StopWebRtcEventLogging(
     content::RenderProcessHost* host,
     const RecordingDoneCallback& callback,
@@ -74,6 +155,36 @@ void WebRtcEventLogHandler::StopWebRtcEventLogging(
                  error_callback));
 }
 
+void WebRtcEventLogHandler::StopWebRtcEventLogging(
+    content::RenderProcessHost* host) {
+  is_rtc_event_logging_in_progress_ = false;
+  for (int local_id : active_peer_connection_local_id_) {
+    host->Send(new PeerConnectionTracker_StopEventLog(local_id));
+  }
+}
+
+void WebRtcEventLogHandler::OnPeerConnectionAdded(int process_id,
+                                                  int connection_id) {
+  active_peer_connection_local_id_.insert(connection_id);
+  if (is_rtc_event_logging_in_progress_ && number_log_files_ > 0) {
+    content::RenderProcessHost* host =
+        content::RenderProcessHost::FromID(process_id);
+    if (host) {
+      BrowserThread::PostTaskAndReplyWithResult(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&CreateFileForProcess, base_file_path_, process_id,
+                     connection_id),
+          base::Bind(&SendEventLogFileToRenderer, host, connection_id));
+      number_log_files_--;
+    }
+  }
+}
+
+void WebRtcEventLogHandler::OnPeerConnectionRemoved(int process_id,
+                                                    int connection_id) {
+  active_peer_connection_local_id_.erase(connection_id);
+}
+
 base::FilePath WebRtcEventLogHandler::GetLogDirectoryAndEnsureExists() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   base::FilePath log_dir_path =
@@ -98,11 +209,8 @@ void WebRtcEventLogHandler::DoStartWebRtcEventLogging(
     error_callback.Run("RTC event logging already in progress");
     return;
   }
-
-  is_rtc_event_logging_in_progress_ = true;
-  base::FilePath prefix_path =
-      GetWebRtcEventLogPrefixPath(log_directory, ++current_rtc_event_log_id_);
-  host->EnableEventLogRecordings(prefix_path);
+  base::FilePath prefix_path = GetWebRtcEventLogPrefixPath(log_directory);
+  StartWebRtcEventLogging(host, prefix_path);
 
   if (delay.is_zero()) {
     const bool is_stopped = false, is_manual_stop = false;
@@ -129,8 +237,9 @@ void WebRtcEventLogHandler::DoStopWebRtcEventLogging(
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_LE(rtc_event_log_id, current_rtc_event_log_id_);
 
-  base::FilePath prefix_path =
-      GetWebRtcEventLogPrefixPath(log_directory, rtc_event_log_id);
+  base::FilePath prefix_path = GetWebRtcEventLogPrefixPath(log_directory);
+  base::FilePath file_path =
+      GetWebRtcEventLogPath(prefix_path, host->GetID(), rtc_event_log_id);
   // Prevent an old posted DoStopWebRtcEventLogging() call to stop a newer dump.
   // This could happen in a sequence like:
   //   Start(10);  // Start dump 1. Post Stop() to run after 10 seconds.
@@ -138,7 +247,7 @@ void WebRtcEventLogHandler::DoStopWebRtcEventLogging(
   //   Start(20);  // Start dump 2. Posted Stop() for 1 should not stop dump 2.
   if (rtc_event_log_id < current_rtc_event_log_id_) {
     const bool is_stopped = false;
-    callback.Run(prefix_path.AsUTF8Unsafe(), is_stopped, is_manual_stop);
+    callback.Run(file_path.AsUTF8Unsafe(), is_stopped, is_manual_stop);
     return;
   }
 
@@ -147,8 +256,7 @@ void WebRtcEventLogHandler::DoStopWebRtcEventLogging(
     return;
   }
 
-  host->DisableEventLogRecordings();
-  is_rtc_event_logging_in_progress_ = false;
+  StopWebRtcEventLogging(host);
   const bool is_stopped = true;
-  callback.Run(prefix_path.AsUTF8Unsafe(), is_stopped, is_manual_stop);
+  callback.Run(file_path.AsUTF8Unsafe(), is_stopped, is_manual_stop);
 }