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

components/net_log/net_log_file_writer_unittest.cc

Index: components/net_log/net_log_file_writer_unittest.cc
diff --git a/components/net_log/net_log_file_writer_unittest.cc b/components/net_log/net_log_file_writer_unittest.cc
index 47b80833e702a7b93da0ac29c5c4b610be30bbb3..7c34ab2fb56b5020a46038fc0af16557c2c89e8d 100644
--- a/components/net_log/net_log_file_writer_unittest.cc
+++ b/components/net_log/net_log_file_writer_unittest.cc
@@ -15,432 +15,592 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "components/net_log/chrome_net_log.h"
+#include "net/base/test_completion_callback.h"
+#include "net/http/http_network_session.h"
 #include "net/log/net_log_capture_mode.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/write_to_file_net_log_observer.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 const char kChannelString[] = "SomeChannel";
 
+// Keep this in sync with kLogRelativePath defined in net_log_file_writer.cc.
+base::FilePath::CharType kLogRelativePath[] =
+    FILE_PATH_LITERAL("net-export/chrome-net-export-log.json");
+
+const char kCaptureModeDefaultString[] = "STRIP_PRIVATE_DATA";
+const char kCaptureModeIncludeCookiesAndCredentialsString[] = "NORMAL";
+const char kCaptureModeIncludeSocketBytesString[] = "LOG_BYTES";
+
+const char kStateUninitializedString[] = "UNINITIALIZED";
+const char kStateNotLoggingString[] = "NOT_LOGGING";
+const char kStateLoggingString[] = "LOGGING";
+
 }  // namespace
 
 namespace net_log {
 
-class TestNetLogFileWriter : public NetLogFileWriter {
- public:
-  explicit TestNetLogFileWriter(ChromeNetLog* chrome_net_log)
-      : NetLogFileWriter(
-            chrome_net_log,
-            base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
-            kChannelString),
-        lie_about_net_export_log_directory_(false) {
-    EXPECT_TRUE(net_log_temp_dir_.CreateUniqueTempDir());
+// Sets |path| to |path_to_return| and always returns true. This function is
+// used to override NetLogFileWriter's usual getter for the default log base
+// directory.
+bool SetPathToGivenAndReturnTrue(const base::FilePath& path_to_return,
+                                 base::FilePath* path) {
+  *path = path_to_return;
+  return true;
+}
+
+// Checks the fields of the state returned by NetLogFileWriter's state callback.
+// |expected_log_capture_mode_string| is only checked if
+// |expected_log_capture_mode_known| is true.
+void VerifyState(std::unique_ptr<base::DictionaryValue> state,
+                 const std::string& expected_state_string,
+                 bool expected_log_exists,
+                 bool expected_log_capture_mode_known,
+                 const std::string& expected_log_capture_mode_string) {
+  std::string state_string;
+  ASSERT_TRUE(state->GetString("state", &state_string));
+  EXPECT_EQ(state_string, expected_state_string);
+
+  bool log_exists;
+  ASSERT_TRUE(state->GetBoolean("logExists", &log_exists));
+  EXPECT_EQ(log_exists, expected_log_exists);
+
+  bool log_capture_mode_known;
+  ASSERT_TRUE(
+      state->GetBoolean("logCaptureModeKnown", &log_capture_mode_known));
+  EXPECT_EQ(log_capture_mode_known, expected_log_capture_mode_known);
+
+  if (expected_log_capture_mode_known) {
+    std::string log_capture_mode_string;
+    ASSERT_TRUE(state->GetString("captureMode", &log_capture_mode_string));
+    EXPECT_EQ(log_capture_mode_string, expected_log_capture_mode_string);
   }
+}
 
-  ~TestNetLogFileWriter() override { EXPECT_TRUE(net_log_temp_dir_.Delete()); }
+::testing::AssertionResult ReadCompleteLogFile(
+    const base::FilePath& log_path,
+    std::unique_ptr<base::DictionaryValue>* root) {
+  DCHECK(!log_path.empty());
 
-  // NetLogFileWriter implementation:
-  bool GetNetExportLogBaseDirectory(base::FilePath* path) const override {
-    if (lie_about_net_export_log_directory_)
-      return false;
-    *path = net_log_temp_dir_.GetPath();
-    return true;
+  if (!base::PathExists(log_path)) {
+    return ::testing::AssertionFailure() << log_path.value()
+                                         << " does not exist.";
   }
+  // Parse log file contents into a dictionary
+  std::string log_string;
+  if (!base::ReadFileToString(log_path, &log_string)) {
+    return ::testing::AssertionFailure() << log_path.value()
+                                         << " could not be read.";
+  }
+  *root = base::DictionaryValue::From(base::JSONReader::Read(log_string));
+  if (!*root) {
+    return ::testing::AssertionFailure()
+           << "Contents of " << log_path.value()
+           << " do not form valid JSON dictionary.";
+  }
+  // Make sure the "constants" section exists
+  base::DictionaryValue* constants;
+  if (!(*root)->GetDictionary("constants", &constants)) {
+    root->reset();
+    return ::testing::AssertionFailure() << log_path.value()
+                                         << " does not contain constants.";
+  }
+  // Make sure the "events" section exists
+  base::ListValue* events;
+  if (!(*root)->GetList("events", &events)) {
+    root->reset();
+    return ::testing::AssertionFailure() << log_path.value()
+                                         << " does not contain events list.";
+  }
+  return ::testing::AssertionSuccess();
+}
 
-  void set_lie_about_net_export_log_directory(
-      bool lie_about_net_export_log_directory) {
-    lie_about_net_export_log_directory_ = lie_about_net_export_log_directory;
+void SetUpTestContextWithQuicTimeoutInfoThenSignal(
+    net::NetLog* net_log,
+    int quic_idle_connection_timeout_seconds,
+    std::unique_ptr<net::TestURLRequestContext>* context,
+    base::WaitableEvent* done_event) {

[eroman, 2017/01/27 02:23:23]

See comment later on (I think this can be accomplished using PostTaskAndReply()
instead of WaitableEvent.

[wangyix1, 2017/01/27 18:21:09]

Done.

+  context->reset(new net::TestURLRequestContext(true));
+  (*context)->set_net_log(net_log);
+
+  std::unique_ptr<net::HttpNetworkSession::Params> params(
+      new net::HttpNetworkSession::Params);
+  params->quic_idle_connection_timeout_seconds =
+      quic_idle_connection_timeout_seconds;
+
+  (*context)->set_http_network_session_params(std::move(params));
+  (*context)->Init();
+
+  done_event->Signal();
+}
+
+// A class that wraps around TestClosure. Provides the ability to wait on a
+// state callback and retrieve the result.
+class TestStateCallback {
+ public:
+  TestStateCallback()
+      : callback_(base::Bind(&TestStateCallback::SetResultThenNotify,
+                             base::Unretained(this))) {}
+
+  const base::Callback<void(std::unique_ptr<base::DictionaryValue>)>& callback()
+      const {
+    return callback_;
+  }
+
+  std::unique_ptr<base::DictionaryValue> WaitForResult() {
+    test_closure_.WaitForResult();
+    return std::move(result_);
   }
 
  private:
-  bool lie_about_net_export_log_directory_;
+  void SetResultThenNotify(std::unique_ptr<base::DictionaryValue> result) {
+    result_ = std::move(result);
+    test_closure_.closure().Run();
+  }
 
-  base::ScopedTempDir net_log_temp_dir_;
+  net::TestClosure test_closure_;
+  std::unique_ptr<base::DictionaryValue> result_;
+  base::Callback<void(std::unique_ptr<base::DictionaryValue>)> callback_;
 };
 
-class NetLogFileWriterTest : public ::testing::Test {
+// A class that wraps around TestClosure. Provides the ability to wait on a
+// file path callback and retrieve the result.
+class TestFilePathCallback {
  public:
-  NetLogFileWriterTest()
-      : net_log_(new ChromeNetLog(
-            base::FilePath(),
-            net::NetLogCaptureMode::Default(),
-            base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
-            kChannelString)),
-        net_log_file_writer_(new TestNetLogFileWriter(net_log_.get())) {}
-
-  std::string GetStateString() const {
-    std::unique_ptr<base::DictionaryValue> dict(
-        net_log_file_writer_->GetState());
-    std::string state;
-    EXPECT_TRUE(dict->GetString("state", &state));
-    return state;
-  }
+  TestFilePathCallback()
+      : callback_(base::Bind(&TestFilePathCallback::SetResultThenNotify,
+                             base::Unretained(this))) {}
 
-  std::string GetLogTypeString() const {
-    std::unique_ptr<base::DictionaryValue> dict(
-        net_log_file_writer_->GetState());
-    std::string log_type;
-    EXPECT_TRUE(dict->GetString("logType", &log_type));
-    return log_type;
+  const base::Callback<void(const base::FilePath&)>& callback() const {
+    return callback_;
   }
 
-  // Make sure the export file has been created and is non-empty, as net
-  // constants will always be written to it on creation.
-  void VerifyNetExportLogExists() {
-    net_export_log_ = net_log_file_writer_->log_path_;
-    ASSERT_TRUE(base::PathExists(net_export_log_));
+  const base::FilePath& WaitForResult() {
+    test_closure_.WaitForResult();
+    return result_;
+  }
 
-    int64_t file_size;
-    // base::GetFileSize returns proper file size on open handles.
-    ASSERT_TRUE(base::GetFileSize(net_export_log_, &file_size));
-    EXPECT_GT(file_size, 0);
+ private:
+  void SetResultThenNotify(const base::FilePath& result) {
+    result_ = result;
+    test_closure_.closure().Run();
   }
 
-  // Make sure the export file has been created and a valid JSON file.  This
-  // should always be the case once logging has been stopped.
-  void VerifyNetExportLogComplete() {
-    VerifyNetExportLogExists();
+  net::TestClosure test_closure_;
+  base::FilePath result_;
+  base::Callback<void(const base::FilePath&)> callback_;
+};
 
-    std::string log;
-    ASSERT_TRUE(ReadFileToString(net_export_log_, &log));
-    base::JSONReader reader;
-    std::unique_ptr<base::Value> json = base::JSONReader::Read(log);
-    EXPECT_TRUE(json);
-  }
+class NetLogFileWriterTest : public ::testing::Test {
+ public:
+  NetLogFileWriterTest()
+      : net_log_(base::FilePath(),
+                 net::NetLogCaptureMode::Default(),
+                 base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
+                 kChannelString),
+        net_log_file_writer_(
+            &net_log_,
+            base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
+            kChannelString),
+        file_thread_("NetLogFileWriter file thread"),
+        net_thread_("NetLogFileWriter net thread") {}
 
-  // Verify state and GetFilePath return correct values if EnsureInit() fails.
-  void VerifyFilePathAndStateAfterEnsureInitFailure() {
-    EXPECT_EQ("UNINITIALIZED", GetStateString());
-    EXPECT_EQ(NetLogFileWriter::STATE_UNINITIALIZED,
-              net_log_file_writer_->state());
+  // ::testing::Test implementation
+  void SetUp() override {
+    ASSERT_TRUE(log_temp_dir_.CreateUniqueTempDir());
 
-    base::FilePath net_export_file_path;
-    EXPECT_FALSE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-  }
+    // Override |net_log_file_writer_|'s default-log-base-directory-getter to
+    // a getter that returns the temp dir created for the test.
+    net_log_file_writer_.SetDefaultLogBaseDirectoryGetterForTest(
+        base::Bind(&SetPathToGivenAndReturnTrue, log_temp_dir_.GetPath()));
 
-  // When we lie in NetExportLogExists, make sure state and GetFilePath return
-  // correct values.
-  void VerifyFilePathAndStateAfterEnsureInit() {
-    EXPECT_EQ("NOT_LOGGING", GetStateString());
-    EXPECT_EQ(NetLogFileWriter::STATE_NOT_LOGGING,
-              net_log_file_writer_->state());
-    EXPECT_EQ("NONE", GetLogTypeString());
-    EXPECT_EQ(NetLogFileWriter::LOG_TYPE_NONE,
-              net_log_file_writer_->log_type());
-
-    base::FilePath net_export_file_path;
-    EXPECT_FALSE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-    EXPECT_FALSE(net_log_file_writer_->NetExportLogExists());
-  }
+    default_log_path_ = log_temp_dir_.GetPath().Append(kLogRelativePath);
 
-  // The following methods make sure the export file has been successfully
-  // initialized by a DO_START command of the given type.
+    file_thread_.Start();

[eroman, 2017/01/27 02:23:23]

Can you put an ASSERT_TRUE() around the calls to Start() ?

[wangyix1, 2017/01/27 18:21:09]

Done.

+    net_thread_.Start();
+    ASSERT_TRUE(file_thread_.WaitUntilThreadStarted());

[eroman, 2017/01/27 02:23:24]

I don't think these two lines are needed for correctness of the tests.

[wangyix1, 2017/01/27 18:21:09]

Done.

+    ASSERT_TRUE(net_thread_.WaitUntilThreadStarted());
 
-  void VerifyFileAndStateAfterDoStart() {
-    VerifyFileAndStateAfterStart(
-        NetLogFileWriter::LOG_TYPE_NORMAL, "NORMAL",
-        net::NetLogCaptureMode::IncludeCookiesAndCredentials());
+    net_log_file_writer_.SetTaskRunners(file_thread_.task_runner(),
+                                        net_thread_.task_runner());
   }
+  void TearDown() override { ASSERT_TRUE(log_temp_dir_.Delete()); }
 
-  void VerifyFileAndStateAfterDoStartStripPrivateData() {
-    VerifyFileAndStateAfterStart(NetLogFileWriter::LOG_TYPE_STRIP_PRIVATE_DATA,
-                                 "STRIP_PRIVATE_DATA",
-                                 net::NetLogCaptureMode::Default());
+  std::unique_ptr<base::DictionaryValue> FileWriterGetState() {
+    TestStateCallback test_callback;
+    net_log_file_writer_.GetState(test_callback.callback());
+    return test_callback.WaitForResult();
   }
 
-  void VerifyFileAndStateAfterDoStartLogBytes() {
-    VerifyFileAndStateAfterStart(NetLogFileWriter::LOG_TYPE_LOG_BYTES,
-                                 "LOG_BYTES",
-                                 net::NetLogCaptureMode::IncludeSocketBytes());
+  base::FilePath FileWriterGetFilePathToCompletedLog() {
+    TestFilePathCallback test_callback;
+    net_log_file_writer_.GetFilePathToCompletedLog(test_callback.callback());
+    return test_callback.WaitForResult();
   }
 
-  // Make sure the export file has been successfully initialized after DO_STOP
-  // command following a DO_START command of the given type.
-
-  void VerifyFileAndStateAfterDoStop() {
-    VerifyFileAndStateAfterDoStop(NetLogFileWriter::LOG_TYPE_NORMAL, "NORMAL");
+  // If |custom_log_path| is empty path, |net_log_file_writer_| will use its
+  // default log path.
+  void StartThenVerifyState(const base::FilePath& custom_log_path,
+                            net::NetLogCaptureMode capture_mode,
+                            const std::string& expected_capture_mode_string) {
+    TestStateCallback test_callback;
+    net_log_file_writer_.StartNetLog(custom_log_path, capture_mode,
+                                     test_callback.callback());
+    VerifyState(test_callback.WaitForResult(), kStateLoggingString, true, true,

[eroman, 2017/01/27 02:23:23]

optional: For readability I suggest extracting test_callback.WatitForResult() to
its own line:

std::unique_ptr<base::DictionaryValue> state = test_callback.WaitForResult();

[wangyix1, 2017/01/27 18:21:09]

Done.

+                expected_capture_mode_string);
+
+    // Make sure NetLogFileWriter::GetFilePath() returns empty path when
+    // logging.
+    EXPECT_TRUE(FileWriterGetFilePathToCompletedLog().empty());
   }
 
-  void VerifyFileAndStateAfterDoStopWithStripPrivateData() {
-    VerifyFileAndStateAfterDoStop(NetLogFileWriter::LOG_TYPE_STRIP_PRIVATE_DATA,
-                                  "STRIP_PRIVATE_DATA");
+  // If |custom_log_path| is empty path, it's assumed the log file with be at
+  // |default_path_|.
+  void StopThenVerifyStateAndFile(
+      const base::FilePath& custom_log_path,
+      std::unique_ptr<base::DictionaryValue> polled_data,
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      const std::string& expected_capture_mode_string) {
+    TestStateCallback test_callback;
+    net_log_file_writer_.StopNetLog(std::move(polled_data), context_getter,
+                                    test_callback.callback());
+    VerifyState(test_callback.WaitForResult(), kStateNotLoggingString, true,

[eroman, 2017/01/27 02:23:24]

same optional suggestion here.

[wangyix1, 2017/01/27 18:21:09]

Done.

+                true, expected_capture_mode_string);
+
+    const base::FilePath& log_path =
+        custom_log_path.empty() ? default_log_path_ : custom_log_path;
+    EXPECT_EQ(FileWriterGetFilePathToCompletedLog(), log_path);
+
+    std::unique_ptr<base::DictionaryValue> root;
+    ASSERT_TRUE(ReadCompleteLogFile(log_path, &root));
   }
 
-  void VerifyFileAndStateAfterDoStopWithLogBytes() {
-    VerifyFileAndStateAfterDoStop(NetLogFileWriter::LOG_TYPE_LOG_BYTES,
-                                  "LOG_BYTES");
-  }
+ protected:
+  ChromeNetLog net_log_;
 
-  std::unique_ptr<ChromeNetLog> net_log_;
   // |net_log_file_writer_| is initialized after |net_log_| so that it can stop
   // obvserving on destruction.
-  std::unique_ptr<TestNetLogFileWriter> net_log_file_writer_;
-  base::FilePath net_export_log_;
-
- private:
-  // Checks state after one of the DO_START* commands.
-  void VerifyFileAndStateAfterStart(
-      NetLogFileWriter::LogType expected_log_type,
-      const std::string& expected_log_type_string,
-      net::NetLogCaptureMode expected_capture_mode) {
-    EXPECT_EQ(NetLogFileWriter::STATE_LOGGING, net_log_file_writer_->state());
-    EXPECT_EQ("LOGGING", GetStateString());
-    EXPECT_EQ(expected_log_type, net_log_file_writer_->log_type());
-    EXPECT_EQ(expected_log_type_string, GetLogTypeString());
-    EXPECT_EQ(expected_capture_mode,
-              net_log_file_writer_->write_to_file_observer_->capture_mode());
-
-    // Check GetFilePath returns false when still writing to the file.
-    base::FilePath net_export_file_path;
-    EXPECT_FALSE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-
-    VerifyNetExportLogExists();
-  }
+  NetLogFileWriter net_log_file_writer_;
 
-  void VerifyFileAndStateAfterDoStop(
-      NetLogFileWriter::LogType expected_log_type,
-      const std::string& expected_log_type_string) {
-    EXPECT_EQ(NetLogFileWriter::STATE_NOT_LOGGING,
-              net_log_file_writer_->state());
-    EXPECT_EQ("NOT_LOGGING", GetStateString());
-    EXPECT_EQ(expected_log_type, net_log_file_writer_->log_type());
-    EXPECT_EQ(expected_log_type_string, GetLogTypeString());
+  base::ScopedTempDir log_temp_dir_;
 
-    base::FilePath net_export_file_path;
-    EXPECT_TRUE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-    EXPECT_EQ(net_export_log_, net_export_file_path);
+  // The default log path that |net_log_file_writer_| will use is cached here.
+  base::FilePath default_log_path_;
 
-    VerifyNetExportLogComplete();
-  }
+  base::Thread file_thread_;
+  base::Thread net_thread_;
 
+ private:
+  // Allows tasks to be posted to the main thread.
   base::MessageLoop message_loop_;
 };
 
-TEST_F(NetLogFileWriterTest, EnsureInitFailure) {
-  net_log_file_writer_->set_lie_about_net_export_log_directory(true);
+TEST_F(NetLogFileWriterTest, InitFail) {
+  // Override net_log_file_writer_'s default log base directory getter to always
+  // fail.
+  net_log_file_writer_.SetDefaultLogBaseDirectoryGetterForTest(
+      base::Bind([](base::FilePath* path) -> bool { return false; }));
 
-  EXPECT_FALSE(net_log_file_writer_->EnsureInit());
-  VerifyFilePathAndStateAfterEnsureInitFailure();
+  // GetState() will cause |net_log_file_writer_| to initialize. In this case,
+  // initialization will fail since |net_log_file_writer_| will fail to
+  // retrieve the default log base directory.
+  VerifyState(FileWriterGetState(), kStateUninitializedString, false, false,
+              "");
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFilePathAndStateAfterEnsureInitFailure();
+  // NetLogFileWriter::GetFilePath() should return empty path if uninitialized.
+  EXPECT_TRUE(FileWriterGetFilePathToCompletedLog().empty());
 }
 
-TEST_F(NetLogFileWriterTest, EnsureInitAllowStart) {
-  EXPECT_TRUE(net_log_file_writer_->EnsureInit());
-  VerifyFilePathAndStateAfterEnsureInit();
-
-  // Calling EnsureInit() second time should be a no-op.
-  EXPECT_TRUE(net_log_file_writer_->EnsureInit());
-  VerifyFilePathAndStateAfterEnsureInit();
-
-  // GetFilePath() should failed when there's no file.
-  base::FilePath net_export_file_path;
-  EXPECT_FALSE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-}
+TEST_F(NetLogFileWriterTest, InitWithoutExistingLog) {
+  // GetState() will cause |net_log_file_writer_| to initialize.
+  VerifyState(FileWriterGetState(), kStateNotLoggingString, false, false, "");
 
-TEST_F(NetLogFileWriterTest, EnsureInitAllowStartOrSend) {
-  net_log_file_writer_->SetUpDefaultNetExportLogPath();
-  net_export_log_ = net_log_file_writer_->log_path_;
-
-  // Create and close an empty log file, to simulate an old log file already
-  // existing.
-  base::ScopedFILE created_file(base::OpenFile(net_export_log_, "w"));
-  ASSERT_TRUE(created_file.get());
-  created_file.reset();
-
-  EXPECT_TRUE(net_log_file_writer_->EnsureInit());
-
-  EXPECT_EQ("NOT_LOGGING", GetStateString());
-  EXPECT_EQ(NetLogFileWriter::STATE_NOT_LOGGING, net_log_file_writer_->state());
-  EXPECT_EQ("UNKNOWN", GetLogTypeString());
-  EXPECT_EQ(NetLogFileWriter::LOG_TYPE_UNKNOWN,
-            net_log_file_writer_->log_type());
-  EXPECT_EQ(net_export_log_, net_log_file_writer_->log_path_);
-  EXPECT_TRUE(base::PathExists(net_export_log_));
-
-  base::FilePath net_export_file_path;
-  EXPECT_TRUE(net_log_file_writer_->GetFilePath(&net_export_file_path));
-  EXPECT_TRUE(base::PathExists(net_export_file_path));
-  EXPECT_EQ(net_export_log_, net_export_file_path);
+  // NetLogFileWriter::GetFilePathToCompletedLog() should return empty path when
+  // no log file exists.
+  EXPECT_TRUE(FileWriterGetFilePathToCompletedLog().empty());
 }
 
-TEST_F(NetLogFileWriterTest, ProcessCommandDoStartAndStop) {
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+TEST_F(NetLogFileWriterTest, InitWithExistingLog) {
+  // Create and close an empty log file to simulate existence of a previous log
+  // file.
+  ASSERT_TRUE(
+      base::CreateDirectoryAndGetError(default_log_path_.DirName(), nullptr));
+  base::ScopedFILE empty_file(base::OpenFile(default_log_path_, "w"));
+  ASSERT_TRUE(empty_file.get());
+  empty_file.reset();
 
-  // Calling a second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+  // GetState() will cause |net_log_file_writer_| to initialize.
+  VerifyState(FileWriterGetState(), kStateNotLoggingString, true, false, "");
 
-  // starting with other log levels should also be no-ops.
-  net_log_file_writer_->ProcessCommand(
-      NetLogFileWriter::DO_START_STRIP_PRIVATE_DATA);
-  VerifyFileAndStateAfterDoStart();
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START_LOG_BYTES);
-  VerifyFileAndStateAfterDoStart();
-
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
-
-  // Calling DO_STOP second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  EXPECT_EQ(FileWriterGetFilePathToCompletedLog(), default_log_path_);
 }
 
-TEST_F(NetLogFileWriterTest,
-       ProcessCommandDoStartAndStopWithPrivateDataStripping) {
-  net_log_file_writer_->ProcessCommand(
-      NetLogFileWriter::DO_START_STRIP_PRIVATE_DATA);
-  VerifyFileAndStateAfterDoStartStripPrivateData();
+TEST_F(NetLogFileWriterTest, StartAndStopWithAllCaptureModes) {
+  const net::NetLogCaptureMode capture_modes[3] = {
+      net::NetLogCaptureMode::Default(),
+      net::NetLogCaptureMode::IncludeCookiesAndCredentials(),
+      net::NetLogCaptureMode::IncludeSocketBytes()};
 
-  // Calling a second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(
-      NetLogFileWriter::DO_START_STRIP_PRIVATE_DATA);
-  VerifyFileAndStateAfterDoStartStripPrivateData();
+  const std::string capture_mode_strings[3] = {
+      kCaptureModeDefaultString, kCaptureModeIncludeCookiesAndCredentialsString,
+      kCaptureModeIncludeSocketBytesString};
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStopWithStripPrivateData();
+  // For each capture mode, start and stop |net_log_file_writer_| in that mode.
+  for (int i = 0; i < 3; ++i) {
+    StartThenVerifyState(base::FilePath(), capture_modes[i],
+                         capture_mode_strings[i]);
 
-  // Calling DO_STOP second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStopWithStripPrivateData();
-}
+    // Starting a second time should be a no-op.
+    StartThenVerifyState(base::FilePath(), capture_modes[i],
+                         capture_mode_strings[i]);
 
-TEST_F(NetLogFileWriterTest, ProcessCommandDoStartAndStopWithByteLogging) {
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START_LOG_BYTES);
-  VerifyFileAndStateAfterDoStartLogBytes();
+    // Starting with other capture modes should also be no-ops. This should also
+    // not affect the capture mode reported by |net_log_file_writer_|.
+    StartThenVerifyState(base::FilePath(), capture_modes[(i + 1) % 3],
+                         capture_mode_strings[i]);
 
-  // Calling a second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START_LOG_BYTES);
-  VerifyFileAndStateAfterDoStartLogBytes();
+    StartThenVerifyState(base::FilePath(), capture_modes[(i + 2) % 3],
+                         capture_mode_strings[i]);
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStopWithLogBytes();
+    StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                               capture_mode_strings[i]);
 
-  // Calling DO_STOP second time should be a no-op.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStopWithLogBytes();
+    // Stopping a second time should be a no-op.
+    StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                               capture_mode_strings[i]);
+  }
 }
 
-TEST_F(NetLogFileWriterTest, DoStartClearsFile) {
-  // Verify file sizes after two consecutive starts/stops are the same (even if
-  // we add some junk data in between).
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+// Verify the file sizes after two consecutive starts/stops are the same (even
+// if some junk data is added in between).
+TEST_F(NetLogFileWriterTest, StartClearsFile) {
+  StartThenVerifyState(base::FilePath(), net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  int64_t start_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &start_file_size));
-
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
   int64_t stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &stop_file_size));
-  EXPECT_GE(stop_file_size, start_file_size);
+  EXPECT_TRUE(base::GetFileSize(default_log_path_, &stop_file_size));
 
   // Add some junk at the end of the file.
   std::string junk_data("Hello");
-  EXPECT_TRUE(
-      base::AppendToFile(net_export_log_, junk_data.c_str(), junk_data.size()));
+  EXPECT_TRUE(base::AppendToFile(default_log_path_, junk_data.c_str(),
+                                 junk_data.size()));
 
   int64_t junk_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &junk_file_size));
+  EXPECT_TRUE(base::GetFileSize(default_log_path_, &junk_file_size));
   EXPECT_GT(junk_file_size, stop_file_size);
 
-  // Execute DO_START/DO_STOP commands and make sure the file is back to the
-  // size before addition of junk data.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
-
-  int64_t new_start_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &new_start_file_size));
-  EXPECT_EQ(new_start_file_size, start_file_size);
+  // Start and stop again and make sure the file is back to the size it was
+  // before adding the junk data.
+  StartThenVerifyState(base::FilePath(), net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
   int64_t new_stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &new_stop_file_size));
+  EXPECT_TRUE(base::GetFileSize(default_log_path_, &new_stop_file_size));
+
   EXPECT_EQ(new_stop_file_size, stop_file_size);
 }
 
-TEST_F(NetLogFileWriterTest, CheckAddEvent) {
-  // Add an event to |net_log_| and then test to make sure that, after we stop
-  // logging, the file is larger than the file created without that event.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+// Adds an event to the log file, then checks that the file is larger than
+// the file created without that event.
+TEST_F(NetLogFileWriterTest, AddEvent) {
+  StartThenVerifyState(base::FilePath(), net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  // Get file size without the event.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
+  // Get file size without the event.
   int64_t stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &stop_file_size));
+  EXPECT_TRUE(base::GetFileSize(default_log_path_, &stop_file_size));
 
-  // Perform DO_START and add an Event and then DO_STOP and then compare
-  // file sizes.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+  StartThenVerifyState(base::FilePath(), net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  // Log an event.
-  net_log_->AddGlobalEntry(net::NetLogEventType::CANCELLED);
+  net_log_.AddGlobalEntry(net::NetLogEventType::CANCELLED);
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  StopThenVerifyStateAndFile(base::FilePath(), nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
   int64_t new_stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &new_stop_file_size));
+  EXPECT_TRUE(base::GetFileSize(default_log_path_, &new_stop_file_size));
+
   EXPECT_GE(new_stop_file_size, stop_file_size);
 }
 
-TEST_F(NetLogFileWriterTest, CheckAddEventWithCustomPath) {
-  // Using a custom path to make sure logging can still occur when
-  // the path has changed.
-  base::FilePath path;
-  net_log_file_writer_->GetNetExportLogBaseDirectory(&path);
-
-  base::FilePath::CharType kCustomPath[] =
+// Using a custom path to make sure logging can still occur when
+// the path has changed.
+TEST_F(NetLogFileWriterTest, AddEventCustomPath) {
+  base::FilePath::CharType kCustomRelativePath[] =
       FILE_PATH_LITERAL("custom/custom/chrome-net-export-log.json");
-  base::FilePath custom_path = path.Append(kCustomPath);
-
-  EXPECT_TRUE(base::CreateDirectoryAndGetError(custom_path.DirName(), nullptr));
+  base::FilePath custom_log_path =
+      log_temp_dir_.GetPath().Append(kCustomRelativePath);
+  EXPECT_TRUE(
+      base::CreateDirectoryAndGetError(custom_log_path.DirName(), nullptr));
 
-  net_log_file_writer_->SetUpNetExportLogPath(custom_path);
-  net_export_log_ = net_log_file_writer_->log_path_;
-  EXPECT_EQ(custom_path, net_export_log_);
+  StartThenVerifyState(custom_log_path, net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  // Add an event to |net_log_| and then test to make sure that, after we stop
-  // logging, the file is larger than the file created without that event.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+  StopThenVerifyStateAndFile(custom_log_path, nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
   // Get file size without the event.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
-
   int64_t stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &stop_file_size));
+  EXPECT_TRUE(base::GetFileSize(custom_log_path, &stop_file_size));
 
-  // Perform DO_START and add an Event and then DO_STOP and then compare
-  // file sizes.
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_START);
-  VerifyFileAndStateAfterDoStart();
+  StartThenVerifyState(custom_log_path, net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
 
-  // Log an event.
-  net_log_->AddGlobalEntry(net::NetLogEventType::CANCELLED);
+  net_log_.AddGlobalEntry(net::NetLogEventType::CANCELLED);
 
-  net_log_file_writer_->ProcessCommand(NetLogFileWriter::DO_STOP);
-  VerifyFileAndStateAfterDoStop();
+  StopThenVerifyStateAndFile(custom_log_path, nullptr, nullptr,
+                             kCaptureModeDefaultString);
 
   int64_t new_stop_file_size;
-  EXPECT_TRUE(base::GetFileSize(net_export_log_, &new_stop_file_size));
+  EXPECT_TRUE(base::GetFileSize(custom_log_path, &new_stop_file_size));
   EXPECT_GE(new_stop_file_size, stop_file_size);
 }
 
+TEST_F(NetLogFileWriterTest, StopWithPolledDataAndContextGetter) {
+  // Create dummy polled data
+  const char kDummyPolledDataPath[] = "dummy_path";
+  const char kDummyPolledDataString[] = "dummy_info";
+  std::unique_ptr<base::DictionaryValue> dummy_polled_data =
+      base::MakeUnique<base::DictionaryValue>();
+  dummy_polled_data->SetString(kDummyPolledDataPath, kDummyPolledDataString);
+
+  // Create test context getter on |net_thread_| and wait for it to finish.

[eroman, 2017/01/27 02:23:24]

wording: the "context getter" isn't created on the |net_thread|, only the
context is.

[wangyix1, 2017/01/27 18:21:09]

Done.

+  std::unique_ptr<net::TestURLRequestContext> context;
+  const int kDummyQuicParam = 75;
+
+  base::WaitableEvent create_context_done_event(

[eroman, 2017/01/27 02:23:24]

Another less verbose way to accomplish this is using PostTaskAndReply().
I haven't verified but I think this would work:

base::TestClosure init_done;

...->PostTaskAndReply(FROM_HERE, base::Bind(...), init_done.closure());

init_done.WaitForResult();

The advantage of this way is you don't need to initialize a WaitableEvent, or
pass one into the setup function. Also if initialization needed to run tasks on
the main thread that would work (since the main thread isn't blocked).

[wangyix1, 2017/01/27 18:21:09]

Done.

+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  net_thread_.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&SetUpTestContextWithQuicTimeoutInfoThenSignal, &net_log_,
+                 kDummyQuicParam, &context, &create_context_done_event));
+  create_context_done_event.Wait();
+
+  scoped_refptr<net::TestURLRequestContextGetter> context_getter(

[eroman, 2017/01/27 02:23:24]

I would say this can move into SetUpTestContextWithQuicTimeout. Currently it
populates |context|, however that is transferred tot eh context_getter -- so
might as well have it just fill in the TestURLRequestContextGetter.

(It can obtain the current task runner since it will be running on network
thread).

[wangyix1, 2017/01/27 18:21:09]

Done.

+      new net::TestURLRequestContextGetter(net_thread_.task_runner(),
+                                           std::move(context)));
+
+  StartThenVerifyState(base::FilePath(), net::NetLogCaptureMode::Default(),
+                       kCaptureModeDefaultString);
+
+  StopThenVerifyStateAndFile(base::FilePath(), std::move(dummy_polled_data),
+                             context_getter, kCaptureModeDefaultString);
+
+  // Read polledData from log file.
+  std::unique_ptr<base::DictionaryValue> root;
+  ASSERT_TRUE(ReadCompleteLogFile(default_log_path_, &root));
+  base::DictionaryValue* polled_data;
+  ASSERT_TRUE(root->GetDictionary("polledData", &polled_data));
+
+  // Check that it contains the field from the polled data that was passed in.
+  std::string dummy_string;
+  ASSERT_TRUE(polled_data->GetString(kDummyPolledDataPath, &dummy_string));
+  EXPECT_EQ(dummy_string, kDummyPolledDataString);
+
+  // Check that it also contains the field from the URLRequestContext that was
+  // passed in.
+  base::DictionaryValue* quic_info;
+  ASSERT_TRUE(polled_data->GetDictionary("quicInfo", &quic_info));
+  base::Value* timeout_value = nullptr;
+  int timeout;
+  ASSERT_TRUE(
+      quic_info->Get("idle_connection_timeout_seconds", &timeout_value));
+  ASSERT_TRUE(timeout_value->GetAsInteger(&timeout));
+  EXPECT_EQ(timeout, kDummyQuicParam);
+}
+
+TEST_F(NetLogFileWriterTest, ReceiveStartWhileInitializing) {
+  // Trigger initialization of |net_log_file_writer_|.
+  TestStateCallback init_callback;
+  net_log_file_writer_.GetState(init_callback.callback());
+
+  // Before running the main message loop, tell |net_log_file_writer_| to start
+  // logging. Not running the main message loop prevents the initialization
+  // process from completing, so this ensures that StartNetLog() is received
+  // before |net_log_file_writer_| finishes initialization, which means this
+  // should be a no-op.
+  TestStateCallback start_during_init_callback;
+  net_log_file_writer_.StartNetLog(base::FilePath(),
+                                   net::NetLogCaptureMode::Default(),
+                                   start_during_init_callback.callback());
+
+  // Now run the main message loop. The state returned by the GetState() call
+  // should be the state after initialization, which should indicate
+  // not-logging.
+  VerifyState(init_callback.WaitForResult(), kStateNotLoggingString, false,
+              false, "");
+
+  // The state returned by the ignored StartNetLog() call should also be the
+  // state after the ongoing initialization finishes, so it should be identical
+  // to the state returned by GetState().
+  VerifyState(start_during_init_callback.WaitForResult(),
+              kStateNotLoggingString, false, false, "");
+
+  // Run an additional GetState() just to make sure |net_log_file_writer_| is
+  // not logging.
+  VerifyState(FileWriterGetState(), kStateNotLoggingString, false, false, "");
+}
+
+TEST_F(NetLogFileWriterTest, ReceiveStartWhileStoppingLog) {
+  // Call StartNetLog() on |net_log_file_writer_| and wait for it to run its
+  // state callback.
+  StartThenVerifyState(base::FilePath(),
+                       net::NetLogCaptureMode::IncludeSocketBytes(),
+                       kCaptureModeIncludeSocketBytesString);
+
+  // Tell |net_log_file_writer_| to stop logging.
+  TestStateCallback stop_callback;
+  net_log_file_writer_.StopNetLog(nullptr, nullptr, stop_callback.callback());
+
+  // Before running the main message loop, tell |net_log_file_writer_| to start
+  // logging. Not running the main message loop prevents the stopping process
+  // from completing, so this ensures StartNetLog() is received before
+  // |net_log_file_writer_| finishes stopping, which means this should be a
+  // no-op.
+  TestStateCallback start_during_stop_callback;
+  net_log_file_writer_.StartNetLog(base::FilePath(),
+                                   net::NetLogCaptureMode::Default(),
+                                   start_during_stop_callback.callback());
+
+  // Now run the main message loop. Since StartNetLog() will be a no-op, it will
+  // simply run the state callback. There are no guarantees for when this state
+  // callback will execute if StartNetLog() is called during the stopping
+  // process, so the state it returns could either be stopping-log or
+  // not-logging. For simplicity, the returned state will not be verified.
+  start_during_stop_callback.WaitForResult();
+
+  // The state returned by the StopNetLog() call should be the state after
+  // stopping, which should indicate not-logging. Also, the capture mode should
+  // be the same as the one passed to the first StartNetLog() call, not the
+  // second (ignored) one.
+  VerifyState(stop_callback.WaitForResult(), kStateNotLoggingString, true, true,
+              kCaptureModeIncludeSocketBytesString);
+
+  // Run an additional GetState() just to make sure |net_log_file_writer_| is
+  // not logging.
+  VerifyState(FileWriterGetState(), kStateNotLoggingString, true, true,
+              kCaptureModeIncludeSocketBytesString);
+}
+
 }  // namespace net_log