Make sure we close the file_handle when we create (but not open) a new file

Make sure we close the file_handle when we create (but not open) a new file

1. Add FileUtilProxy::Create that does not leave a file_handle opened.
2. Update the file_util_operation to use FileUtilProxy::Create instead of FileUtilProxy::CreateOrOpen so that no other tasks get queued in before we close the handle.

BUG=58424, 58473
TEST=fast/filesystem/

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62192

[Kavita Kanetkar, 2010-10-11 19:15:55]

LGTM

On 2010/10/11 19:09:10, Kinuko Yasuda wrote:
>

[Kavita Kanetkar, 2010-10-11 19:16:28]

LGTM

[dumi, 2010-10-11 20:27:18]

LGTM

http://codereview.chromium.org/3717001/diff/2002/5001
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3717001/diff/2002/5001#newcode122
base/file_util_proxy.cc:122: bool create_only,
maybe change the name to no_file_handle, or close_handle or something like that?
'create_only' could also be interpreted as "open the file, but only if it
doesn't exist and needs to be created".

[kinuko, 2010-10-11 20:50:22]

On 2010/10/11 20:27:18, dumi wrote:
> LGTM
> 
> http://codereview.chromium.org/3717001/diff/2002/5001
> File base/file_util_proxy.cc (right):
> 
> http://codereview.chromium.org/3717001/diff/2002/5001#newcode122
> base/file_util_proxy.cc:122: bool create_only,
> maybe change the name to no_file_handle, or close_handle or something like
that?
> 'create_only' could also be interpreted as "open the file, but only if it
> doesn't exist and needs to be created".

Changed the flag name to return_no_handle.  Will be submitting if no one stops
me.

[brettw, 2010-10-13 22:48:32]

http://codereview.chromium.org/3717001/diff/8001/9002
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3717001/diff/8001/9002#newcode52
base/file_util_proxy.h:52: // CreateOrOpen but it doesn't return a file handle.
Sorry I didn't look at this before it was checked in.

From looking at the function name, Create is a bit confusing. I would normally
expect from an API with functions Create and CreateOrOpen that Create forces the
file to be created, giving me an empty file no matter what, while CreateOrOpen
would create me an empty one or open an existing one. I would expect both to
give me the handle.

I wonder if there's some name like EnsureFileExists that might make the behavior
more clear?

Can you also specify in the comment what the value will be in the callback?

[kinuko, 2010-10-13 23:01:28]

http://codereview.chromium.org/3717001/diff/8001/9002
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3717001/diff/8001/9002#newcode52
base/file_util_proxy.h:52: // CreateOrOpen but it doesn't return a file handle.
On 2010/10/13 22:48:32, brettw wrote:
> Sorry I didn't look at this before it was checked in.
> 
> From looking at the function name, Create is a bit confusing. I would normally
> expect from an API with functions Create and CreateOrOpen that Create forces
the
> file to be created, giving me an empty file no matter what, while CreateOrOpen
> would create me an empty one or open an existing one. I would expect both to
> give me the handle.
> I wonder if there's some name like EnsureFileExists that might make the
behavior
> more clear?

I see the point, sorry for bringing the confusing change.

EnsureFileExists sounds clearer, but we want it to return an error
(ERROR_FILE_EXIST, sort of EEXIST) if the file already exists and the method
name may not make sense in the case?

Hmm what if I simply remove this new method and add a flag like
'close_handle_before_callback' to CreateOrOpen?  (Or add an overloaded
CreateOrOpen that takes extra flags)

> Can you also specify in the comment what the value will be in the callback?

I will.

[kinuko, 2010-10-13 23:21:22]

On Wed, Oct 13, 2010 at 4:11 PM, <brettw@chromium.org> wrote:

