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..c9ba8c2520183f772ac3690ccf3f31676dc301a1 100644
--- a/components/net_log/net_log_file_writer.h
+++ b/components/net_log/net_log_file_writer.h
@@ -8,66 +8,135 @@
 #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 {
 
 class ChromeNetLog;
 
-// NetLogFileWriter logs all the NetLog entries into a specified file.
+// NetLogFileWriter is used exclusively as a support class for net-export.
+// It's a singleton that acts as the interface to all NetExportMessageHandlers
+// which can tell it to start or stop logging in response to the net-export UIs'

[eroman, 2017/01/18 21:32:43]

UIs' --> UI's

[wangyix1, 2017/01/19 23:09:46]

Done.

+// user actions. Because it's a singleton, the logging state can be shared
+// between multiple instances of the net-export UI. Internally, it manages an

[eroman, 2017/01/18 21:32:43]

Good comment

[wangyix1, 2017/01/19 23:09:46]

Acknowledged.

+// instance of net::FileNetLogObserver and handles the attaching/detaching of it
+// to the ChromeNetLog. This class is used by the iOS and non-iOS
+// implementations of net-export.
 //
-// NetLogFileWriter maintains the current logging state (state_) and log file
-// type (log_type_) of the logging into a chrome-net-export-log.json file.
+// NetLogFileWriter maintains the current logging state (using the members
+// (|state_|, |log_exists_|, |log_capture_mode_known_|, |log_capture_mode_|).
+// Its three main commands are StartNetLog(), StopNetLog() and GetState(). These
+// are the only functions that may cause NetLogFileWriter to change state.
+// Also, NetLogFileWriter is lazily initialized. A portion of the initialization
+// needs to run on the |file_task_runner_|.
 //
-// The following are the possible states
-// a) Only Start is allowed (STATE_NOT_LOGGING, LOG_TYPE_NONE).
-// 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.
+// This class is created and destroyed on the UI thread, and all public entry
+// points are to be called on the UI thread. Internally, the class may run some
+// code on the |file_task_runner_| and |net_task_runner_|.
 class NetLogFileWriter {
  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.
-  };
+  // The three main commands StartNetLog(), StopNetLog(), and GetState() and the
+  // getter GetFilePathToCompletedLog() all accept a callback param which is
+  // used to notify the caller of the results of that function. For all these
+  // commands, the callback will always be executed even if the command ends up
+  // being a no-op or if some failure occurs.
 
-  virtual ~NetLogFileWriter();
+  using StateCallback =
+      base::Callback<void(std::unique_ptr<base::DictionaryValue>)>;
 
-  // Accepts the button command and executes it.
-  void ProcessCommand(Command command);
+  using FilePathCallback = base::Callback<void(const base::FilePath&)>;
 
-  // 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);
+  using DirectoryGetter = base::Callback<bool(base::FilePath*)>;
 
-  // Creates a Value summary of the state of the NetLogFileWriter. The caller is
-  // responsible for deleting the returned value.
-  base::DictionaryValue* GetState();
+  ~NetLogFileWriter();
 
-  // Updates |log_path_| to the |custom_path|.
-  void SetUpNetExportLogPath(const base::FilePath& custom_path);
+  // Starts collecting NetLog data into the file at |log_path|. If |log_path| is
+  // empty, the default log path is used. It is a no-op if NetLogFileWriter is
+  // already collecting data into a file, and |capture_mode| is ignored.
+  // ignored.
+  // TODO(mmenke): That's rather weird behavior, think about improving it.
+  //
+  // If NetLogFileWriter is not initialized, StartNetLog() will trigger
+  // initialization.
+  //
+  // |state_callback| will be executed at the end of StartNetLog()
+  // asynchronously.
+  void StartNetLog(const base::FilePath& log_path,
+                   net::NetLogCaptureMode capture_mode,
+                   const StateCallback& state_callback);
+
+  // Stops collecting NetLog data into the file. It is a no-op if
+  // NetLogFileWriter is currently not logging.
+  //
+  // |polled_data| is a JSON dictionary that will be appended to the end of the
+  // log; it's for adding additional info to the log that aren't events.
+  // If |context_getter| is not null, then  StopNetLog() will automatically
+  // append net info (from net::GetNetInfo() retrieved using |context_getter|)
+  // to |polled_data|.
+  //
+  // |state_callback| will be executed at the end of StopNetLog()
+  // asynchronously.
+  void StopNetLog(std::unique_ptr<base::DictionaryValue> polled_data,
+                  scoped_refptr<net::URLRequestContextGetter> context_getter,
+                  const StateCallback& state_callback);
+
+  // Creates a Value summary of the state of the NetLogFileWriter and calls
+  // |state_callback| with that Value as the param.
+  //
+  // If NetLogFileWriter is not initialized, GetState() will trigger
+  // initialization.
+  //
+  // |state_callback| will be executed at the end of GetState() asynchronously.
+  void GetState(const StateCallback& state_callback);
+
+  // Gets the log filepath. |path_callback| will be used to notify the caller
+  // when the filepath is retrieved. |path_callback| will be executed with an
+  // empty filepath if any of the following occurs:
+  // (1) The NetLogFileWriter is not initialized.
+  // (2) The log file does not exist.
+  // (3) The NetLogFileWriter is currently logging.
+  // (4) The log file's permissions could not be set to all.
+  //
+  // |path_callback| will be executed at the end of GetFilePathToCompletedLog()
+  // asynchronously.
+  void GetFilePathToCompletedLog(const FilePathCallback& path_callback) const;
+
+  // Sets the task runners used by NetLogFileWriter for doing file I/O and
+  // network I/O respectively. This must be called prior to using the
+  // NetLogFileWriter. The task runners must not be changed once set. However,
+  // calling this function again with the same parameters is OK.
+  void SetTaskRunners(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> net_task_runner);
+
+  // Converts to/from the string representation of a capture mode used by
+  // net_export.js.
+  static std::string CaptureModeToString(net::NetLogCaptureMode capture_mode);
+  static net::NetLogCaptureMode CaptureModeFromString(
+      const std::string& capture_mode_string);
+
+  // Overrides the getter used to retrieve the default log base directory during
+  // initialization. Should only be used by unit tests.
+  void SetDefaultLogBaseDirectoryGetter(const DirectoryGetter& getter);

