No dbus::FileDescriptor in DebugDaemonClient

chrome/browser/metrics/perf/perf_output.cc

 // Copyright 2016 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/metrics/perf/perf_output.h"
 
-#include <memory>
-
 #include "base/bind.h"
-#include "base/location.h"
-#include "base/task_runner_util.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 
-namespace {
-
-// Create a dbus::FileDescriptor from a base::File.
-dbus::ScopedFileDescriptor CreateFileDescriptor(base::File pipe_write_end) {
-  dbus::ScopedFileDescriptor file_descriptor(new dbus::FileDescriptor);
-  file_descriptor->PutValue(pipe_write_end.TakePlatformFile());
-  file_descriptor->CheckValidity();
-  return file_descriptor;
-}
-
-}  // namespace
-
 PerfOutputCall::PerfOutputCall(
     scoped_refptr<base::TaskRunner> blocking_task_runner,
     base::TimeDelta duration,
     const std::vector<std::string>& perf_args,
     const DoneCallback& callback)
     : blocking_task_runner_(blocking_task_runner),
       duration_(duration),
       perf_args_(perf_args),
       done_callback_(callback),
       weak_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   perf_data_pipe_reader_.reset(new chromeos::PipeReaderForString(
       blocking_task_runner_,
       base::Bind(&PerfOutputCall::OnIOComplete, weak_factory_.GetWeakPtr())));
 
-  base::File pipe_write_end = perf_data_pipe_reader_->StartIO();
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner_.get(), FROM_HERE,
-      base::Bind(&CreateFileDescriptor, base::Passed(&pipe_write_end)),
-      base::Bind(&PerfOutputCall::OnFileDescriptorCreated,
-                 weak_factory_.GetWeakPtr()));
+  base::ScopedFD pipe_write_end = perf_data_pipe_reader_->StartIO();

[Ilya Sherman, 2016/08/23 19:40:38]

FWIW, the only reason that I know what "FD" means in this context is because the
CL is replacing something that was named "FileDescriptor".  IMO it would be
great to rename "ScopedFD" for greater clarity.  (Not intended to block this CL,
just food for thought.)

[hashimoto, 2016/08/24 15:59:10]

It was unfortunate that ScopedFD was introduced with the current name 7 years
ago with almost no discussion in https://codereview.chromium.org/496007.
While I agree it's generally better to avoid unnecessary abbreviation, renaming
ScopedFD to, say, ScopedFileDescriptor would require some effort ("git grep
ScopedFD | wc -l" gives 516).
It's not impossible, but the cost will be a bit too large compared to the merit
IMHO.

BTW, the term "FD" frequently appears in man pages (e.g. man open) and other
POSIX documentations, so it might be acceptable to use this abbreviation as long
as it's in platform-specific code where the reader is expected to be familiar
with POSIX terms.

+

[Ilya Sherman, 2016/08/23 19:40:38]

Optional nit: I'd omit this newline.

[hashimoto, 2016/08/24 15:59:10]

Done.

+  chromeos::DebugDaemonClient* client =
+      chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
+

[Ilya Sherman, 2016/08/23 19:40:38]

Ditto

[hashimoto, 2016/08/24 15:59:10]

Done.

+  client->GetPerfOutput(duration_, perf_args_, pipe_write_end.get(),
+                        base::Bind(&PerfOutputCall::OnGetPerfOutputError,
+                                   weak_factory_.GetWeakPtr()));
 }
 
 PerfOutputCall::~PerfOutputCall() {}
 
-void PerfOutputCall::OnFileDescriptorCreated(
-    dbus::ScopedFileDescriptor file_descriptor) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(file_descriptor);
-
-  chromeos::DebugDaemonClient* client =
-      chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
-
-  client->GetPerfOutput(duration_, perf_args_, std::move(file_descriptor),
-                        base::Bind(&PerfOutputCall::OnGetPerfOutputError,
-                                   weak_factory_.GetWeakPtr()));
-}
-
 void PerfOutputCall::OnIOComplete() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   std::string stdout_data;
   perf_data_pipe_reader_->GetData(&stdout_data);
 
   done_callback_.Run(stdout_data);
   // The callback may delete us, so it's hammertime: Can't touch |this|.
 }
 
 void PerfOutputCall::OnGetPerfOutputError(const std::string& error_name,
                                           const std::string& error_message) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Signal pipe reader to shut down. This will cause
   // OnIOComplete to be called, probably with an empty string.
   perf_data_pipe_reader_->OnDataReady(-1);
 }