Remove PlatforFile from fileapi/native_file_util

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 274 matching lines @@
       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) {
-  return NativeFileUtil::Close(file);
+  base::File auto_closed(file);
+  return base::File::FILE_OK;
 }
 
 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 767 matching lines @@
     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 (base::PathExists(dest_local_path)) {
-      if (!base::DeleteFile(dest_local_path, true /* recursive */)) {
-        NOTREACHED();

[kinuko, 2014/03/24 09:56:58]

Why are you removing these NOTREACHED?

[rvargas (doing something else), 2014/03/24 23:02:48]

Because they are DCHECKing at the same time that handling the failure (frowned
upon by style), and not checking an actual precondition that can be verified as
it is outside of the control of the code (there are many reasons for the OS to
refuse to delete a file, or in general, succeed on any system call).

[kinuko, 2014/03/25 03:15:26]

I'm not original author of this code, and fine with deleting it in these
particular cases, but I think having NOTREACHED and also handling failure is not
that uncommon in our code base. (E.g. when we want to detect naive coding errors
in tests but still want to handle gracefully in prod case when there's a slight
chance that the case could occur (as in this case))

+      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) {
-      error = NativeFileUtil::CreateOrOpen(
-          dest_local_path, file_flags, handle, &created);
+      // TODO(rvargas): Remove PlatformFile from this code.
+      base::File file;
+      error = NativeFileUtil::CreateOrOpen(dest_local_path, file_flags, &file);
+      if (error == base::File::FILE_OK) {
+        created = file.created();
+        *handle = file.TakePlatformFile();
+      }
       // 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);
     }
   }
   if (error != base::File::FILE_OK)
     return error;
 
   if (!created) {
-    NOTREACHED();
     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;
   }
 
   // This removes the root, including the trailing slash, leaving a relative
@@ skipping 274 matching lines @@
     return base::File::FILE_ERROR_NOT_A_FILE;
 
   int64 delta = 0;
   if (file_flags & (base::PLATFORM_FILE_CREATE_ALWAYS |
                     base::PLATFORM_FILE_OPEN_TRUNCATED)) {
     // The file exists and we're truncating.
     delta = -platform_file_info.size;
     AllocateQuota(context, delta);
   }
 
-  error = NativeFileUtil::CreateOrOpen(
-      local_path, file_flags, file_handle, created);
+  // TODO(rvargas): make FileSystemFileUtil use base::File.
+  base::File file;
+  error = NativeFileUtil::CreateOrOpen(local_path, file_flags, &file);
   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;
   }
 
+  if (error != base::File::FILE_OK)
+    return error;
+
+  *created = file.created();
+  *file_handle = file.TakePlatformFile();
+
   // If truncating we need to update the usage.
-  if (error == base::File::FILE_OK && delta) {
+  if (delta) {
     UpdateUsage(context, url, delta);
     context->change_observers()->Notify(
         &FileChangeObserver::OnModifyFile, MakeTuple(url));
   }
   return error;
 }
 
 bool ObfuscatedFileUtil::HasIsolatedStorage(const GURL& origin) {
   return special_storage_policy_.get() &&
       special_storage_policy_->HasIsolatedStorage(origin);
 }
 
 }  // namespace fileapi