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

chrome/browser/ui/webui/net_export_ui.cc

Index: chrome/browser/ui/webui/net_export_ui.cc
diff --git a/chrome/browser/ui/webui/net_export_ui.cc b/chrome/browser/ui/webui/net_export_ui.cc
index d95a6e0fab7e8df294fe3e3db4a861ac079b7310..ae8ebf91234beec115f532e3c3627ff5c74b7e1c 100644
--- a/chrome/browser/ui/webui/net_export_ui.cc
+++ b/chrome/browser/ui/webui/net_export_ui.cc
@@ -31,6 +31,8 @@
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "content/public/browser/web_ui_message_handler.h"
+#include "net/log/net_log_util.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
@@ -85,20 +87,10 @@ class NetExportMessageHandler
   void FileSelectionCanceled(void* params) override;
 
  private:
-  // Calls NetLogFileWriter's ProcessCommand with DO_START and DO_STOP commands.
-  static void ProcessNetLogCommand(
-      base::WeakPtr<NetExportMessageHandler> net_export_message_handler,
-      net_log::NetLogFileWriter* net_log_file_writer,
-      net_log::NetLogFileWriter::Command command);
+  void StartNetLogAndNotifyUI(const base::FilePath& log_path,

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

I suggest naming this "StartNetLogThenNotifyUI"

(Replace And with Then to clarify that it happens sequentially rather than in
parallel)

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

Done.

+                              net::NetLogCaptureMode capture_mode);
 
-  // Returns the path to the file which has NetLog data.
-  static base::FilePath GetNetLogFileName(
-      net_log::NetLogFileWriter* net_log_file_writer);
-
-  // Send state/file information from NetLogFileWriter.
-  static void SendExportNetLogInfo(
-      base::WeakPtr<NetExportMessageHandler> net_export_message_handler,
-      net_log::NetLogFileWriter* net_log_file_writer);
+  void StopNetLogAndNotifyUI();

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

Same comment here.

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

Done.

 
   // Send NetLog data via email. This runs on UI thread.

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

The mention of "UI" thread is probably redundant, now that everything runs on UI
thread in here.

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

Done.

   static void SendEmail(const base::FilePath& file_to_send);
@@ -115,12 +107,10 @@ class NetExportMessageHandler
   // UI.
   static bool UsingMobileUI();
 
-  // Sets the correct start command and sends this to ProcessNetLogCommand.
-  void StartNetLog();
-
   // Call NetExportView.onExportNetLogInfoChanged JavsScript function in the

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

While you are editing this, could you change "Call" to "Calls" ?

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

Done.

-  // renderer, passing in |arg|. Takes ownership of |arg|.
-  void OnExportNetLogInfoChanged(base::Value* arg);
+  // renderer, passing in |file_writer_state|.
+  void NotifyUIWithNetLogFileWriterState(
+      std::unique_ptr<base::DictionaryValue> file_writer_state);
 
   // Opens the SelectFileDialog UI with the default path to save a
   // NetLog file.
@@ -138,7 +128,9 @@ class NetExportMessageHandler
   // base::Unretained.
   net_log::NetLogFileWriter* net_log_file_writer_;
 
-  std::string log_mode_;
+  // The capture mode the user chose in the UI when logging started is cached
+  // here.

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

Please explain the purpose of this variable, and its validity range (i.e. this
is only really valid while the file dialog is open on the desktop UI).

An alternative would be to pass the capture mode directly to SelectFile(), and
not need this member -- SelectFile takes a params pointer which it will pass
back to the delegate in FileSelected().

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

Done.

+  net::NetLogCaptureMode capture_mode_;
 
   scoped_refptr<ui::SelectFileDialog> select_file_dialog_;
 
