Use base::File for DebugDaemonClient::GetDebugLogs

chrome/browser/ui/webui/net_internals/net_internals_ui.cc

Index: chrome/browser/ui/webui/net_internals/net_internals_ui.cc
diff --git a/chrome/browser/ui/webui/net_internals/net_internals_ui.cc b/chrome/browser/ui/webui/net_internals/net_internals_ui.cc
index 05b4db31433559ce526679f222279d843f1b526a..adb40bc54432ffa8ca2c04d458988b49f54d8f79 100644
--- a/chrome/browser/ui/webui/net_internals/net_internals_ui.cc
+++ b/chrome/browser/ui/webui/net_internals/net_internals_ui.cc
@@ -27,7 +27,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
-#include "base/threading/worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
@@ -209,48 +208,6 @@ content::WebUIDataSource* CreateNetInternalsHTMLSource() {
 }
 
 #if defined(OS_CHROMEOS)
-// Small helper class used to create temporary log file and pass its
-// handle and error status to callback.
-// Use case:
-// DebugLogFileHelper* helper = new DebugLogFileHelper();
-// base::WorkerPool::PostTaskAndReply(FROM_HERE,
-//     base::Bind(&DebugLogFileHelper::DoWork, base::Unretained(helper), ...),
-//     base::Bind(&DebugLogFileHelper::Reply, base::Owned(helper), ...),
-//     false);
-class DebugLogFileHelper {
- public:
-  typedef base::Callback<void(base::File file,
-                              const base::FilePath& file_path)>
-      DebugLogFileCallback;
-
-  DebugLogFileHelper() {}
-
-  ~DebugLogFileHelper() {}
-
-  void DoWork(const base::FilePath& fileshelf) {
-    const base::FilePath::CharType kLogFileName[] =
-        FILE_PATH_LITERAL("debug-log.tgz");
-
-    file_path_ = fileshelf.Append(kLogFileName);
-    file_path_ = logging::GenerateTimestampedName(file_path_,
-                                                  base::Time::Now());
-
-    int flags =  base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
-    file_.Initialize(file_path_, flags);
-  }
-
-  void Reply(const DebugLogFileCallback& callback) {
-    DCHECK(!callback.is_null());
-    callback.Run(file_.Pass(), file_path_);
-  }
-
- private:
-  base::File file_;
-  base::FilePath file_path_;
-
-  DISALLOW_COPY_AND_ASSIGN(DebugLogFileHelper);
-};
-
 // Following functions are used for getting debug logs. Logs are
 // fetched from /var/log/* and put on the fileshelf.
 
@@ -260,56 +217,39 @@ class DebugLogFileHelper {
 typedef base::Callback<void(const base::FilePath& log_path,
                             bool succeded)> StoreDebugLogsCallback;
 
-// Closes file handle, so, should be called on the WorkerPool thread.
-void CloseDebugLogFile(base::File file) {
-  file.Close();
-}
-
-// Closes file handle and deletes debug log file, so, should be called
-// on the WorkerPool thread.
-void CloseAndDeleteDebugLogFile(base::File file,
-                                const base::FilePath& file_path) {
-  file.Close();
-  base::DeleteFile(file_path, false);
-}
-
 // Called upon completion of |WriteDebugLogToFile|. Closes file
 // descriptor, deletes log file in the case of failure and calls
 // |callback|.
 void WriteDebugLogToFileCompleted(const StoreDebugLogsCallback& callback,
-                                  base::File file,
                                   const base::FilePath& file_path,
                                   bool succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!succeeded) {
-    bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-        base::Bind(&CloseAndDeleteDebugLogFile, Passed(&file), file_path),
-        base::Bind(callback, file_path, false), false);
+    bool posted = BrowserThread::PostBlockingPoolTaskAndReply(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&base::DeleteFile), file_path, false),
+        base::Bind(callback, file_path, false));
     DCHECK(posted);
     return;
   }
-  bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-      base::Bind(&CloseDebugLogFile, Passed(&file)),
-      base::Bind(callback, file_path, true), false);
-  DCHECK(posted);
+  callback.Run(file_path, true);
 }
 
 // Stores into |file_path| debug logs in the .tgz format. Calls
 // |callback| upon completion.
 void WriteDebugLogToFile(const StoreDebugLogsCallback& callback,
-                         base::File file,
+                         base::File* file,
                          const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!file.IsValid()) {
+  if (!file->IsValid()) {
     LOG(ERROR) <<
         "Can't create debug log file: " << file_path.AsUTF8Unsafe() << ", " <<
-        "error: " << file.error_details();
+        "error: " << file->error_details();
     return;
   }
   chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->GetDebugLogs(
-      file.GetPlatformFile(),
-      base::Bind(&WriteDebugLogToFileCompleted,
-                 callback, Passed(&file), file_path));
+      file->Pass(),

[mmenke, 2014/05/08 14:29:34]

Erm...I don't see Pass() defined anywhere for a base::File.

[hashimoto, 2014/05/08 15:13:12]

Like scoped_ptr, base::File contains MOVE_ONLY_TYPE_FOR_CPP_03 in its class
definition so it comes with Pass() method somehow implicitly.

[mmenke, 2014/05/08 15:43:50]

I'm having trouble understanding their explanation, but it looks to me like
we're leaking the file* (Though we are moving its internals correctly).  It
should be passed as a base::Owned, not base::Unretained, no?

[hashimoto, 2014/05/08 15:59:51]

StoreDebugLogs() below runs base::File::Initialize() on the blocking pool with
base::Unretained and runs WriteDebugLogToFile() on the UI thread with the
initialized base::File passed with base::Owned.
base::File object is kept alive until WriteDebugLogToFile() finishes and it gets
correctly destroyed after that.

+      base::Bind(&WriteDebugLogToFileCompleted, callback, file_path));
 }
 
 // Stores debug logs in the .tgz archive on the |fileshelf|. The file
@@ -320,12 +260,20 @@ void StoreDebugLogs(const base::FilePath& fileshelf,
                     const StoreDebugLogsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!callback.is_null());
-  DebugLogFileHelper* helper = new DebugLogFileHelper();
-  bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-      base::Bind(&DebugLogFileHelper::DoWork,
-          base::Unretained(helper), fileshelf),
-      base::Bind(&DebugLogFileHelper::Reply, base::Owned(helper),
-          base::Bind(&WriteDebugLogToFile, callback)), false);
+
+  const base::FilePath::CharType kLogFileName[] =
+      FILE_PATH_LITERAL("debug-log.tgz");
+
+  base::FilePath file_path = fileshelf.Append(kLogFileName);
+  file_path = logging::GenerateTimestampedName(file_path, base::Time::Now());
+
+  int flags =  base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
+  base::File* file = new base::File;
+  bool posted = BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&base::File::Initialize,
+                 base::Unretained(file), file_path, flags),
+      base::Bind(&WriteDebugLogToFile, callback, base::Owned(file), file_path));
   DCHECK(posted);
 }
 #endif  // defined(OS_CHROMEOS)