Remove PlatforFile from fileapi/native_file_util

Remove PlatforFile from fileapi/native_file_util

BUG=322664
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259540

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

PTAL

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: if
(!base::DirectoryExists(path.DirName())) {
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)?

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:134: return
base::File::FILE_ERROR_NOT_A_FILE;
Maybe there should be an explicit flag for opening directories (there is already
one, although Windows only)

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.h (left):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.h:42: static base::File::Error
CreateOrOpen(
Actually I would prefer to return base::File instead even if it breaks the
overall symmetry of the class (although not every method currently returns
Error). It makes it absolutely explicit that there is a transfer of ownership
for the file (which is the overall objective), and it eliminates the small issue
about having two ways to do the same thing (return value vs.
file.error_details())

On the other hand, for something called CreateOrOpen, it is pretty clear (at
least to me) that |File* file| is an out argument and the owner is the caller.

BTW, a similar change to FileSystemFileUtil would follow.

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

looking good

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: if
(!base::DirectoryExists(path.DirName())) {
On 2014/03/21 22:39:21, rvargas wrote:
> 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)?

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.

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.h (left):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.h:42: static base::File::Error
CreateOrOpen(
On 2014/03/21 22:39:21, rvargas wrote:
> Actually I would prefer to return base::File instead even if it breaks the
> overall symmetry of the class (although not every method currently returns
> Error). It makes it absolutely explicit that there is a transfer of ownership
> for the file (which is the overall objective), and it eliminates the small
issue
> about having two ways to do the same thing (return value vs.
> file.error_details())
> 
> On the other hand, for something called CreateOrOpen, it is pretty clear (at
> least to me) that |File* file| is an out argument and the owner is the caller.
> 
> BTW, a similar change to FileSystemFileUtil would follow.

I'm fine with having this return base::File.

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.cc (left):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1084: NOTREACHED();
Why are you removing these NOTREACHED?

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

Thanks

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: if
(!base::DirectoryExists(path.DirName())) {
On 2014/03/24 09:56:58, kinuko wrote:
> On 2014/03/21 22:39:21, rvargas wrote:
> > 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)?
> 
> 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.

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?

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.cc (left):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1084: NOTREACHED();
On 2014/03/24 09:56:58, kinuko wrote:
> Why are you removing these NOTREACHED?

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:25]

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: if
(!base::DirectoryExists(path.DirName())) {
On 2014/03/24 23:02:48, rvargas wrote:
> On 2014/03/24 09:56:58, kinuko wrote:
> > On 2014/03/21 22:39:21, rvargas wrote:
> > > 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)?
> > 
> > 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.
> 
> 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?

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.

> 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.

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.

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
File webkit/browser/fileapi/obfuscated_file_util.cc (left):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/o...
webkit/browser/fileapi/obfuscated_file_util.cc:1084: NOTREACHED();
On 2014/03/24 23:02:48, rvargas wrote:
> On 2014/03/24 09:56:58, kinuko wrote:
> > Why are you removing these NOTREACHED?
> 
> 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).

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))

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

+willchan for base

I think this is ready now.

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/20001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: if
(!base::DirectoryExists(path.DirName())) {
On 2014/03/25 03:15:26, kinuko wrote:
> On 2014/03/24 23:02:48, rvargas wrote:
> > On 2014/03/24 09:56:58, kinuko wrote:
> > > On 2014/03/21 22:39:21, rvargas wrote:
> > > > 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)?
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.

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)

> 
> > 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.
> 
> 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.

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.

[kinuko, 2014-03-26 03:35:05]

fileapi/ lgtm

https://codereview.chromium.org/206783004/diff/40001/webkit/browser/fileapi/n...
File webkit/browser/fileapi/native_file_util.cc (right):

https://codereview.chromium.org/206783004/diff/40001/webkit/browser/fileapi/n...
webkit/browser/fileapi/native_file_util.cc:129: return
base::File(base::File::FILE_ERROR_NOT_FOUND);
Ah, ok, to return a specific error.. (I think I misread some of your previous
comments) Thanks for keeping the logic here.

[willchan no longer on Chromium, 2014-03-26 05:44:38]

lgtm

[Lei Zhang, 2014-03-26 06:11:33]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-03-26 06:11:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/206783004/40001

[Lei Zhang, 2014-03-26 06:12:43]

On 2014/03/26 06:11:33, Lei Zhang wrote:
> The CQ bit was checked by mailto:thestig@chromium.org

It turns out I have a CL that I'd like to land for M35, and I'd rather not add
more base::PlatformFile code, so expediting the process here.

[commit-bot: I haz the power, 2014-03-26 09:59:46]

Change committed as 259540