Base: Introduce a new version of FileUtilProxy that works with File.

base/files/file_proxy.cc

Index: base/files/file_proxy.cc
diff --git a/base/files/file_util_proxy.cc b/base/files/file_proxy.cc
similarity index 16%
copy from base/files/file_util_proxy.cc
copy to base/files/file_proxy.cc
index 141e4e10606189969d8c27e5eac6d915d8e1e0fc..aa24e8af81089d7202093afa73555fbd064baeaa 100644
--- a/base/files/file_util_proxy.cc
+++ b/base/files/file_proxy.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/files/file_util_proxy.h"
+#include "base/files/file_proxy.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -15,147 +15,158 @@
 
 namespace base {
 
+class FileHelper {
+ public:
+   FileHelper(FileProxy* proxy, File file)
+      : file_(file.Pass()),
+        proxy_(AsWeakPtr(proxy)),
+        error_(File::FILE_ERROR_FAILED) {
+   }
+
+   void PassFile() {
+     if (proxy_)
+       proxy_->SetFile(file_.Pass());
+   }
+
+ protected:
+  File file_;
+  WeakPtr<FileProxy> proxy_;
+  File::Error error_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FileHelper);
+};
+
 namespace {
 
-void CallWithTranslatedParameter(const FileUtilProxy::StatusCallback& callback,
-                                 bool value) {
-  DCHECK(!callback.is_null());
-  callback.Run(value ? File::FILE_OK : File::FILE_ERROR_FAILED);
-}
+class GenericFileHelper : public FileHelper {
+ public:
+  GenericFileHelper(FileProxy* proxy, File file)
+      : FileHelper(proxy, file.Pass()) {
+  }
+
+  void Close() {
+    file_.Close();
+    error_ = File::FILE_OK;
+  }
+
+  void SetTimes(Time last_access_time, Time last_modified_time) {
+    bool rv = file_.SetTimes(last_access_time, last_modified_time);
+    error_ = rv ? File::FILE_OK : File::FILE_ERROR_FAILED;
+  }
 
-// Helper classes or routines for individual methods.
-class CreateOrOpenHelper {
+  void SetLength(int64 length) {
+    if (file_.SetLength(length))
+      error_ = File::FILE_OK;
+  }
+
+  void Flush() {
+    if (file_.Flush())
+      error_ = File::FILE_OK;
+  }
+
+  void Reply(const FileProxy::StatusCallback& callback) {
+    PassFile();
+    if (!callback.is_null())
+      callback.Run(error_);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GenericFileHelper);
+};
+
+class CreateOrOpenHelper : public FileHelper {
  public:
-  CreateOrOpenHelper(TaskRunner* task_runner,
-                     const FileUtilProxy::CloseTask& close_task)
-      : task_runner_(task_runner),
-        close_task_(close_task),
-        file_handle_(kInvalidPlatformFileValue),
-        created_(false),
-        error_(File::FILE_OK) {}
-
-  ~CreateOrOpenHelper() {
-    if (file_handle_ != kInvalidPlatformFileValue) {
-      task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(base::IgnoreResult(close_task_), file_handle_));
-    }
+  CreateOrOpenHelper(FileProxy* proxy, File file)
+      : FileHelper(proxy, file.Pass()) {
   }
 
-  void RunWork(const FileUtilProxy::CreateOrOpenTask& task) {
-    error_ = task.Run(&file_handle_, &created_);
+  void RunWork(const FilePath& file_path, int file_flags) {
+    file_.Initialize(file_path, file_flags);
+    error_ = file_.IsValid() ? File::FILE_OK : file_.error_details();
   }
 
-  void Reply(const FileUtilProxy::CreateOrOpenCallback& callback) {
+  void Reply(const FileProxy::StatusCallback& callback) {
     DCHECK(!callback.is_null());
-    callback.Run(error_, PassPlatformFile(&file_handle_), created_);
+    PassFile();
+    callback.Run(error_);
   }
 
  private:
-  scoped_refptr<TaskRunner> task_runner_;
-  FileUtilProxy::CloseTask close_task_;
-  PlatformFile file_handle_;
-  bool created_;
-  File::Error error_;
   DISALLOW_COPY_AND_ASSIGN(CreateOrOpenHelper);
 };
 
-class CreateTemporaryHelper {
+class CreateTemporaryHelper : public FileHelper {
  public:
-  explicit CreateTemporaryHelper(TaskRunner* task_runner)
-      : task_runner_(task_runner),
-        file_handle_(kInvalidPlatformFileValue),
-        error_(File::FILE_OK) {}
-
-  ~CreateTemporaryHelper() {
-    if (file_handle_ != kInvalidPlatformFileValue) {
-      FileUtilProxy::Close(
-          task_runner_.get(), file_handle_, FileUtilProxy::StatusCallback());
-    }
+  CreateTemporaryHelper(FileProxy* proxy, File file)
+      : FileHelper(proxy, file.Pass()) {
   }
 
-  void RunWork(int additional_file_flags) {
+  void RunWork(uint32 additional_file_flags) {
     // TODO(darin): file_util should have a variant of CreateTemporaryFile
-    // that returns a FilePath and a PlatformFile.
-    base::CreateTemporaryFile(&file_path_);
+    // that returns a FilePath and a File.
+    CreateTemporaryFile(&file_path_);
 
-    int file_flags =
-        PLATFORM_FILE_WRITE |
-        PLATFORM_FILE_TEMPORARY |
-        PLATFORM_FILE_CREATE_ALWAYS |
-        additional_file_flags;
+    uint32 file_flags = File::FLAG_WRITE |
+                        File::FLAG_TEMPORARY |
+                        File::FLAG_CREATE_ALWAYS |
+                        additional_file_flags;
 
-    error_ = File::FILE_OK;
-    // TODO(rvargas): Convert this code to use File.
-    file_handle_ =
-        CreatePlatformFile(file_path_, file_flags, NULL,
-                           reinterpret_cast<PlatformFileError*>(&error_));
+    file_.Initialize(file_path_, file_flags);
+    error_ = file_.IsValid() ? File::FILE_OK : file_.error_details();
   }
 
-  void Reply(const FileUtilProxy::CreateTemporaryCallback& callback) {
+  void Reply(const FileProxy::CreateTemporaryCallback& callback) {
     DCHECK(!callback.is_null());
-    callback.Run(error_, PassPlatformFile(&file_handle_), file_path_);
+    PassFile();
+    callback.Run(error_, file_path_);
   }
 
  private:
-  scoped_refptr<TaskRunner> task_runner_;
-  PlatformFile file_handle_;
   FilePath file_path_;
-  File::Error error_;
   DISALLOW_COPY_AND_ASSIGN(CreateTemporaryHelper);
 };
 
-class GetFileInfoHelper {
+class GetInfoHelper : public FileHelper {
  public:
-  GetFileInfoHelper()
-      : error_(File::FILE_OK) {}
-
-  void RunWorkForFilePath(const FilePath& file_path) {
-    if (!PathExists(file_path)) {
-      error_ = File::FILE_ERROR_NOT_FOUND;
-      return;
-    }
-    // TODO(rvargas): switch this file to base::File.
-    if (!GetFileInfo(file_path, reinterpret_cast<File::Info*>(&file_info_)))
-      error_ = File::FILE_ERROR_FAILED;
+  GetInfoHelper(FileProxy* proxy, File file)
+      : FileHelper(proxy, file.Pass()) {
   }
 
-  void RunWorkForPlatformFile(PlatformFile file) {
-    if (!GetPlatformFileInfo(
-            file, reinterpret_cast<PlatformFileInfo*>(&file_info_))) {
-      error_ = File::FILE_ERROR_FAILED;
-    }
+  void RunWork() {
+    if (file_.GetInfo(&file_info_))
+      error_  = File::FILE_OK;
   }
 
-  void Reply(const FileUtilProxy::GetFileInfoCallback& callback) {
-    if (!callback.is_null()) {
-      callback.Run(error_, file_info_);
-    }
+  void Reply(const FileProxy::GetFileInfoCallback& callback) {
+    PassFile();
+    DCHECK(!callback.is_null());
+    callback.Run(error_, file_info_);
   }
 
  private:
-  File::Error error_;
   File::Info file_info_;
-  DISALLOW_COPY_AND_ASSIGN(GetFileInfoHelper);
+  DISALLOW_COPY_AND_ASSIGN(GetInfoHelper);
 };
 
-class ReadHelper {
+class ReadHelper : public FileHelper {
  public:
-  explicit ReadHelper(int bytes_to_read)
-      : buffer_(new char[bytes_to_read]),
+  ReadHelper(FileProxy* proxy, File file, int bytes_to_read)
+      : FileHelper(proxy, file.Pass()),
+        buffer_(new char[bytes_to_read]),
         bytes_to_read_(bytes_to_read),
-        bytes_read_(0) {}
+        bytes_read_(0) {
+  }
 
-  void RunWork(PlatformFile file, int64 offset) {
-    bytes_read_ = ReadPlatformFile(file, offset, buffer_.get(), bytes_to_read_);
+  void RunWork(int64 offset) {
+    bytes_read_ = file_.Read(offset, buffer_.get(), bytes_to_read_);
+    error_ = (bytes_read_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK;
   }
 
-  void Reply(const FileUtilProxy::ReadCallback& callback) {
-    if (!callback.is_null()) {
-      File::Error error =
-          (bytes_read_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK;
-      callback.Run(error, buffer_.get(), bytes_read_);
-    }
+  void Reply(const FileProxy::ReadCallback& callback) {
+    PassFile();
+    DCHECK(!callback.is_null());
+    callback.Run(error_, buffer_.get(), bytes_read_);
   }
 
  private:
@@ -165,26 +176,27 @@ class ReadHelper {
   DISALLOW_COPY_AND_ASSIGN(ReadHelper);
 };
 
-class WriteHelper {
+class WriteHelper : public FileHelper {
  public:
-  WriteHelper(const char* buffer, int bytes_to_write)
-      : buffer_(new char[bytes_to_write]),
+  WriteHelper(FileProxy* proxy,
+              File file,
+              const char* buffer, int bytes_to_write)
+      : FileHelper(proxy, file.Pass()),
+        buffer_(new char[bytes_to_write]),
         bytes_to_write_(bytes_to_write),
         bytes_written_(0) {
     memcpy(buffer_.get(), buffer, bytes_to_write);
   }
 
-  void RunWork(PlatformFile file, int64 offset) {
-    bytes_written_ = WritePlatformFile(file, offset, buffer_.get(),
-                                       bytes_to_write_);
+  void RunWork(int64 offset) {
+    bytes_written_ = file_.Write(offset, buffer_.get(), bytes_to_write_);
+    error_ = (bytes_written_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK;
   }
 
-  void Reply(const FileUtilProxy::WriteCallback& callback) {
-    if (!callback.is_null()) {
-      File::Error error =
-          (bytes_written_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK;
-      callback.Run(error, bytes_written_);
-    }
+  void Reply(const FileProxy::WriteCallback& callback) {
+    PassFile();
+    if (!callback.is_null())
+      callback.Run(error_, bytes_written_);
   }
 
  private:
@@ -194,229 +206,120 @@ class WriteHelper {
   DISALLOW_COPY_AND_ASSIGN(WriteHelper);
 };
 
-File::Error CreateOrOpenAdapter(
-    const FilePath& file_path, int file_flags,
-    PlatformFile* file_handle, bool* created) {
-  DCHECK(file_handle);
-  DCHECK(created);
-  if (!DirectoryExists(file_path.DirName())) {
-    // If its parent does not exist, should return NOT_FOUND error.
-    return File::FILE_ERROR_NOT_FOUND;
-  }
-  File::Error error = File::FILE_OK;
-  *file_handle =
-      CreatePlatformFile(file_path, file_flags, created,
-                         reinterpret_cast<PlatformFileError*>(&error));
-  return error;
-}
+}  // namespace
 
-File::Error CloseAdapter(PlatformFile file_handle) {
-  if (!ClosePlatformFile(file_handle)) {
-    return File::FILE_ERROR_FAILED;
-  }
-  return File::FILE_OK;
+FileProxy::FileProxy() : task_runner_(NULL) {
 }
 
-File::Error DeleteAdapter(const FilePath& file_path, bool recursive) {
-  if (!PathExists(file_path)) {
-    return File::FILE_ERROR_NOT_FOUND;
-  }
-  if (!base::DeleteFile(file_path, recursive)) {
-    if (!recursive && !base::IsDirectoryEmpty(file_path)) {
-      return File::FILE_ERROR_NOT_EMPTY;
-    }
-    return File::FILE_ERROR_FAILED;
-  }
-  return File::FILE_OK;
+FileProxy::FileProxy(TaskRunner* task_runner) : task_runner_(task_runner) {
 }
 
-}  // namespace
+FileProxy::~FileProxy() {

[kinuko, 2014/03/03 05:58:58]

This may lead to close a file on non-blocking thread? Should we call Close() if
file_ is valid?

[rvargas (doing something else), 2014/03/03 20:38:34]

Correct. I think that would be an error that would be caught by the thread
restriction code. If it is not, it would mean that the operation was valid on
the current thread (maybe at shutdown, when the IO restrictions may be lifted).

The unit tests is an example of code that actually relies on the cleanup
happening when this object is destroyed, without proxying Close to another
thread (of course, could be changed to proxy the call).

Another option would be to assume automatic cleanup at destruction and remove
Close from the API, but that is a little complicated given that all the API
returns false when posting a task fails...

BTW, another complication is that it is valid to do

proxy.Write();
delete proxy;

and even if we check if the object is valid at destruction we will miss the
operation in progress and closing the file will happen on the main thread.

[kinuko, 2014/03/04 04:07:45]

Ok... not performing Close automatically in dtor also makes sense. Could we have
a comment about the behavior? Calling proxy.Write() and forgetting about proxy
looks like the easiest mistake we could make.

[rvargas (doing something else), 2014/03/04 23:53:53]

I added a comment to the class description.

+}
 
-// static
-bool FileUtilProxy::CreateOrOpen(
-    TaskRunner* task_runner,
-    const FilePath& file_path, int file_flags,
-    const CreateOrOpenCallback& callback) {
-  return RelayCreateOrOpen(
-      task_runner,
-      base::Bind(&CreateOrOpenAdapter, file_path, file_flags),
-      base::Bind(&CloseAdapter),
-      callback);
+bool FileProxy::CreateOrOpen(const FilePath& file_path,
+                             uint32 file_flags,
+                             const StatusCallback& callback) {
+  CreateOrOpenHelper* helper = new CreateOrOpenHelper(this, File());
+  return task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      Bind(&CreateOrOpenHelper::RunWork, Unretained(helper), file_path,
+           file_flags),
+      Bind(&CreateOrOpenHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::CreateTemporary(
-    TaskRunner* task_runner,
-    int additional_file_flags,
-    const CreateTemporaryCallback& callback) {
-  CreateTemporaryHelper* helper = new CreateTemporaryHelper(task_runner);
-  return task_runner->PostTaskAndReply(
+bool FileProxy::CreateTemporary(uint32 additional_file_flags,
+                                const CreateTemporaryCallback& callback) {
+  CreateTemporaryHelper* helper = new CreateTemporaryHelper(this, File());
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
       Bind(&CreateTemporaryHelper::RunWork, Unretained(helper),
            additional_file_flags),
       Bind(&CreateTemporaryHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::Close(
-    TaskRunner* task_runner,
-    base::PlatformFile file_handle,
-    const StatusCallback& callback) {
-  return RelayClose(
-      task_runner,
-      base::Bind(&CloseAdapter),
-      file_handle, callback);
+bool FileProxy::IsValid() const {
+  return file_.IsValid();
 }
 
-// Retrieves the information about a file. It is invalid to pass NULL for the
-// callback.
-bool FileUtilProxy::GetFileInfo(
-    TaskRunner* task_runner,
-    const FilePath& file_path,
-    const GetFileInfoCallback& callback) {
-  GetFileInfoHelper* helper = new GetFileInfoHelper;
-  return task_runner->PostTaskAndReply(
-      FROM_HERE,
-      Bind(&GetFileInfoHelper::RunWorkForFilePath,
-           Unretained(helper), file_path),
-      Bind(&GetFileInfoHelper::Reply, Owned(helper), callback));
+File FileProxy::TakeFile() {
+  return file_.Pass();
 }
 
-// static
-bool FileUtilProxy::GetFileInfoFromPlatformFile(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    const GetFileInfoCallback& callback) {
-  GetFileInfoHelper* helper = new GetFileInfoHelper;
-  return task_runner->PostTaskAndReply(
+bool FileProxy::Close(const StatusCallback& callback) {
+  GenericFileHelper* helper = new GenericFileHelper(this, file_.Pass());
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&GetFileInfoHelper::RunWorkForPlatformFile,
-           Unretained(helper), file),
-      Bind(&GetFileInfoHelper::Reply, Owned(helper), callback));
+      Bind(&GenericFileHelper::Close, Unretained(helper)),
+      Bind(&GenericFileHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::DeleteFile(TaskRunner* task_runner,
-                               const FilePath& file_path,
-                               bool recursive,
-                               const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner, FROM_HERE,
-      Bind(&DeleteAdapter, file_path, recursive),
-      callback);
+bool FileProxy::GetInfo(const GetFileInfoCallback& callback) {
+  GetInfoHelper* helper = new GetInfoHelper(this, file_.Pass());
+  return task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      Bind(&GetInfoHelper::RunWork, Unretained(helper)),
+      Bind(&GetInfoHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::Read(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    int64 offset,
-    int bytes_to_read,
-    const ReadCallback& callback) {
-  if (bytes_to_read < 0) {
+bool FileProxy::Read(int64 offset,
+                     int bytes_to_read,
+                     const ReadCallback& callback) {
+  if (bytes_to_read < 0)
     return false;
-  }
-  ReadHelper* helper = new ReadHelper(bytes_to_read);
-  return task_runner->PostTaskAndReply(
+
+  ReadHelper* helper = new ReadHelper(this, file_.Pass(), bytes_to_read);
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&ReadHelper::RunWork, Unretained(helper), file, offset),
+      Bind(&ReadHelper::RunWork, Unretained(helper), offset),
       Bind(&ReadHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::Write(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    int64 offset,
-    const char* buffer,
-    int bytes_to_write,
-    const WriteCallback& callback) {
-  if (bytes_to_write <= 0 || buffer == NULL) {
+bool FileProxy::Write(int64 offset,
+                      const char* buffer,
+                      int bytes_to_write,
+                      const WriteCallback& callback) {
+  if (bytes_to_write <= 0 || buffer == NULL)
     return false;
-  }
-  WriteHelper* helper = new WriteHelper(buffer, bytes_to_write);
-  return task_runner->PostTaskAndReply(
-      FROM_HERE,
-      Bind(&WriteHelper::RunWork, Unretained(helper), file, offset),
-      Bind(&WriteHelper::Reply, Owned(helper), callback));
-}
-
-// static
-bool FileUtilProxy::Touch(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    const Time& last_access_time,
-    const Time& last_modified_time,
-    const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner,
-      FROM_HERE,
-      Bind(&TouchPlatformFile, file,
-           last_access_time, last_modified_time),
-      Bind(&CallWithTranslatedParameter, callback));
-}
 
-// static
-bool FileUtilProxy::Touch(
-    TaskRunner* task_runner,
-    const FilePath& file_path,
-    const Time& last_access_time,
-    const Time& last_modified_time,
-    const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner,
+  WriteHelper* helper =
+      new WriteHelper(this, file_.Pass(), buffer, bytes_to_write);
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&TouchFile, file_path, last_access_time, last_modified_time),
-      Bind(&CallWithTranslatedParameter, callback));
+      Bind(&WriteHelper::RunWork, Unretained(helper), offset),
+      Bind(&WriteHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::Truncate(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    int64 length,
-    const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner,
+bool FileProxy::SetTimes(Time last_access_time,
+                         Time last_modified_time,
+                         const StatusCallback& callback) {
+  GenericFileHelper* helper = new GenericFileHelper(this, file_.Pass());
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&TruncatePlatformFile, file, length),
-      Bind(&CallWithTranslatedParameter, callback));
+      Bind(&GenericFileHelper::SetTimes, Unretained(helper), last_access_time,
+           last_modified_time),
+      Bind(&GenericFileHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::Flush(
-    TaskRunner* task_runner,
-    PlatformFile file,
-    const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner,
+bool FileProxy::SetLength(int64 length, const StatusCallback& callback) {
+  GenericFileHelper* helper = new GenericFileHelper(this, file_.Pass());
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&FlushPlatformFile, file),
-      Bind(&CallWithTranslatedParameter, callback));
+      Bind(&GenericFileHelper::SetLength, Unretained(helper), length),
+      Bind(&GenericFileHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::RelayCreateOrOpen(
-    TaskRunner* task_runner,
-    const CreateOrOpenTask& open_task,
-    const CloseTask& close_task,
-    const CreateOrOpenCallback& callback) {
-  CreateOrOpenHelper* helper = new CreateOrOpenHelper(
-      task_runner, close_task);
-  return task_runner->PostTaskAndReply(
+bool FileProxy::Flush(const StatusCallback& callback) {
+  GenericFileHelper* helper = new GenericFileHelper(this, file_.Pass());
+  return task_runner_->PostTaskAndReply(
       FROM_HERE,
-      Bind(&CreateOrOpenHelper::RunWork, Unretained(helper), open_task),
-      Bind(&CreateOrOpenHelper::Reply, Owned(helper), callback));
+      Bind(&GenericFileHelper::Flush, Unretained(helper)),
+      Bind(&GenericFileHelper::Reply, Owned(helper), callback));
 }
 
-// static
-bool FileUtilProxy::RelayClose(
-    TaskRunner* task_runner,
-    const CloseTask& close_task,
-    PlatformFile file_handle,
-    const StatusCallback& callback) {
-  return base::PostTaskAndReplyWithResult(
-      task_runner, FROM_HERE, Bind(close_task, file_handle), callback);
+void FileProxy::SetFile(File file) {
+  DCHECK(!file_.IsValid());
+  file_ = file.Pass();
 }
 
 }  // namespace base