Add ongoing events to net-export log when logging starts

net/log/file_net_log_observer.h

Index: net/log/file_net_log_observer.h
diff --git a/net/log/file_net_log_observer.h b/net/log/file_net_log_observer.h
index b7c8a2fac427b551da5fac1b4b9250815f81cadd..063ea5ebf4be6ec9a275de157533be0fea875bf6 100644
--- a/net/log/file_net_log_observer.h
+++ b/net/log/file_net_log_observer.h
@@ -21,7 +21,6 @@ class SingleThreadTaskRunner;
 namespace net {
 
 class NetLogCaptureMode;
-class URLRequestContext;
 
 // FileNetLogObserver watches the NetLog event stream and sends all entries to
 // either a group of files in a directory (bounded mode) or to a single file
@@ -49,16 +48,12 @@ class URLRequestContext;
 // on which thread is used.
 class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
  public:
+  // Constructs a FileNetLogObserver in bounded mode.

[eroman, 2017/02/21 22:14:23]

I generally like the split of file allocation vs start observing, however I am
not enthusiastic about the constructor overloading.

Instead of using overloaded constructors how about either of these patterns
(just a small adjustment from current code):

  (1) Factory method  (my preferred approach)
  (2) Named initialization methods

The difficulty with constructor overloading is it is unclear when reading the
callers whether they are using bounded vs unbounded modes as the parameter all
kind of look the same. Having different symbol names clarifies this as part of
the API contract.

-------
Approach 1 details:
-------

Using the factory method idiom you would split things up into 2 static methods,
and one instance method:

static std::unique_ptr<FileNetLogObserver> CreateBounded(... params for bounded
mode...);
static std::unique_ptr<FileNetLogObserver> CreateUnbounded(... params for
bounded mode...);
void StartObserving(NetLog*, CaptureMode);

Additionally you would make all the constructors (probably just 1 total)
private.

This is conceptually the same as what you have now, except the constructors are
essentially given names (and allocation is handled internally rather than by the
caller).

--------
Approach 2
--------

With this approach instead of factory methods you would have two initialization
methods:

 InitBounded(...);
 InitUnbounded(...);

This makes selection of the mode explicit as they have a different name, however
has issues that the constructor method version does not (there are more illegal
states to consider, such as if the object is never initialized, or initialized
twice).

[wangyix1, 2017/02/22 01:28:18]

Done, I went with factory methods.

+  //
   // |file_task_runner| indicates the task runner that should be used to post
   // tasks from the main thread to the file thread.
-  explicit FileNetLogObserver(
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner);
-
-  ~FileNetLogObserver() override;
-
-  // Starts observing |net_log| in bounded mode and writes output files to
-  // |directory|.
-  // May only be called once in the lifetime of the object.
+  //
+  // |directory| is the directory where the log files will be.
   //
   // |max_total_size| is the approximate limit on the cumulative size of all
   // netlog files.
@@ -69,48 +64,44 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
   // |constants| is an optional legend for decoding constant values used in
   // the log. It should generally be a modified version of GetNetConstants().
   // If not present, the output of GetNetConstants() will be used.
+  FileNetLogObserver(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
+      const base::FilePath& directory,
+      size_t max_total_size,
+      size_t total_num_files,
+      std::unique_ptr<base::Value> constants);
+
+  // Constructs a FileNetLogObserver in unbounded mode.
   //
-  // |url_request_context| is an optional URLRequestContext that will be used
-  // to pre-populate the log with information about in-progress events. If the
-  // context is non-NULL, StartObservingBounded() must be called on the
-  // context's thread.
-  void StartObservingBounded(NetLog* net_log,
-                             NetLogCaptureMode capture_mode,
-                             const base::FilePath& directory,
-                             std::unique_ptr<base::Value> constants,
-                             URLRequestContext* url_request_context,
-                             size_t max_total_size,
-                             size_t total_num_files);
-
-  // Starts observing |net_log| in unbounded mode and writes to the file at
-  // |filepath|.
-  // May only be called once in the lifetime of the object.
+  // |file_task_runner| indicates the task runner that should be used to post
+  // tasks from the main thread to the file thread.
+  //
+  // |log_path| is where the log file will be.
   //
   // |constants| is an optional legend for decoding constant values used in
   // the log. It should generally be a modified version of GetNetConstants().
   // If not present, the output of GetNetConstants() will be used.
-  //
-  // |url_request_context| is an optional URLRequestContext that will be used
-  // to pre-populate the log with information about in-progress events. If the
-  // context is non-NULL, StartObservingUnbounded() must be called on
-  // the context's thread.
-  void StartObservingUnbounded(NetLog* net_log,
-                               NetLogCaptureMode capture_mode,
-                               const base::FilePath& filepath,
-                               std::unique_ptr<base::Value> constants,
-                               URLRequestContext* url_request_context);
+  FileNetLogObserver(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
+      const base::FilePath& log_path,
+      std::unique_ptr<base::Value> constants);
+
+  ~FileNetLogObserver() override;
+
+  // Attach this observer to |net_log| and begin observing events.
+  void StartObserving(NetLog* net_log, NetLogCaptureMode capture_mode);
 
   // Stops observing net_log(). Must be called after StartObservingBounded/

[eroman, 2017/02/21 22:14:23]

(a) StartObservingBounded/ StartObservingUnbounded no longer exist (b) Can you
also mention in the comment that this has the effect of closing the output
file(s) ?

[wangyix1, 2017/02/22 01:28:18]

Done.

   // StartObservingUnbounded. Should be called before destruction of the
   // FileNetLogObserver and the NetLog, or the NetLog files will be deleted when
   // the observer is destroyed.
   //
+  // |polled_data| is an optional argument used to add additional network stack
+  // state to the log.
+  //
   // |callback| will be run on whichever thread StopObserving() was called on
   // once all file writing is complete and the netlog files can be accessed
   // safely.
-  //
-  // |polled_data| is an optional argument used to add additional network stack
-  // state to the log.
   void StopObserving(std::unique_ptr<base::Value> polled_data,
                      const base::Closure& callback);
 
@@ -123,12 +114,7 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
   class BoundedFileWriter;
   class UnboundedFileWriter;
 
-  // Performs tasks common to both StartObservingBounded() and
-  // StartObservingUnbounded().
-  void StartObservingHelper(NetLog* net_log,
-                            NetLogCaptureMode capture_mode,
-                            std::unique_ptr<base::Value> constants,
-                            URLRequestContext* url_request_context);
+  void InitializeFileWriter(std::unique_ptr<base::Value> constants);
 
   scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;