Move net-export thread-hopping code into NetLogFileWriter and add IO polled data.

components/net_log/net_log_file_writer.h

Index: components/net_log/net_log_file_writer.h
diff --git a/components/net_log/net_log_file_writer.h b/components/net_log/net_log_file_writer.h
index 17033e624b8aecf4f56e1f3c96b540875943445b..ea4976a920ccfd3bc78310530ba7e9d122bca31b 100644
--- a/components/net_log/net_log_file_writer.h
+++ b/components/net_log/net_log_file_writer.h
@@ -8,19 +8,25 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "net/log/net_log_capture_mode.h"
 
 namespace base {
 class DictionaryValue;
+class SingleThreadTaskRunner;
+class Value;
 }
 
 namespace net {
-class NetLogCaptureMode;
-class WriteToFileNetLogObserver;
+class FileNetLogObserver;
+class URLRequestContextGetter;
 }
 
 namespace net_log {
@@ -30,45 +36,57 @@ class ChromeNetLog;
 // NetLogFileWriter logs all the NetLog entries into a specified file.

[eroman, 2017/01/06 21:46:23]

Please add a description for the threading model:

  * This class is created and destroyed on the UI thread, and all public entry
points are to be called on the UI thread

  * However internally the class may run some code on the file_task_runner_ and
net_task_runner_.

[wangyix1, 2017/01/10 22:20:15]

Done.

 //
 // NetLogFileWriter maintains the current logging state (state_) and log file
-// type (log_type_) of the logging into a chrome-net-export-log.json file.
+// state (log_exists_, log_capture_mode_known_, and log_capture_mode_).
 //
-// The following are the possible states
-// a) Only Start is allowed (STATE_NOT_LOGGING, LOG_TYPE_NONE).
+// The following are the possible states after initialization:
+// a) Only Start is allowed (STATE_NOT_LOGGING, log_exists_ is false).
 // b) Only Stop is allowed (STATE_LOGGING).
-// c) Either Send or Start is allowed (STATE_NOT_LOGGING, anything but
-//    LOG_TYPE_NONE).
-//
-// This is created/destroyed on the main thread, but all other function calls
-// occur on a background thread.
-//
-// This relies on the UI thread outlasting all other threads for thread safety.
-class NetLogFileWriter {
+// c) Either Send or Start is allowed (STATE_NOT_LOGGING, log_exists_ is true).
+class NetLogFileWriter : public base::SupportsWeakPtr<NetLogFileWriter> {

[eroman, 2017/01/06 21:46:23]

Why is SupportsWeakPtr<> needed?  (the weak_ptr_factory_ member added should be
sufficient).

[wangyix1, 2017/01/10 22:20:16]

Done.

  public:
-  // This enum lists the UI button commands it could receive.
-  enum Command {
-    DO_START_LOG_BYTES,  // Call StartNetLog logging all bytes received.
-    DO_START,            // Call StartNetLog.
-    DO_START_STRIP_PRIVATE_DATA,  // Call StartNetLog stripping private data.
-    DO_STOP,                      // Call StopNetLog.
-  };
 
   virtual ~NetLogFileWriter();

[eroman, 2017/01/06 21:46:23]

Does this class still need a vtable? The only virtual method
(GetNetExportLogBaseDirectory()) has been removed. How are the unit-tests
impacted by this change?

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-  // Accepts the button command and executes it.
-  void ProcessCommand(Command command);
+  typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)>

[eroman, 2017/01/06 21:46:23]

For new code we prefer "using" over typedef.

[wangyix1, 2017/01/10 22:20:16]

Done.

+      StateCallback;
 
-  // Returns true and the path to the file. If there is no file to
-  // send, then it returns false. It also returns false when actively logging to
-  // the file.
-  bool GetFilePath(base::FilePath* path);
+  // Start collecting NetLog data into chrome-net-export-log.json file in

[eroman, 2017/01/06 21:46:23]

Can you update this to read "Starts" rather than "Start" ? (I realize this is
just moving an existing comment).

[wangyix1, 2017/01/10 22:20:15]

Done.

+  // a directory, using the specified capture mode. It is a no-op if we are
+  // already collecting data into a file, and |capture_mode| is ignored.
+  // ignored.
+  // TODO(mmenke):  That's rather weird behavior, think about improving it.
+  void StartNetLog(net::NetLogCaptureMode capture_mode,
+                   StateCallback state_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-  // Creates a Value summary of the state of the NetLogFileWriter. The caller is
-  // responsible for deleting the returned value.
-  base::DictionaryValue* GetState();
+  // Stop collecting NetLog data into the file. It is a no-op if we
+  // are not collecting data into a file.
+  void StopNetLog(std::unique_ptr<base::DictionaryValue> polled_data,
+                  scoped_refptr<net::URLRequestContextGetter> context_getter,
+                  StateCallback state_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

+
+  // Creates a Value summary of the state of the NetLogFileWriter and calls
+  // |state_callback| with the state as the param.
+  void GetState(StateCallback state_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

+
+  typedef base::Callback<void(const base::FilePath&)> FilePathCallback;

[eroman, 2017/01/06 21:46:23]

For new code we prefer "using" over typedef.

[wangyix1, 2017/01/10 22:20:15]

Done.

+
+  // Gets the log filepath; if it's valid and the file writer is not actively
+  // logging, |success_callback| is called with the log filepath as the param.

[eroman, 2017/01/06 21:46:23]

Please update the comment to describer what happens in the other case (i.e. does
it call back with empty FilePath(), or not callback at all).

[wangyix1, 2017/01/10 22:20:15]

Done.

+  void GetFilePath(FilePathCallback success_callback);
 
   // Updates |log_path_| to the |custom_path|.
   void SetUpNetExportLogPath(const base::FilePath& custom_path);
 
+  void SetFileTaskRunner(

[eroman, 2017/01/06 21:46:23]

Please add some explanation for this mechanism (either here, or in the top-level
comments for the class).

[wangyix1, 2017/01/10 22:20:15]

Done.

+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner);
+
+  void SetNetTaskRunner(
+      scoped_refptr<base::SingleThreadTaskRunner> net_task_runner);
+
+  scoped_refptr<base::SingleThreadTaskRunner> GetFileTaskRunner() const;
+
+  scoped_refptr<base::SingleThreadTaskRunner> GetNetTaskRunner() const;
+
  protected:
   // Constructs a NetLogFileWriter. Only one instance is created in browser
   // process.
@@ -76,10 +94,6 @@ class NetLogFileWriter {
                    const base::CommandLine::StringType& command_line_string,
                    const std::string& channel_string);
 
-  // Returns path name to base::GetTempDir() directory. Returns false if
-  // base::GetTempDir() fails.
-  virtual bool GetNetExportLogBaseDirectory(base::FilePath* path) const;
-
  private:
   friend class ChromeNetLog;
   friend class NetLogFileWriterTest;
@@ -103,64 +117,73 @@ class NetLogFileWriter {
     STATE_LOGGING,
   };
 
-  // The type of the current log file on disk.
-  enum LogType {
-    // There is no current log file.
-    LOG_TYPE_NONE,
-    // The file predates this session. May or may not have private data.
-    // TODO(davidben): This state is kind of silly.
-    LOG_TYPE_UNKNOWN,
-    // The log includes raw bytes.
-    LOG_TYPE_LOG_BYTES,
-    // The file includes all data.
-    LOG_TYPE_NORMAL,
-    // The file has credentials and cookies stripped.
-    LOG_TYPE_STRIP_PRIVATE_DATA,
+  // Performs file-related initialization tasks on the file thread, and runs
+  // |success_callback| if the initialization tasks succeeded.
+  void InitAndCallback(base::Closure success_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

+
+  // Struct used to store the results of SetUpDefaultLogPath.  Will be passed to
+  // InitStateAndCallback()
+  struct DefaultLogPathResults {
+    bool default_log_path_success;
+    base::FilePath default_log_path;
+    bool log_exists;
   };
 
-  // Returns the NetLog::CaptureMode corresponding to a LogType.
-  static net::NetLogCaptureMode GetCaptureModeForLogType(LogType log_type);
+  // File-related initialization tasks
+  static DefaultLogPathResults SetUpDefaultLogPath();
 
-  // Initializes the |state_| to STATE_NOT_LOGGING and |log_type_| to
-  // LOG_TYPE_NONE (if there is no file from earlier run) or
-  // LOG_TYPE_UNKNOWN (if there is a file from earlier run). Returns
-  // false if initialization of |log_path_| fails.
-  bool EnsureInit();
+  // Initializes |state_| and |log_path_| using results from
+  // SetUpDefaultLogPath(), and runs |success_callback| if
+  // |default_log_path_success| is true.
+  void InitStateAndCallback(base::Closure success_callback,

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

+                            const DefaultLogPathResults& results);
 
-  // Start collecting NetLog data into chrome-net-export-log.json file in
-  // a directory, using the specified capture mode. It is a no-op if we are
-  // already collecting data into a file, and |capture_mode| is ignored.
-  // ignored.
-  // TODO(mmenke):  That's rather weird behavior, think about improving it.
-  void StartNetLog(LogType log_type);
+  // Only called during state transition from STATE_NOT_LOGGING to STATE_LOGGING
+  void StartLogging(net::NetLogCaptureMode capture_mode,
+                    StateCallback state_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-  // Stop collecting NetLog data into the file. It is a no-op if we
-  // are not collecting data into a file.
-  void StopNetLog();
+  // Only called during state transition from STATE_LOGGING to STATE_NOT_LOGGING
+  static std::unique_ptr<base::DictionaryValue> AddNetInfo(
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      std::unique_ptr<base::DictionaryValue> polled_data);
+
+  // Only called during state transition from STATE_LOGGING to STATE_NOT_LOGGING
+  void StopLogging(StateCallback state_callback,

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

+                   std::unique_ptr<base::DictionaryValue> polled_data);
 
-  // Updates |log_path_| to be the base::FilePath to use for log files, which
-  // will be inside the base::GetTempDir() directory. Returns false if
-  // base::GetTempDir() fails, or unable to create a subdirectory for logging
-  // within that directory.
-  bool SetUpDefaultNetExportLogPath();
+  // Creates a Value summary of the state of the NetLogFileWriter. The caller is
+  // responsible for deleting the returned value.

[eroman, 2017/01/06 21:46:23]

Can this return an std::unique_ptr<> instead?

[wangyix1, 2017/01/10 22:20:15]

Done.

+  base::DictionaryValue* GetState();
 
-  // Returns true if a file exists at |log_path_|.
-  bool NetExportLogExists() const;
+  // Gets the state and runs state_callback with the state as a param.
+  void RunStateCallback(StateCallback state_callback);

[eroman, 2017/01/06 21:46:23]

Can callback can be a const-ref?

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-  base::ThreadChecker thread_checker_;
+  // If running on a POSIX OS, this sets all the permission flags of the file at
+  // |path| to 1. Returns false if file does not exist.
+  static bool SetPosixFilePermissionsAll(const base::FilePath& path);
+
+  static void RunFilePathCallback(FilePathCallback path_callback,
+                                  const base::FilePath& path, bool runCallback);
 
   // Helper function for unit tests.
   State state() const { return state_; }
-  LogType log_type() const { return log_type_; }
+
+  base::ThreadChecker thread_checker_;
+
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;

[eroman, 2017/01/06 21:46:23]

Please add a comment for the data members below explaining what is accessed from
which thread.

I believe everything is to be accessed solely from the UI thread (i.e. what
thread_checker_ is bound to).
And file_task_runner_ is just used for shuttling data around for the disk IO.

[wangyix1, 2017/01/10 22:20:15]

Done.

+  scoped_refptr<base::SingleThreadTaskRunner> net_task_runner_;
 
   State state_;       // Current state of NetLogFileWriter.
-  LogType log_type_;  // Type of current log file on disk.
+
+  bool log_exists_;   // Whether or not the log exists on disk
+  bool log_capture_mode_known_;
+  net::NetLogCaptureMode log_capture_mode_;
 
   base::FilePath log_path_;  // base::FilePath to the NetLog file.
 
   // |write_to_file_observer_| watches the NetLog event stream, and
   // sends all entries to the file created in StartNetLog().
-  std::unique_ptr<net::WriteToFileNetLogObserver> write_to_file_observer_;
+  std::unique_ptr<net::FileNetLogObserver> write_to_file_observer_;
 
   // The |chrome_net_log_| is owned by the browser process, cached here to avoid
   // using global (g_browser_process).
@@ -169,6 +192,8 @@ class NetLogFileWriter {
   const base::CommandLine::StringType command_line_string_;
   const std::string channel_string_;
 
+  base::WeakPtrFactory<NetLogFileWriter> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetLogFileWriter);
 };