@@ -149,7 +141,17 @@ class NetExportMessageHandler
 
 NetExportMessageHandler::NetExportMessageHandler()
     : net_log_file_writer_(g_browser_process->net_log()->net_log_file_writer()),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  if (!net_log_file_writer_->GetFileTaskRunner()) {

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

Since these will always be set in unison, how about unconditionally doing:

net_log_file_writer_->SetTaskRunners(....FILE_USER_BLOCKING, ....IO);

And inside of the implementation for SetTaskRunners(), if the task runners have
not been assigned yet it does so, otherwise it asserts that they are the same as
the ones passed in.

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

Done.

+    net_log_file_writer_->SetFileTaskRunner(
+        BrowserThread::GetTaskRunnerForThread(
+            BrowserThread::FILE_USER_BLOCKING));
+  }
+  if (!net_log_file_writer_->GetNetTaskRunner()) {
+    net_log_file_writer_->SetNetTaskRunner(
+        BrowserThread::GetTaskRunnerForThread(BrowserThread::IO));
+  }
+}
 
 NetExportMessageHandler::~NetExportMessageHandler() {
   // There may be a pending file dialog, it needs to be told that the user
@@ -157,11 +159,10 @@ NetExportMessageHandler::~NetExportMessageHandler() {
   if (select_file_dialog_.get())
     select_file_dialog_->ListenerDestroyed();
 
-  // Cancel any in-progress requests to collect net_log into a file.
-  BrowserThread::PostTask(BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-                          base::Bind(&net_log::NetLogFileWriter::ProcessCommand,
-                                     base::Unretained(net_log_file_writer_),
-                                     net_log::NetLogFileWriter::DO_STOP));
+  net_log_file_writer_->StopNetLog(
+      nullptr,
+      Profile::FromWebUI(web_ui())->GetRequestContext(),
+      base::Bind([](std::unique_ptr<base::DictionaryValue>) {}));
 }
 
 void NetExportMessageHandler::RegisterMessages() {
@@ -187,19 +188,29 @@ void NetExportMessageHandler::RegisterMessages() {
 
 void NetExportMessageHandler::OnGetExportNetLogInfo(
     const base::ListValue* list) {
-  BrowserThread::PostTask(
-      BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-      base::Bind(&NetExportMessageHandler::SendExportNetLogInfo,
-                 weak_ptr_factory_.GetWeakPtr(), net_log_file_writer_));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  net_log_file_writer_->GetState(
+    base::Bind(&NetExportMessageHandler::NotifyUIWithNetLogFileWriterState,
+               weak_ptr_factory_.GetWeakPtr()));
 }
 
 void NetExportMessageHandler::OnStartNetLog(const base::ListValue* list) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  bool result = list->GetString(0, &log_mode_);
+  std::string capture_mode_string;
+  bool result = list->GetString(0, &capture_mode_string);
   DCHECK(result);
 
+  if (capture_mode_string == "LOG_BYTES") {

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

Can we internalize the conversion from "capture mode string" --> capture mode in
the NetLogFileWriter?

 * NetLogFileWriter has code that does the inverse mapping (from capture mode to
string), so makes sense for the code to live side by side
  *  The iOS frontend will need to repeat this code if it lives in the message
handler.

Can either add a helper function, or make StartNetLog() take the parameter as as
string again.

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

Done.

+    capture_mode_ = net::NetLogCaptureMode::IncludeSocketBytes();
+  } else if (capture_mode_string == "NORMAL") {
+    capture_mode_ = net::NetLogCaptureMode::IncludeCookiesAndCredentials();
+  } else {
+    DCHECK_EQ("STRIP_PRIVATE_DATA", capture_mode_string);
+    capture_mode_ = net::NetLogCaptureMode::Default();
+  }
+
   if (UsingMobileUI()) {
-    StartNetLog();
+    StartNetLogAndNotifyUI(base::FilePath(), capture_mode_);
   } else {
     base::FilePath initial_dir = last_save_dir.Pointer()->empty() ?
         DownloadPrefs::FromBrowserContext(
@@ -212,76 +223,41 @@ void NetExportMessageHandler::OnStartNetLog(const base::ListValue* list) {
 }
 
 void NetExportMessageHandler::OnStopNetLog(const base::ListValue* list) {
-  ProcessNetLogCommand(weak_ptr_factory_.GetWeakPtr(), net_log_file_writer_,
-                       net_log::NetLogFileWriter::DO_STOP);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  StopNetLogAndNotifyUI();
 }
 
 void NetExportMessageHandler::OnSendNetLog(const base::ListValue* list) {
-  content::BrowserThread::PostTaskAndReplyWithResult(
-      content::BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-      base::Bind(&NetExportMessageHandler::GetNetLogFileName,
-                 base::Unretained(net_log_file_writer_)),
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  net_log_file_writer_->GetFilePath(
       base::Bind(&NetExportMessageHandler::SendEmail));
 }
 
-void NetExportMessageHandler::StartNetLog() {
-  net_log::NetLogFileWriter::Command command;
-  if (log_mode_ == "LOG_BYTES") {
-    command = net_log::NetLogFileWriter::DO_START_LOG_BYTES;
-  } else if (log_mode_ == "NORMAL") {
-    command = net_log::NetLogFileWriter::DO_START;
-  } else {
-    DCHECK_EQ("STRIP_PRIVATE_DATA", log_mode_);
-    command = net_log::NetLogFileWriter::DO_START_STRIP_PRIVATE_DATA;
-  }
+void NetExportMessageHandler::StartNetLogAndNotifyUI(
+    const base::FilePath& log_path,
+    net::NetLogCaptureMode capture_mode) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (!log_path.empty())
+    net_log_file_writer_->SetUpNetExportLogPath(log_path);

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

Rather than a separate method SetUpNetExportLogPath(), can you move this to a
parameter to the call below net_log_file_writer_->StartNetLog(...) ?

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

Done.

 
-  ProcessNetLogCommand(weak_ptr_factory_.GetWeakPtr(), net_log_file_writer_,
-                       command);
+  net_log_file_writer_->StartNetLog(capture_mode,
+      base::Bind(&NetExportMessageHandler::NotifyUIWithNetLogFileWriterState,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-// static
-void NetExportMessageHandler::ProcessNetLogCommand(
-    base::WeakPtr<NetExportMessageHandler> net_export_message_handler,
-    net_log::NetLogFileWriter* net_log_file_writer,
-    net_log::NetLogFileWriter::Command command) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING)) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-        base::Bind(&NetExportMessageHandler::ProcessNetLogCommand,
-                   net_export_message_handler, net_log_file_writer, command));
-    return;
-  }
+void NetExportMessageHandler::StopNetLogAndNotifyUI() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE_USER_BLOCKING);
-  net_log_file_writer->ProcessCommand(command);
-  SendExportNetLogInfo(net_export_message_handler, net_log_file_writer);
-}
+  std::unique_ptr<base::DictionaryValue> polled_data;
 
-// static
-base::FilePath NetExportMessageHandler::GetNetLogFileName(
-    net_log::NetLogFileWriter* net_log_file_writer) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE_USER_BLOCKING);
-  base::FilePath net_export_file_path;
-  net_log_file_writer->GetFilePath(&net_export_file_path);
-  return net_export_file_path;
-}
+  // fill polled_data with browser-specific polled data.

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

Please add a "TODO(crbug.com/438656)" here

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

Done.

 
-// static
-void NetExportMessageHandler::SendExportNetLogInfo(
-    base::WeakPtr<NetExportMessageHandler> net_export_message_handler,
-    net_log::NetLogFileWriter* net_log_file_writer) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE_USER_BLOCKING);
-  base::DictionaryValue* dict = net_log_file_writer->GetState();
-  dict->SetBoolean("useMobileUI", UsingMobileUI());
-  base::Value* value = dict;
-  if (!BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&NetExportMessageHandler::OnExportNetLogInfoChanged,
-                 net_export_message_handler,
-                 value))) {
-    // Failed posting the task, avoid leaking.
-    delete value;
-  }
+  net_log_file_writer_->StopNetLog(
+      std::move(polled_data),
+      Profile::FromWebUI(web_ui())->GetRequestContext(),
+      base::Bind(&NetExportMessageHandler::NotifyUIWithNetLogFileWriterState,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 // static
@@ -313,11 +289,12 @@ bool NetExportMessageHandler::UsingMobileUI() {
 #endif
 }
 
-void NetExportMessageHandler::OnExportNetLogInfoChanged(base::Value* arg) {
-  std::unique_ptr<base::Value> value(arg);
+void NetExportMessageHandler::NotifyUIWithNetLogFileWriterState(
+    std::unique_ptr<base::DictionaryValue> file_writer_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  file_writer_state->SetBoolean("useMobileUI", UsingMobileUI());
   web_ui()->CallJavascriptFunctionUnsafe(net_log::kOnExportNetLogInfoChanged,
-                                         *arg);
+                                         *file_writer_state);
 }
 
 void NetExportMessageHandler::ShowSelectFileDialog(
@@ -345,18 +322,9 @@ void NetExportMessageHandler::FileSelected(const base::FilePath& path,
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(select_file_dialog_);
   select_file_dialog_ = nullptr;
-
   *last_save_dir.Pointer() = path.DirName();
-  BrowserThread::PostTaskAndReply(
-      BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-      base::Bind(&net_log::NetLogFileWriter::SetUpNetExportLogPath,
-                 base::Unretained(net_log_file_writer_), path),
-      // NetExportMessageHandler is tied to the lifetime of the tab
-      // so it cannot be assured that it will be valid when this
-      // StartNetLog is called. Instead of using base::Unretained a
-      // weak pointer is used to adjust for this.
-      base::Bind(&NetExportMessageHandler::StartNetLog,
-                 weak_ptr_factory_.GetWeakPtr()));
+
+  StartNetLogAndNotifyUI(path, capture_mode_);
 }
 
 void NetExportMessageHandler::FileSelectionCanceled(void* params) {