Fixed GetDebugLogs functionality.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/net_internals/net_internals_ui.h"
 
 #include <algorithm>
 #include <list>
 #include <string>
 #include <utility>
@@ skipping 148 matching lines @@
 #ifdef OS_CHROMEOS
 // Following functions are used for getting debug logs. Logs are
 // fetched from /var/log/* and put on the fileshelf.
 
 // Called once StoreDebugLogs is complete. Takes two parameters:
 // - log_path: where the log file was saved in the case of success;
 // - succeeded: was the log file saved successfully.
 typedef base::Callback<void(const FilePath& log_path,
                             bool succeded)> StoreDebugLogsCallback;
 
+// Deletes debug log file, so should be called on the FILE thread.
+void DeleteDebugLogFile(const FilePath& log_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  file_util::Delete(log_path, false);
+}
+
 // Called once creation of the debug log file is completed. If
-// creation failed, deletes the file by |log_path|. So, this function
-// should be called on the FILE thread. After all, calls |callback| on
-// the UI thread.
+// creation failed, deletes the file by |log_path|. After all, calls
+// |callback| on the UI thread.
 void CreateDebugLogFileCompleted(const StoreDebugLogsCallback& callback,
                                  const FilePath& log_path,
                                  bool succeeded) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  if (!succeeded)
-    file_util::Delete(log_path, false);
-
+  if (!succeeded) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                            base::Bind(&DeleteDebugLogFile, log_path));
+  }
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(callback, log_path, succeeded));
 }
 
 // Retrieves debug logs from DebugDaemon and puts them on the
 // fileshelf directory into .tgz archive. So, this function should be
 // called on the FILE thread. Calls CreateDebugLogFileCompleted on the
 // FILE thread when creation of archive is completed.
 void CreateDebugLogFile(const StoreDebugLogsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   const FilePath::CharType kLogFileName[] = FILE_PATH_LITERAL("debug-log.tgz");
 
   FilePath fileshelf_dir;
-  if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &fileshelf_dir)) {
+  if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS_SAFE, &fileshelf_dir)) {

[Sam Leffler, 2012/05/14 15:52:38]

should this be download_util::GetDefaultDownloadDirectory()?

[ygorshenin1, 2012/05/15 12:46:20]

Done.

     LOG(ERROR) << "Can't get fileshelf dir";
     CreateDebugLogFileCompleted(callback, FilePath(), false);
     return;
   }
 
   FilePath log_path = fileshelf_dir.Append(kLogFileName);
   log_path = logging::GenerateTimestampedName(log_path, base::Time::Now());
 
   int flags = base::PLATFORM_FILE_CREATE_ALWAYS |
       base::PLATFORM_FILE_WRITE;
   bool created;
   PlatformFileError error;
   PlatformFile log_file = base::CreatePlatformFile(
       log_path, flags, &created, &error);

[Sam Leffler, 2012/05/14 15:52:38]

feels like this belongs in download_util (just a comment)

 
   if (!created) {
     LOG(ERROR) <<
         "Can't create log file: " << log_path.AsUTF8Unsafe() << ", " <<
         "error: " << error;
     CreateDebugLogFileCompleted(callback, log_path, false);
     return;
   }
-  chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->
-      GetDebugLogs(log_file,
-                   base::Bind(&CreateDebugLogFileCompleted,
-                              callback, log_path));
+
+  chromeos::DebugDaemonClient* client =
+      chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&chromeos::DebugDaemonClient::GetDebugLogs,
+                 base::Unretained(client),
+                 log_file,
+                 base::Bind(&CreateDebugLogFileCompleted,
+                            callback,
+                            log_path)));
 }
 
 // Delegates the job of saving debug logs on the fileshelf to
 // CreateDebugLogsFile on the FILE thread. Calls |callback| on the UI
 // thread when saving is completed. This function should be called on
 // the UI thread.
 void StoreDebugLogs(const StoreDebugLogsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   BrowserThread::PostTask(

[Sam Leffler, 2012/05/14 15:52:38]

I think it's cleaner to make this a call to BrowserThread::PostTaskAndReply as
it eliminates the explicit return hand-off to the UI thread; doing this might
also allow you to move much of this code to debug_daemon_client.cc

It was also my understanding that it was desirable to not explicitly push file
ops to the FILE thread but instead send them to the WorkerPool (as I did for my
equivalent changes); it should be simple to do that

[ygorshenin1, 2012/05/15 12:46:20]

Thanks! It's a cool idea to use WorkerPool for file ops. It requires a helper
class, but code became more readable and understandable.

On 2012/05/14 15:52:38, Sam Leffler wrote:

       BrowserThread::FILE, FROM_HERE,
       base::Bind(&CreateDebugLogFile, callback));
 }
 #endif  // OS_CHROMEOS
 
 // This class receives javascript messages from the renderer.
 // Note that the WebUI infrastructure runs on the UI thread, therefore all of
 // this class's methods are expected to run on the UI thread.
 //
 // Since the network code we want to run lives on the IO thread, we proxy
@@ skipping 1428 matching lines @@
 }
 
 NetInternalsUI::NetInternalsUI(content::WebUI* web_ui)
     : WebUIController(web_ui) {
   web_ui->AddMessageHandler(new NetInternalsMessageHandler());
 
   // Set up the chrome://net-internals/ source.
   Profile* profile = Profile::FromWebUI(web_ui);
   ChromeURLDataManager::AddDataSource(profile, CreateNetInternalsHTMLSource());
 }