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..ef2a0485f7ad71b8cbd2e04681fde153818c8c89 100644
--- a/components/net_log/net_log_file_writer.h
+++ b/components/net_log/net_log_file_writer.h
@@ -8,66 +8,117 @@
 #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 logs all the NetLog entries into a specified file. It does

[eroman, 2017/01/11 19:18:38]

I suggest also mentioning that this is used exclusively as a support-class for
net-export.
 * Shared by ios and non-ios implementations of net-export UI
 * Used as a singleton, so the net-export logging state can be shared between
instances of the UI

(In fact separately I will propose renaming this to something like
NetExportFileWriter)

[wangyix1, 2017/01/14 02:15:47]

Done.

+// so by managing an instance of net::FileNetLogObserver and handles the
+// attaching/detaching of it to the ChromeNetLog.
 //
-// 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.
-  };
-
-  virtual ~NetLogFileWriter();
-
-  // Accepts the button command and executes it.
-  void ProcessCommand(Command command);
-
-  // 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);
-
-  // Creates a Value summary of the state of the NetLogFileWriter. The caller is
-  // responsible for deleting the returned value.
-  base::DictionaryValue* GetState();
-
-  // Updates |log_path_| to the |custom_path|.
-  void SetUpNetExportLogPath(const base::FilePath& custom_path);
+  ~NetLogFileWriter();
+
+  // The three main commands StartNetLog(), StopNetLog(), and GetState() all
+  // accept a |state_callback| param which can be used to notify the caller of
+  // NetLogFileWriter's new state after a command finshes. |state_callback| will
+  // always be executed by a command even if the command ends up being a no-op
+  // or if some failure occurs.
+  using StateCallback =
+      base::Callback<void(std::unique_ptr<base::DictionaryValue>)>;
+
+  // 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 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.
+  //
+  // If NetLogFileWriter is not initialized, this will trigger initialization
+  // and |state_callback| will be executed at the end of StartNetLog()
+  // asynchronously (whether initialization is successful or not). Otherwise,
+  // |state_callback| will be executed at the end synchronously.

[eroman, 2017/01/11 19:18:39]

Instead of having a synchronous case, can you make all paths call state_callback
asynchronously?

That way there are fewer special cases for testing.

Synchronous callback invocation can be tricky and we avoid them wherever
possible (it can introduce unexpected re-entrancy problems)

[wangyix1, 2017/01/14 02:15:47]

Done.

+  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 we

[eroman, 2017/01/11 19:18:39]

nit: can you modify the language to avoid "we" ?

See also
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M
for motivation.

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // are not currently 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.
+  // StopNetLog() will automatically append net info (from net::GetNetInfo())
+  // to |polled_data|, which it will retrieve using |context_getter|.
+  //
+  // |state_callback| will always be executed at the end of StopNetLog()
+  // (even if this function does a no-op or if some failure occurs). It will
+  // be executed 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, this will trigger initialization
+  // and |state_callback| will be executed at the end of GetState()
+  // asynchronously (whether initialization is successful or not). Otherwise,

[eroman, 2017/01/11 19:18:39]

Same comment as earlier -- it is easier for consumers if this guarantees
execution to only be async.

This can be achieved by doing an extra post-task, and helps ensure that callers
don't need to be prepared to handle synchronous completion. The error cases in
particular tend to not be well exercised, so having it behave differently puts
it at risk of misbehaving (re-entrancy bugs can be subtle especially as changes
are made to the client code).

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // |state_callback| will be executed at the end synchronously.
+  void GetState(const StateCallback& state_callback);
+
+  using FilePathCallback = base::Callback<void(const base::FilePath&)>;

[eroman, 2017/01/11 19:18:39]

nit: move this to the top of the class declaration (beside the other using
statement)

See https://google.github.io/styleguide/cppguide.html#Declaration_Order

[wangyix1, 2017/01/14 02:15:47]

Done.

+
+  // Gets the log filepath. |path_callback| will be used to notify the caller

[eroman, 2017/01/11 19:18:39]

Excellent comments!

[wangyix1, 2017/01/14 02:15:47]

Acknowledged.

+  // when the filepath is retrieved. |path_callback| will be executed with an
+  // empty filepath if any of the following occurs:
+  // (1) The log file does not exist.
+  // (2) The NetLogFileWriter is currently logging.
+  // (3) The log file's permissions could not be set to all.
+  //
+  // In cases (1) and (2), |path_callback| will be executed synchronously. In

[eroman, 2017/01/11 19:18:39]

