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

chrome/browser/media/webrtc_event_log_handler.h

 // Copyright 2016 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.
 
 #ifndef CHROME_BROWSER_MEDIA_WEBRTC_EVENT_LOG_HANDLER_H_
 #define CHROME_BROWSER_MEDIA_WEBRTC_EVENT_LOG_HANDLER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
+#include <set>
 #include <string>
 
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 
 namespace content {
 class RenderProcessHost;
 }  // namespace content
 class Profile;
 
 // WebRtcEventLogHandler provides an interface to start and stop
 // the WebRTC event log.
 class WebRtcEventLogHandler
     : public base::RefCountedThreadSafe<WebRtcEventLogHandler> {
  public:
   typedef base::Callback<void(bool, const std::string&)> GenericDoneCallback;
   typedef base::Callback<void(const std::string&)> RecordingErrorCallback;
   typedef base::Callback<void(const std::string&, bool, bool)>
       RecordingDoneCallback;
 
   // Key used to attach the handler to the RenderProcessHost.
   static const char kWebRtcEventLogHandlerKey[];
 
-  explicit WebRtcEventLogHandler(Profile* profile);
+  WebRtcEventLogHandler(int host_id, Profile* profile);
 
   // Starts an RTC event log. The call writes the most recent events to a
   // file and then starts logging events for the given |delay|.
   // If |delay| is zero, the logging will continue until
   // StopWebRtcEventLogging()
   // is explicitly invoked.
   // |callback| is invoked once recording stops. If |delay| is zero
   // |callback| is invoked once recording starts.
   // If a recording was already in progress, |error_callback| is invoked instead
   // of |callback|.
   void StartWebRtcEventLogging(content::RenderProcessHost* host,
                                base::TimeDelta delay,
                                const RecordingDoneCallback& callback,
                                const RecordingErrorCallback& error_callback);
 
+  // Starts an RTC event log for each peerconnection on the specified |host|.

[Henrik Grunell, 2016/05/10 08:44:56]

It's not clear to me how this function relates to the above. Does the above one
also start for each peer connection?

[Ivo-OOO until feb 6, 2016/05/12 13:23:23]

Yes it does, I will add a comment to clarify that.

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

I also don't understand why one can specify either delay+callbacks or base file
path, but not all. Could you explain that in the comments?

+  // A base file_path can be supplied, which will be extended to include several
+  // identifiers to ensure uniqueness. If a recording was already in progress,
+  // this call will be ignored.
+  void StartWebRtcEventLogging(content::RenderProcessHost* host,
+                               const base::FilePath& file_path);
+
   // Stops an RTC event log. |callback| is invoked once recording
   // stops. If no recording was in progress, |error_callback| is invoked instead
   // of |callback|.
   void StopWebRtcEventLogging(content::RenderProcessHost* host,
                               const RecordingDoneCallback& callback,
                               const RecordingErrorCallback& error_callback);
 
+  // Stops an RTC event log for each peerconnection on the specified |host|.
+  // If no recording was in progress, this call has no effect.
+  void StopWebRtcEventLogging(content::RenderProcessHost* host);
+
+  // Signal that a PeerConnection was added to the RenderProcessHost
+  // identified by |render_id|.
+  void OnPeerConnectionAdded(int render_id, int local_id);

[dcheng, 2016/05/10 06:48:17]

Nit: Call this |process_id| to be consistent with terminology elsewhere (and
make it less ambiguous what the ID is for, since we have RenderFrame and
RenderView as well). Same comment below.

Also, what is "local_id"? I'm guessing it's an ID corresponding to the
connection? Maybe connection_id?

[Ivo-OOO until feb 6, 2016/05/12 13:23:23]

Good idea, done. I changed the argument to the constructor to process_id as
well.

+
+  // Signal that a PeerConnection was removed from the RenderProcessHost
+  // identified by |render_id|.
+  void OnPeerConnectionRemoved(int render_id, int local_id);
+
  private:
   friend class base::RefCountedThreadSafe<WebRtcEventLogHandler>;
   virtual ~WebRtcEventLogHandler();
 
   base::FilePath GetLogDirectoryAndEnsureExists();
 
   // Helper for starting RTC event logs.
   void DoStartWebRtcEventLogging(content::RenderProcessHost* host,
                                  base::TimeDelta delay,
                                  const RecordingDoneCallback& callback,
                                  const RecordingErrorCallback& error_callback,
                                  const base::FilePath& log_directory);
 
   // Helper for stopping RTC event logs.
   void DoStopWebRtcEventLogging(content::RenderProcessHost* host,
                                 bool is_manual_stop,
                                 uint64_t audio_debug_recordings_id,
                                 const RecordingDoneCallback& callback,
                                 const RecordingErrorCallback& error_callback,
                                 const base::FilePath& log_directory);
 
   // The profile associated with our renderer process.
   Profile* const profile_;
 
   // Must be accessed on the UI thread.
   bool is_rtc_event_logging_in_progress_;
 
+  // In case new PeerConnections are created during logging, the path is needed
+  // to enable logging for them.
+  base::FilePath base_file_path_;
+
+  // The local identifiers of all the currently active PeerConnections.
+  std::set<int> active_peer_connection_lid_;

[Henrik Grunell, 2016/05/10 08:44:56]

Write out what "l" stands for. (local?)

[Ivo-OOO until feb 6, 2016/05/12 13:23:23]

Done.

+
+  // Number of log files that can be created.
+  int number_log_files_;

[Henrik Grunell, 2016/05/10 08:44:56]

So, this is the max number of files allowed? If so, use a constant in the
implementation file instead of a member variable.

[Ivo-OOO until feb 6, 2016/05/12 13:23:23]

This is the not the maximum, but the number of files that can still be created
before the maximum is reached. It starts at Max, and each time a log file is
started this number is decreased by one. I will clarify the description.

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

I think it's clearer to have this variable count the number of open files
(instead of "remaining"), and compare to the max allowed specified as a constant
in the impl file (which it is now I think).

+
   // This counter allows saving each log in a separate file.
   uint64_t current_rtc_event_log_id_;
 
   base::ThreadChecker thread_checker_;
   DISALLOW_COPY_AND_ASSIGN(WebRtcEventLogHandler);
 };
 
 #endif  // CHROME_BROWSER_MEDIA_WEBRTC_EVENT_LOG_HANDLER_H_