Remove PlatformFile from fileapi::FileSystemFileUtil

webkit/browser/fileapi/obfuscated_file_util.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 "webkit/browser/fileapi/obfuscated_file_util.h"
 
 #include <queue>
 #include <string>
 #include <vector>
 
@@ skipping 87 matching lines @@
 }
 
 enum IsolatedOriginStatus {
   kIsolatedOriginMatch,
   kIsolatedOriginDontMatch,
   kIsolatedOriginStatusMax,
 };
 
 }  // namespace
 
-using base::PlatformFile;
-
 class ObfuscatedFileEnumerator
     : public FileSystemFileUtil::AbstractFileEnumerator {
  public:
   ObfuscatedFileEnumerator(
       SandboxDirectoryDatabase* db,
       FileSystemOperationContext* context,
       ObfuscatedFileUtil* obfuscated_file_util,
       const FileSystemURL& root_url,
       bool recursive)
       : db_(db),
@@ skipping 146 matching lines @@
       file_task_runner_(file_task_runner),
       get_type_string_for_url_(get_type_string_for_url),
       known_type_strings_(known_type_strings),
       sandbox_delegate_(sandbox_delegate) {
 }
 
 ObfuscatedFileUtil::~ObfuscatedFileUtil() {
   DropDatabases();
 }
 
-base::File::Error ObfuscatedFileUtil::CreateOrOpen(
+base::File ObfuscatedFileUtil::CreateOrOpen(
     FileSystemOperationContext* context,
-    const FileSystemURL& url, int file_flags,
-    PlatformFile* file_handle, bool* created) {
-  base::File::Error error = CreateOrOpenInternal(context, url, file_flags,
-                                                 file_handle, created);
-  if (*file_handle != base::kInvalidPlatformFileValue &&
-      file_flags & base::PLATFORM_FILE_WRITE &&
+    const FileSystemURL& url, int file_flags) {
+  base::File file = CreateOrOpenInternal(context, url, file_flags);
+  if (file.IsValid() && file_flags & base::PLATFORM_FILE_WRITE &&
       context->quota_limit_type() == quota::kQuotaLimitTypeUnlimited &&
       sandbox_delegate_) {
-    DCHECK_EQ(base::File::FILE_OK, error);
     sandbox_delegate_->StickyInvalidateUsageCache(url.origin(), url.type());
   }
-  return error;
-}
-
-base::File::Error ObfuscatedFileUtil::Close(
-    FileSystemOperationContext* context,
-    base::PlatformFile file) {
-  base::File auto_closed(file);
-  return base::File::FILE_OK;
+  return file.Pass();
 }
 
 base::File::Error ObfuscatedFileUtil::EnsureFileExists(
     FileSystemOperationContext* context,
     const FileSystemURL& url,
     bool* created) {
   SandboxDirectoryDatabase* db = GetDirectoryDatabase(url, true);
   if (!db)
     return base::File::FILE_ERROR_FAILED;
 
@@ skipping 14 matching lines @@
   if (!db->GetFileWithPath(VirtualPath::DirName(url.path()), &parent_id))
     return base::File::FILE_ERROR_NOT_FOUND;
 
   FileInfo file_info;
   InitFileInfo(&file_info, parent_id,
                VirtualPath::BaseName(url.path()).value());
 
   int64 growth = UsageForPath(file_info.name.size());
   if (!AllocateQuota(context, growth))
     return base::File::FILE_ERROR_NO_SPACE;
-  base::File::Error error = CreateFile(
-      context, base::FilePath(), url, &file_info, 0, NULL);
+  base::File::Error error = CreateFile(context, base::FilePath(), url,
+                                       &file_info);
   if (created && base::File::FILE_OK == error) {
     *created = true;
     UpdateUsage(context, url, growth);
     context->change_observers()->Notify(
         &FileChangeObserver::OnCreateFile, MakeTuple(url));
   }
   return error;
 }
 
 base::File::Error ObfuscatedFileUtil::CreateDirectory(
@@ skipping 245 matching lines @@
   error = base::File::FILE_ERROR_FAILED;
   if (copy) {
     if (overwrite) {
       error = NativeFileUtil::CopyOrMoveFile(
           src_local_path,
           dest_local_path,
           option,
           fileapi::NativeFileUtil::CopyOrMoveModeForDestination(
               dest_url, true /* copy */));
     } else {  // non-overwrite
-      error = CreateFile(context, src_local_path,
-                         dest_url, &dest_file_info, 0, NULL);
+      error = CreateFile(context, src_local_path, dest_url, &dest_file_info);
     }
   } else {
     if (overwrite) {
       if (db->OverwritingMoveFile(src_file_id, dest_file_id)) {
         if (base::File::FILE_OK !=
             NativeFileUtil::DeleteFile(dest_local_path))
           LOG(WARNING) << "Leaked a backing file.";
         error = base::File::FILE_OK;
       } else {
         error = base::File::FILE_ERROR_FAILED;
@@ skipping 84 matching lines @@
   base::File::Error error;
   if (overwrite) {
     base::FilePath dest_local_path =
         DataPathToLocalPath(dest_url, dest_file_info.data_path);
     error = NativeFileUtil::CopyOrMoveFile(
         src_file_path, dest_local_path,
         FileSystemOperation::OPTION_NONE,
         fileapi::NativeFileUtil::CopyOrMoveModeForDestination(dest_url,
                                                               true /* copy */));
   } else {
-    error = CreateFile(context, src_file_path,
-                       dest_url, &dest_file_info, 0, NULL);
+    error = CreateFile(context, src_file_path, dest_url, &dest_file_info);
   }
 
   if (error != base::File::FILE_OK)
     return error;
 
   if (overwrite) {
     context->change_observers()->Notify(
         &FileChangeObserver::OnModifyFile, MakeTuple(dest_url));
   } else {
     context->change_observers()->Notify(
@@ skipping 327 matching lines @@
     *platform_file_path = local_path;
   } else if (error == base::File::FILE_ERROR_NOT_FOUND) {
     LOG(WARNING) << "Lost a backing file.";
     InvalidateUsageCache(context, url.origin(), url.type());
     if (!db->RemoveFileInfo(file_id))
       return base::File::FILE_ERROR_FAILED;
   }
   return error;
 }
 
+base::File::Error ObfuscatedFileUtil::GetDbRootAndLocalPath(
+    FileSystemOperationContext* context,
+    const FileSystemURL& dest_url,
+    SandboxDirectoryDatabase** db,
+    base::FilePath* root,
+    base::FilePath* local_path) {
+  *db = GetDirectoryDatabase(dest_url, true);

[kinuko, 2014/05/12 05:39:57]

I'd probably remove this part from this method but just take *db as input param,
this method seems to be doing too many things that are not strongly related. 
Also... this looks to be the only callsite of GenerateNewLocalPath, so probably
it (GenerateNewLocalPath) could be extended to getting the root directory part
too, i.e.:

SandboxDirectoryDatabase* db = GetDirectoryDatabase(dest_url, true);
GenerateNewLocalPath(db, context, dest_url, &root, local_path);

[rvargas (doing something else), 2014/05/12 23:39:59]

Done.

+
+  base::File::Error error = base::File::FILE_OK;
+  *root = GetDirectoryForURL(dest_url, false, &error);
+  if (error != base::File::FILE_OK)
+    return error;
+
+  return GenerateNewLocalPath(*db, context, dest_url, local_path);
+}
+
+base::File::Error ObfuscatedFileUtil::PostCreateFile(

[kinuko, 2014/05/12 05:39:57]

The term 'Post' is a bit confusing (we often use it to refer to posting a task),
CommitCreateFile() maybe?

Also since this is basically to update the DB related code, I'd put *db param
first.

[rvargas (doing something else), 2014/05/12 23:39:59]

Done. (db is now the first in_out argument)

+    const base::FilePath& root,
+    const base::FilePath& local_path,
+    FileInfo* dest_file_info,
+    SandboxDirectoryDatabase* db) {
+  // This removes the root, including the trailing slash, leaving a relative
+  // path.
+  dest_file_info->data_path = base::FilePath(
+      local_path.value().substr(root.value().length() + 1));
+
+  FileId file_id;
+  base::File::Error error = db->AddFileInfo(*dest_file_info, &file_id);
+  if (error != base::File::FILE_OK)
+    return error;
+
+  TouchDirectory(db, dest_file_info->parent_id);
+  return base::File::FILE_OK;
+}
+
+base::File ObfuscatedFileUtil::CreateAndOpenFile(
+    FileSystemOperationContext* context,
+    const FileSystemURL& dest_url,
+    FileInfo* dest_file_info, int file_flags) {
+  SandboxDirectoryDatabase* db;
+  base::FilePath root, dest_local_path;
+  base::File::Error error = GetDbRootAndLocalPath(context, dest_url, &db, &root,
+                                                  &dest_local_path);
+  if (error != base::File::FILE_OK)
+    return base::File(error);
+
+  if (base::PathExists(dest_local_path)) {
+    if (!base::DeleteFile(dest_local_path, true /* recursive */))
+      return base::File(base::File::FILE_ERROR_FAILED);
+    LOG(WARNING) << "A stray file detected";
+    InvalidateUsageCache(context, dest_url.origin(), dest_url.type());
+  }
+
+  base::File file = NativeFileUtil::CreateOrOpen(dest_local_path, file_flags);
+  if (!file.IsValid())
+    return file.Pass();
+
+  if (!file.created()) {
+    file.Close();
+    base::DeleteFile(dest_local_path, false /* recursive */);
+    return base::File(base::File::FILE_ERROR_FAILED);
+  }
+
+  error = PostCreateFile(root, dest_local_path, dest_file_info, db);
+  if (error != base::File::FILE_OK) {
+    file.Close();
+    base::DeleteFile(dest_local_path, false /* recursive */);
+    return base::File(error);
+  }
+
+  return file.Pass();
+}
+
 base::File::Error ObfuscatedFileUtil::CreateFile(
     FileSystemOperationContext* context,
     const base::FilePath& src_file_path,
     const FileSystemURL& dest_url,
-    FileInfo* dest_file_info, int file_flags, PlatformFile* handle) {
-  if (handle)
-    *handle = base::kInvalidPlatformFileValue;
-  SandboxDirectoryDatabase* db = GetDirectoryDatabase(dest_url, true);
-
-  base::File::Error error = base::File::FILE_OK;
-  base::FilePath root = GetDirectoryForURL(dest_url, false, &error);
-  if (error != base::File::FILE_OK)
-    return error;
-
-  base::FilePath dest_local_path;
-  error = GenerateNewLocalPath(db, context, dest_url, &dest_local_path);
+    FileInfo* dest_file_info) {
+  SandboxDirectoryDatabase* db;
+  base::FilePath root, dest_local_path;
+  base::File::Error error = GetDbRootAndLocalPath(context, dest_url, &db, &root,
+                                                  &dest_local_path);
   if (error != base::File::FILE_OK)
     return error;
 
   bool created = false;
-  if (!src_file_path.empty()) {
-    DCHECK(!file_flags);
-    DCHECK(!handle);
-    error = NativeFileUtil::CopyOrMoveFile(
-        src_file_path, dest_local_path,
-        FileSystemOperation::OPTION_NONE,
-        fileapi::NativeFileUtil::CopyOrMoveModeForDestination(dest_url,
-                                                              true /* copy */));
-    created = true;
-  } else {
+  if (src_file_path.empty()) {
     if (base::PathExists(dest_local_path)) {
       if (!base::DeleteFile(dest_local_path, true /* recursive */))
         return base::File::FILE_ERROR_FAILED;
       LOG(WARNING) << "A stray file detected";
       InvalidateUsageCache(context, dest_url.origin(), dest_url.type());
     }
 
-    if (handle) {
-      // TODO(rvargas): Remove PlatformFile from this code.
-      base::File file =
-          NativeFileUtil::CreateOrOpen(dest_local_path, file_flags);
-      if (file.IsValid()) {
-        created = file.created();
-        *handle = file.TakePlatformFile();
-        error = base::File::FILE_OK;
-      } else {
-        error = file.error_details();
-      }
-      // If this succeeds, we must close handle on any subsequent error.
-    } else {
-      DCHECK(!file_flags);  // file_flags is only used by CreateOrOpen.
-      error = NativeFileUtil::EnsureFileExists(dest_local_path, &created);
-    }
+    error = NativeFileUtil::EnsureFileExists(dest_local_path, &created);
+  } else {
+    error = NativeFileUtil::CopyOrMoveFile(
+        src_file_path, dest_local_path,
+        FileSystemOperation::OPTION_NONE,
+        fileapi::NativeFileUtil::CopyOrMoveModeForDestination(dest_url,
+                                                              true /* copy */));
+    created = true;
   }
   if (error != base::File::FILE_OK)
     return error;
+  if (!created)
+    return base::File::FILE_ERROR_FAILED;
 
-  if (!created) {
-    if (handle) {
-      DCHECK_NE(base::kInvalidPlatformFileValue, *handle);
-      base::ClosePlatformFile(*handle);
-      base::DeleteFile(dest_local_path, false /* recursive */);
-      *handle = base::kInvalidPlatformFileValue;
-    }
-    return base::File::FILE_ERROR_FAILED;
-  }
+  return PostCreateFile(root, dest_local_path, dest_file_info, db);
 
-  // This removes the root, including the trailing slash, leaving a relative
-  // path.
-  dest_file_info->data_path = base::FilePath(
-      dest_local_path.value().substr(root.value().length() + 1));
-
-  FileId file_id;
-  error = db->AddFileInfo(*dest_file_info, &file_id);
-  if (error != base::File::FILE_OK) {
-    if (handle) {
-      DCHECK_NE(base::kInvalidPlatformFileValue, *handle);
-      base::ClosePlatformFile(*handle);
-      *handle = base::kInvalidPlatformFileValue;
-    }
-    base::DeleteFile(dest_local_path, false /* recursive */);
-    return error;
-  }
-  TouchDirectory(db, dest_file_info->parent_id);
-
-  return base::File::FILE_OK;
 }
 
 base::FilePath ObfuscatedFileUtil::DataPathToLocalPath(
     const FileSystemURL& url, const base::FilePath& data_path) {
   base::File::Error error = base::File::FILE_OK;
   base::FilePath root = GetDirectoryForURL(url, false, &error);
   if (error != base::File::FILE_OK)
     return base::FilePath();
   return root.Append(data_path);
 }
@@ skipping 186 matching lines @@
   error = NativeFileUtil::CreateDirectory(
       new_local_path, false /* exclusive */, false /* recursive */);
   if (error != base::File::FILE_OK)
     return error;
 
   *local_path =
       new_local_path.AppendASCII(base::StringPrintf("%08" PRId64, number));
   return base::File::FILE_OK;
 }
 
-base::File::Error ObfuscatedFileUtil::CreateOrOpenInternal(
+base::File ObfuscatedFileUtil::CreateOrOpenInternal(
     FileSystemOperationContext* context,
-    const FileSystemURL& url, int file_flags,
-    PlatformFile* file_handle, bool* created) {
-  DCHECK(!(file_flags & (base::PLATFORM_FILE_DELETE_ON_CLOSE |
-        base::PLATFORM_FILE_HIDDEN | base::PLATFORM_FILE_EXCLUSIVE_READ |
-        base::PLATFORM_FILE_EXCLUSIVE_WRITE)));
+    const FileSystemURL& url, int file_flags) {
+  DCHECK(!(file_flags & (base::File::FLAG_DELETE_ON_CLOSE |
+        base::File::FLAG_HIDDEN | base::File::FLAG_EXCLUSIVE_READ |
+        base::File::FLAG_EXCLUSIVE_WRITE)));
   SandboxDirectoryDatabase* db = GetDirectoryDatabase(url, true);
   if (!db)
-    return base::File::FILE_ERROR_FAILED;
+    return base::File(base::File::FILE_ERROR_FAILED);
   FileId file_id;
   if (!db->GetFileWithPath(url.path(), &file_id)) {
     // The file doesn't exist.
-    if (!(file_flags & (base::PLATFORM_FILE_CREATE |
-        base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_OPEN_ALWAYS)))
-      return base::File::FILE_ERROR_NOT_FOUND;
+    if (!(file_flags & (base::File::FLAG_CREATE |
+        base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_OPEN_ALWAYS))) {
+      return base::File(base::File::FILE_ERROR_NOT_FOUND);
+    }
     FileId parent_id;
-    if (!db->GetFileWithPath(VirtualPath::DirName(url.path()),
-                             &parent_id))
-      return base::File::FILE_ERROR_NOT_FOUND;
+    if (!db->GetFileWithPath(VirtualPath::DirName(url.path()), &parent_id))
+      return base::File(base::File::FILE_ERROR_NOT_FOUND);
     FileInfo file_info;
     InitFileInfo(&file_info, parent_id,
                  VirtualPath::BaseName(url.path()).value());
 
     int64 growth = UsageForPath(file_info.name.size());
     if (!AllocateQuota(context, growth))
-      return base::File::FILE_ERROR_NO_SPACE;
-    base::File::Error error = CreateFile(
-        context, base::FilePath(),
-        url, &file_info, file_flags, file_handle);
-    if (created && base::File::FILE_OK == error) {
-      *created = true;
+      return base::File(base::File::FILE_ERROR_NO_SPACE);
+    base::File file = CreateAndOpenFile(context, url, &file_info, file_flags);
+    if (file.IsValid()) {
       UpdateUsage(context, url, growth);
       context->change_observers()->Notify(
           &FileChangeObserver::OnCreateFile, MakeTuple(url));
     }
-    return error;
+    return file.Pass();
   }
 
-  if (file_flags & base::PLATFORM_FILE_CREATE)
-    return base::File::FILE_ERROR_EXISTS;
+  if (file_flags & base::File::FLAG_CREATE)
+    return base::File(base::File::FILE_ERROR_EXISTS);
 
   base::File::Info platform_file_info;
   base::FilePath local_path;
   FileInfo file_info;
   base::File::Error error = GetFileInfoInternal(
       db, context, url, file_id, &file_info, &platform_file_info, &local_path);
   if (error != base::File::FILE_OK)
-    return error;
+    return base::File(error);
   if (file_info.is_directory())
-    return base::File::FILE_ERROR_NOT_A_FILE;
+    return base::File(base::File::FILE_ERROR_NOT_A_FILE);
 
   int64 delta = 0;
-  if (file_flags & (base::PLATFORM_FILE_CREATE_ALWAYS |
-                    base::PLATFORM_FILE_OPEN_TRUNCATED)) {
+  if (file_flags & (base::File::FLAG_CREATE_ALWAYS |
+                    base::File::FLAG_OPEN_TRUNCATED)) {
     // The file exists and we're truncating.
     delta = -platform_file_info.size;
     AllocateQuota(context, delta);
   }
 
-  // TODO(rvargas): make FileSystemFileUtil use base::File.
   base::File file = NativeFileUtil::CreateOrOpen(local_path, file_flags);
   if (!file.IsValid()) {
     error = file.error_details();
     if (error == base::File::FILE_ERROR_NOT_FOUND) {
       // TODO(tzik): Also invalidate on-memory usage cache in UsageTracker.
       // TODO(tzik): Delete database entry after ensuring the file lost.
       InvalidateUsageCache(context, url.origin(), url.type());
       LOG(WARNING) << "Lost a backing file.";
-      error = base::File::FILE_ERROR_FAILED;
+      return base::File(base::File::FILE_ERROR_FAILED);
     }
-    return error;
+    return file.Pass();
   }
 
-  *created = file.created();
-  *file_handle = file.TakePlatformFile();
-
   // If truncating we need to update the usage.
   if (delta) {
     UpdateUsage(context, url, delta);
     context->change_observers()->Notify(
         &FileChangeObserver::OnModifyFile, MakeTuple(url));
   }
-  return base::File::FILE_OK;
+  return file.Pass();
 }
 
 bool ObfuscatedFileUtil::HasIsolatedStorage(const GURL& origin) {
   return special_storage_policy_.get() &&
       special_storage_policy_->HasIsolatedStorage(origin);
 }
 
 }  // namespace fileapi