Same comment -- can we make all the completions async?

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // case (3) and the success case, it will be executed asynchronously.
+  void GetFilePath(const FilePathCallback& path_callback) const;

[eroman, 2017/01/11 19:18:39]

[optional] I suggest changing this function's name (since it is a bit unexpected
that it fails if logging is in progress). What about:

 * GetFilePathToCompletedLog()
 * GetFilePathIfDone()

[wangyix1, 2017/01/14 02:15:47]

Done.

+
+  // Sets the task runners used by NetLogFileWriter for running file-specific

[eroman, 2017/01/11 19:18:38]

I suggest expanding this comment:

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 with
the same parameters is OK.

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // and net-specific tasks that must be run on those threads.
+  void SetTaskRunners(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> net_task_runner);
+
+  static std::string CaptureModeToString(net::NetLogCaptureMode capture_mode);

[eroman, 2017/01/11 19:18:38]

Suggest adding this comment:

// Converts to/from the string representation of a capture mode used by
net_export.js

[wangyix1, 2017/01/14 02:15:47]

Done.

+  static net::NetLogCaptureMode CaptureModeFromString(
+      const std::string& capture_mode_string);
 
  protected:
   // Constructs a NetLogFileWriter. Only one instance is created in browser
@@ -76,91 +127,109 @@ 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, EnsureInitFailure);

[eroman, 2017/01/11 19:18:39]

NOTE: I assume this will be updated in a subsequent patchset

[wangyix1, 2017/01/14 02:15:47]

Acknowledged.

   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,
+  // Helper function used by commands that may trigger initialization
+  // (StartNetLog(), GetState()). This function attemps to initialize

[eroman, 2017/01/11 19:18:39]

typo: attempts

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // NetLogFileWriter asynchronously, then executes |callback|.
+  void InitAndCallback(const base::Closure& callback);

[eroman, 2017/01/11 19:18:39]

[optional] InitThenCallback() ?

[wangyix1, 2017/01/14 02:15:47]

Done.

+
+  // Struct used to store the results of SetUpDefaultLogPath() which will be
+  // passed to InitStateAndCallback().
+  struct DefaultLogPathResults {

[eroman, 2017/01/11 19:18:39]

Move the struct declaration to be below the enum State
(we group together declarations at the start of public/private/protected blocks)

[wangyix1, 2017/01/14 02:15:47]

Done.

+    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);
+  // Contains file-related initialization tasks. Will run on the file task
+  // runner.
+  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(), then executes |callback|.

[eroman, 2017/01/11 19:18:39]

mention if the execution  of |callback| is synchronous vs asynchronous

[wangyix1, 2017/01/14 02:15:47]

Done.

+  void InitStateAndCallback(const base::Closure& callback,
+                            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);
+  void StartLogging(const base::FilePath& log_path,
+                    net::NetLogCaptureMode capture_mode,
+                    const StateCallback& state_callback);
 
-  // Stop collecting NetLog data into the file. It is a no-op if we
-  // are not collecting data into a file.
-  void StopNetLog();
+  // Adds net info from net::GetNetInfo() to |polled_data|. Will run on the

[eroman, 2017/01/11 19:18:39]

Will run on the --> must be run on the

[wangyix1, 2017/01/14 02:15:47]

Done.

+  // net task runner.
+  static std::unique_ptr<base::DictionaryValue> AddNetInfo(

[eroman, 2017/01/11 19:18:39]

Consider moving this declaration to the .cc
(i.e. it doesn't need to be exposed in the header file, since it is a private
static method).

[wangyix1, 2017/01/14 02:15:47]

Done.

+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      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();
+  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;
+  void ResetObserverThenSetStateNotLogging(const StateCallback& state_callback);
 
-  base::ThreadChecker thread_checker_;
+  std::unique_ptr<base::DictionaryValue> GetState() const;
+
+  // Gets the state and runs |state_callback|.
+  void RunStateCallback(const StateCallback& state_callback) const;
+
+  // 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);

[eroman, 2017/01/11 19:18:38]

This can also be moved to the .cc file  (i.e. remove from header).

[wangyix1, 2017/01/14 02:15:47]

Done.

+
+  // 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);
 
   // Helper function for unit tests.
   State state() const { return state_; }
-  LogType log_type() const { return log_type_; }
 
-  State state_;       // Current state of NetLogFileWriter.
-  LogType log_type_;  // Type of current log file on disk.
+  // All members are accessed solely from the main thread (the thread that
+  // |thread_checker_| is bound to).
+
+  base::ThreadChecker thread_checker_;
+
+  // 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.
+
+  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 +238,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);
 };