[eroman, 2017/01/18 21:32:43]

For functions that are exclusively exposed for unit-tests, please name them with
a suffix of "ForTest()" or "ForTesting()".

This hilights expectations to future code reviewers if someone tries to use the
function in non-test code. There is also some automatic support for detecting
this via our PRESUBMIT checks. See
https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=ForTest+f:.py&sq=package:...

[wangyix1, 2017/01/19 23:09:46]

Done.

 
  protected:
   // Constructs a NetLogFileWriter. Only one instance is created in browser
@@ -76,91 +145,97 @@ 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;
 
-  // Allow tests to access our innards for testing purposes.
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, EnsureInitFailure);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, EnsureInitAllowStart);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, EnsureInitAllowStartOrSend);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, ProcessCommandDoStartAndStop);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, DoStartClearsFile);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, CheckAddEvent);
-  FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, CheckAddEventWithCustomPath);
-
-  // This enum lists the possible state NetLogFileWriter could be in. It is used
-  // to enable/disable "Start", "Stop" and "Send" (email) UI actions.
+  // The possible logging states of NetLogFileWriter.
   enum State {
     STATE_UNINITIALIZED,
+    // Currently in the process of initializing.
+    STATE_INITIALIZING,
     // Not currently logging to file.
     STATE_NOT_LOGGING,
     // Currently logging to file.
     STATE_LOGGING,
+    // Currently in the process of stopping the log.
+    STATE_STOPPING_LOG,
   };
 
-  // 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,
+  // Struct used to store the results of SetUpDefaultLogPath() which will be
+  // passed to InitStateThenCallback().
+  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);
+  // Helper function used by commands that may trigger initialization
+  // (StartNetLog(), GetState()). This function attempts to initialize
+  // NetLogFileWriter asynchronously, then executes |callback|.
+  void InitThenCallback(const base::Closure& callback);
 
-  // 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();
+  // Contains file-related initialization tasks. Will run on the file task
+  // runner.
+  static DefaultLogPathResults SetUpDefaultLogPath(

[eroman, 2017/01/18 21:32:43]

Can this be part of the .cc file instead of a static method in the .h ?

[wangyix1, 2017/01/19 23:09:46]

This would require exposing the struct NetLogFileWriter::DefaultLogPathResults
as public, which I don't think is appropriate. Also, I think the method makes
sense as a static method in the class since it's a helper that's highly specific
to NetLogFileWriter.

+      const DirectoryGetter& default_log_base_directory_getter);
 
-  // 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);
+  // Initializes |state_| and |log_path_| using results from
+  // SetUpDefaultLogPath(), then executes |callback| synchronously.
+  void InitStateThenCallback(
+      const base::Closure& callback,
+      const DefaultLogPathResults& set_up_default_log_path_results);
 
-  // Stop collecting NetLog data into the file. It is a no-op if we
-  // are not collecting data into a file.
-  void StopNetLog();
+  // Executes |state_callback| synchronously. StartLogging() is expected to be
+  // called asynchronously.
+  void StartLogging(const base::FilePath& log_path,
+                    net::NetLogCaptureMode capture_mode,
+                    const StateCallback& state_callback);
 
-  // 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();
+  // Executes |state_callback| asynchronously.
+  void StopLogging(const StateCallback& state_callback,
+                   std::unique_ptr<base::DictionaryValue> polled_data);
 
-  // Returns true if a file exists at |log_path_|.
-  bool NetExportLogExists() const;
+  // Executes |state_callback| synchronously.
+  // ResetObserverThenSetStateNotLogging() is expected to be called
+  // asynchronously.
+  void ResetObserverThenSetStateNotLogging(const StateCallback& state_callback);
+
+  std::unique_ptr<base::DictionaryValue> GetState() const;
+
+  // Gets the state and runs |state_callback| synchronously.
+  void RunStateCallback(const StateCallback& state_callback) const;
+
+  // Gets the state and runs |state_callback| asynchronously.
+  void RunStateCallbackAsync(const StateCallback& state_callback) const;
+
+  // If |set_file_permissions_success| is true, |path_callback| is executed with
+  // |path|. Otherwise, |path_callback| is executed with empty path.
+  static void RunFilePathCallback(const FilePathCallback& path_callback,
+                                  const base::FilePath& path,
+                                  bool set_file_permissions_success);
+
+  // All members are accessed solely from the main thread (the thread that
+  // |thread_checker_| is bound to).
 
   base::ThreadChecker thread_checker_;
 
-  // Helper function for unit tests.
-  State state() const { return state_; }
-  LogType log_type() const { return log_type_; }
+  // Task runners for file-specific and net-specific tasks that must run on a
+  // file or net thread.
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> net_task_runner_;
+
+  State state_;  // Current logging state of NetLogFileWriter.
 
-  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 +244,12 @@ class NetLogFileWriter {
   const base::CommandLine::StringType command_line_string_;
   const std::string channel_string_;
 
+  // Used by unit tests to override the default log base directory retrieved
+  // during initialization. This getter is initialized to base::GetTempDir().
+  DirectoryGetter default_log_base_directory_getter_;
+
+  base::WeakPtrFactory<NetLogFileWriter> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetLogFileWriter);
 };