Remove PlatforFile from fileapi/native_file_util

webkit/browser/fileapi/native_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/native_file_util.h"
 
 #include "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/file_stream.h"
@@ skipping 104 matching lines @@
 
 NativeFileUtil::CopyOrMoveMode NativeFileUtil::CopyOrMoveModeForDestination(
     const FileSystemURL& dest_url, bool copy) {
   if (copy) {
     return dest_url.mount_option().copy_sync_option() == COPY_SYNC_OPTION_SYNC ?
         COPY_SYNC : COPY_NOSYNC;
   }
   return MOVE;
 }
 
-base::File::Error NativeFileUtil::CreateOrOpen(
-    const base::FilePath& path, int file_flags,
-    PlatformFile* file_handle, bool* created) {
+base::File::Error NativeFileUtil::CreateOrOpen(const base::FilePath& path,
+                                               int file_flags,
+                                               base::File* file) {
+  DCHECK(!file->IsValid());
   if (!base::DirectoryExists(path.DirName())) {

[rvargas (doing something else), 2014/03/21 22:39:21]

Is there a platform where this would not fail during create? Or was the
objective to standardize on the actual return code (which I saw callers
overriding)?

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

The latter. (The main purpose of having this thin layer on top of PlatformFile
or File is basically to have the error code we want to have) If we could be sure
we get the same NOT_FOUND error from file->error_details() I think we're fine
too.

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

I'm still curious about it. Are we promising a particular error code to web
developers (if this ever gets that far)? I don't recall seeing an API that
actually succeeds in enforcing that only a given error code can be returned...
what if there is something like lack of privileges that should take precedence
over existence of the path? Or a path that is slightly malformed?

What I'm wondering is how to deal with this:
1. Leave this code as is
2. Add some unit tests to base to check that other things being equal,
FILE_ERROR_NOT_FOUND is the actual code returned by base
3. Add a way to create a File with a given error code so that this code can
override what base would return
4. Go with what base decides to return and let callers decide if they care.

Any preference?

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

These error codes are basically following this spec:
http://www.w3.org/TR/file-system-api/#errors-and-exceptions
We also use the error code reported by web developers to diagnose error
situations.

The order of error case checking affects the return code, yes, but we try to be
consistent for obvious error cases that are mentioned in the spec.
If the current base code returns FILE_ERROR_NOT_FOUND 2. sounds good enough, but
otherwise I prefer 1. (since this class's basically to implement web-facing API,
which doesn't need to be same as how platform layer works). 3. feels a bit
overkill, and 4. breaks the existing behavior.

[rvargas (doing something else), 2014/03/26 00:35:46]

There are not that many error codes on that doc so it's not like there's a lot
of choices, but still that doc is pretty open about an implementation returning
an error code. It of course says what a particular error means (once someone
decides to return it)
I ran some tests and all bots return FILE_ERROR_NOT_FOUND for that use case.
However, I don't want to have a contract for base that it _has to_ return this
error code. I think base functions should return the error code that reflects
more closely what happens, and as I said, at any time there could be a better
option

I went for adding another constructor to base, so that this code can maintain
whatever promises it wants to, and we can return a File directly.

     // If its parent does not exist, should return NOT_FOUND error.
     return base::File::FILE_ERROR_NOT_FOUND;
   }
   if (base::DirectoryExists(path))
     return base::File::FILE_ERROR_NOT_A_FILE;

[rvargas (doing something else), 2014/03/21 22:39:21]

Maybe there should be an explicit flag for opening directories (there is already
one, although Windows only)

 
-  // TODO(rvargas): convert this code to use base::File.
-  base::PlatformFileError error_code = base::PLATFORM_FILE_OK;
-  *file_handle = base::CreatePlatformFile(path, file_flags,
-                                          created, &error_code);
-  return static_cast<base::File::Error>(error_code);
-}
-
-base::File::Error NativeFileUtil::Close(PlatformFile file_handle) {
-  if (!base::ClosePlatformFile(file_handle))
-    return base::File::FILE_ERROR_FAILED;
-  return base::File::FILE_OK;
+  *file = base::File(path, file_flags);
+  if (file->IsValid())
+    return base::File::FILE_OK;
+  return file->error_details();
 }
 
 base::File::Error NativeFileUtil::EnsureFileExists(
     const base::FilePath& path,
     bool* created) {
   if (!base::DirectoryExists(path.DirName()))
     // If its parent does not exist, should return NOT_FOUND error.
     return base::File::FILE_ERROR_NOT_FOUND;
 
   // Tries to create the |path| exclusively.  This should fail
@@ skipping 160 matching lines @@
   if (!base::DirectoryExists(path))
     return base::File::FILE_ERROR_NOT_A_DIRECTORY;
   if (!base::IsDirectoryEmpty(path))
     return base::File::FILE_ERROR_NOT_EMPTY;
   if (!base::DeleteFile(path, false))
     return base::File::FILE_ERROR_FAILED;
   return base::File::FILE_OK;
 }
 
 }  // namespace fileapi