>
> http://codereview.chromium.org/3717001/diff/8001/9002
> File base/file_util_proxy.h (right):
>
> http://codereview.chromium.org/3717001/diff/8001/9002#newcode52
> base/file_util_proxy.h:52: // CreateOrOpen but it doesn't return a file
> handle.
> On 2010/10/13 23:01:28, Kinuko Yasuda wrote:
>
>> On 2010/10/13 22:48:32, brettw wrote:
>> > Sorry I didn't look at this before it was checked in.
>> >
>> > From looking at the function name, Create is a bit confusing. I
>>
> would normally
>
>> > expect from an API with functions Create and CreateOrOpen that
>>
> Create forces
>
>> the
>> > file to be created, giving me an empty file no matter what, while
>>
> CreateOrOpen
>
>> > would create me an empty one or open an existing one. I would expect
>>
> both to
>
>> > give me the handle.
>> > I wonder if there's some name like EnsureFileExists that might make
>>
> the
>
>> behavior
>> > more clear?
>>
>
>  I see the point, sorry for bringing the confusing change.
>>
>
>  EnsureFileExists sounds clearer, but we want it to return an error
>> (ERROR_FILE_EXIST, sort of EEXIST) if the file already exists and the
>>
> method
>
>> name may not make sense in the case?
>>
>
> Hmm, I see. I think EnsureFileExists would still be okay as long as the
> error values are documented clearly (maybe comment "The error code in
> the callback will be XXX if the file was created, or ERROR_FILE_EXIST if
> it already exists."
>

Sounds like it makes sense.  I buy this.


>  "CreateFileIfNonexistent" is the alternative I can come up with if you
> prefer that, but it seems kind of long.
>
> No matter what it's called, we should be sure to document the
> ERROR_FILE_EXIST behavior? It sounds like almost all callers will want
> to know about this.
>

My comment wasn't accurate enough, CreateOrOpen returns FILE_EXIST error if
PLATFORM_FILE_CREATE file_flag is given as CreatePlatformFile does.  To make
it clear I will add a comment to CreateOrOpen too though.

I'll make a separate changeset to address these issues (will cc you).

Thanks!


>  http://codereview.chromium.org/3717001/show
>

[brettw, 2010-10-13 23:24:54]

On Wed, Oct 13, 2010 at 4:20 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> On Wed, Oct 13, 2010 at 4:11 PM, <brettw@chromium.org> wrote:
>>
>> http://codereview.chromium.org/3717001/diff/8001/9002
>> File base/file_util_proxy.h (right):
>>
>> http://codereview.chromium.org/3717001/diff/8001/9002#newcode52
>> base/file_util_proxy.h:52: // CreateOrOpen but it doesn't return a file
>> handle.
>> On 2010/10/13 23:01:28, Kinuko Yasuda wrote:
>>>
>>> On 2010/10/13 22:48:32, brettw wrote:
>>> > Sorry I didn't look at this before it was checked in.
>>> >
>>> > From looking at the function name, Create is a bit confusing. I
>>
>> would normally
>>>
>>> > expect from an API with functions Create and CreateOrOpen that
>>
>> Create forces
>>>
>>> the
>>> > file to be created, giving me an empty file no matter what, while
>>
>> CreateOrOpen
>>>
>>> > would create me an empty one or open an existing one. I would expect
>>
>> both to
>>>
>>> > give me the handle.
>>> > I wonder if there's some name like EnsureFileExists that might make
>>
>> the
>>>
>>> behavior
>>> > more clear?
>>
>>> I see the point, sorry for bringing the confusing change.
>>
>>> EnsureFileExists sounds clearer, but we want it to return an error
>>> (ERROR_FILE_EXIST, sort of EEXIST) if the file already exists and the
>>
>> method
>>>
>>> name may not make sense in the case?
>>
>> Hmm, I see. I think EnsureFileExists would still be okay as long as the
>> error values are documented clearly (maybe comment "The error code in
>> the callback will be XXX if the file was created, or ERROR_FILE_EXIST if
>> it already exists."
>
> Sounds like it makes sense.  I buy this.
>
>>
>> "CreateFileIfNonexistent" is the alternative I can come up with if you
>> prefer that, but it seems kind of long.
>>
>> No matter what it's called, we should be sure to document the
>> ERROR_FILE_EXIST behavior? It sounds like almost all callers will want
>> to know about this.
>
> My comment wasn't accurate enough, CreateOrOpen returns FILE_EXIST error if
> PLATFORM_FILE_CREATE file_flag is given as CreatePlatformFile does.  To make
> it clear I will add a comment to CreateOrOpen too though.
> I'll make a separate changeset to address these issues (will cc you).
> Thanks!

Thanks for helping improve this!

Brett