No dbus::FileDescriptor in DebugDaemonClient

chromeos/dbus/debug_daemon_client.h

Index: chromeos/dbus/debug_daemon_client.h
diff --git a/chromeos/dbus/debug_daemon_client.h b/chromeos/dbus/debug_daemon_client.h
index 803500d489e26504c2bc678e1b8f87be98bc084a..c9b3932a69b227390d7ccdb208b989dc8110436a 100644
--- a/chromeos/dbus/debug_daemon_client.h
+++ b/chromeos/dbus/debug_daemon_client.h
@@ -17,7 +17,6 @@
 #include "base/trace_event/tracing_agent.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_client.h"
-#include "dbus/file_descriptor.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
@@ -33,12 +32,11 @@ class CHROMEOS_EXPORT DebugDaemonClient
   // - succeeded: was the logs stored successfully.
   typedef base::Callback<void(bool succeeded)> GetDebugLogsCallback;
 
-  // Requests to store debug logs into |file| and calls |callback|
+  // Requests to store debug logs into |file_descriptor| and calls |callback|
   // when completed. Debug logs will be stored in the .tgz if
   // |is_compressed| is true, otherwise in logs will be stored in .tar format.
   virtual void DumpDebugLogs(bool is_compressed,
-                             base::File file,
-                             scoped_refptr<base::TaskRunner> task_runner,
+                             int file_descriptor,

[satorux1, 2016/08/23 04:43:37]

How about taking ownership of the file descriptor as base::File? I thought
that'd make ownership clearer.

[hashimoto, 2016/08/23 06:38:50]

Here we want to avoid owning the given FD because there is no need to do it [1]
and it can result in UI jank [2].

1. There is no need to own the FD because appending a FD to a D-Bus message
doesn't require ownership as dbus_message_iter_append_basic() duplicates the FD.

2. The user code (debug_log_writer.cc) passes a FD of a regular file which can
cause UI jank when being closed on the UI thread, instead of a pipe FD, to this
method. From DebugDaemonClient's point of view, there is no way to know if it's
OK to close the given FD on the UI thread.

[satorux1, 2016/08/23 07:02:02]

Makes sense. In that case, could you add a comment that says this function does
not close |file_descriptor|?

[hashimoto, 2016/08/23 07:41:27]

Added a comment about FD duplication.

                              const GetDebugLogsCallback& callback) = 0;
 
   // Called once SetDebugMode() is complete. Takes one parameter:
@@ -102,7 +100,7 @@ class CHROMEOS_EXPORT DebugDaemonClient
   // no data written.
   virtual void GetPerfOutput(base::TimeDelta duration,
                              const std::vector<std::string>& perf_args,
-                             dbus::ScopedFileDescriptor file_descriptor,
+                             int file_descriptor,

[satorux1, 2016/08/23 04:43:37]

ditto. maybe take ownership?

[hashimoto, 2016/08/23 06:38:50]

Acknowledged.

[satorux1, 2016/08/23 07:02:02]

ditto. a comment about file_descriptor ownership would be helpful here too.

[hashimoto, 2016/08/23 07:41:27]

Done.

                              const DBusMethodErrorCallback& error_callback) = 0;
 
   // Callback type for GetScrubbedLogs(), GetAllLogs() or GetUserLogFiles().