Support creating temporary files for sync file operations.

Support creating temporary files for sync file operations.

BUG=None
TEST=None


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

[noelutz, 2011-06-02 22:26:30]

I would like to use FileUtilProxy::CreateTemporary() followed by
FileUtilProxy::Write() which is currently not supported because
CreateTemporary() opens the file for async operations but Write() doesn't
support async file operations.

This change modifies CreateTemporary() to let you open the file for sync
operations.

noé.

[Sam Kerner (Chrome), 2011-06-03 01:08:37]

Thanks for the cleanup.

Generally, // is preferred to /* */.

http://codereview.chromium.org/7066067/diff/1/content/common/url_fetcher.cc
File content/common/url_fetcher.cc (right):

http://codereview.chromium.org/7066067/diff/1/content/common/url_fetcher.cc#n...
content/common/url_fetcher.cc:316: false /* open file for synchronous file
operations */,
Prefer // to /* */

[Sam Kerner (Chrome), 2011-06-03 14:31:41]

http://codereview.chromium.org/7066067/diff/1/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/1/base/file_util_proxy.h#newcode73
base/file_util_proxy.h:73: bool aync,
Are there reasonable use cases for other sets of file flags?  If so, it might be
better to allow a user to pass in the flags they want.

[noelutz, 2011-06-03 17:10:47]

Please take another look.
Thanks,
noe.

On 2011/06/03 14:31:41, Sam Kerner (Chrome) wrote:
> http://codereview.chromium.org/7066067/diff/1/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
> http://codereview.chromium.org/7066067/diff/1/base/file_util_proxy.h#newcode73
> base/file_util_proxy.h:73: bool aync,
> Are there reasonable use cases for other sets of file flags?  If so, it might
be
> better to allow a user to pass in the flags they want.

[darin (slow to review), 2011-06-03 17:51:45]

http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
base/file_util_proxy.h:73: bool aync,
please avoid adding bool parameters like this.  it makes the callsite very
unreadable.  you just see true/false, with no idea what it means.  it is better
to define an enum.

another option is to create a separate function name, which describes the
variant you want.

however, i also want to ask:  why are you using FileUtilProxy::Write and not
net::FileStream to write to a file?

[darin (slow to review), 2011-06-03 17:52:23]

http://codereview.chromium.org/7066067/diff/2004/content/browser/renderer_hos...
File content/browser/renderer_host/redirect_to_file_resource_handler.cc (right):

http://codereview.chromium.org/7066067/diff/2004/content/browser/renderer_hos...
content/browser/renderer_host/redirect_to_file_resource_handler.cc:79: true,  //
open tmp file for async operations
the fact that you were encouraged to write this comment should be an indicator
that an enum value would be better!

[noelutz, 2011-06-03 18:26:07]

Thanks for your input!

On Fri, Jun 3, 2011 at 10:51 AM,  <darin@chromium.org> wrote:
>
> http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
>
>
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
> base/file_util_proxy.h:73: bool aync,
> please avoid adding bool parameters like this.  it makes the callsite
> very unreadable.  you just see true/false, with no idea what it means.
> it is better to define an enum.

Good point.  I will definitely keep that in mind for next time.

> another option is to create a separate function name, which describes
> the variant you want.
>
> however, i also want to ask:  why are you using FileUtilProxy::Write and
> not net::FileStream to write to a file?

No reason.  I can totally use net::FileStream - thank you.  I reverted
my change to the FileUtilProxy.  Sam: I reverted the change to the URL
fetcher as well; maybe you can use FileStream as well?

I did, however, add a comment to CreateTemporary to indicate that the
resulting platform file cannot be used with Write() because I think
it's a bit confusing.

Thanks,
noe.

>
> http://codereview.chromium.org/7066067/
>



-- 
-|||--

[darin (slow to review), 2011-06-03 18:31:11]

On Fri, Jun 3, 2011 at 11:25 AM, Noé Lutz <noelutz@google.com> wrote:

> Thanks for your input!
>
> On Fri, Jun 3, 2011 at 10:51 AM,  <darin@chromium.org> wrote:
> >
> > http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
> > File base/file_util_proxy.h (right):
> >
> >
>
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
> > base/file_util_proxy.h:73: bool aync,
> > please avoid adding bool parameters like this.  it makes the callsite
> > very unreadable.  you just see true/false, with no idea what it means.
> > it is better to define an enum.
>
> Good point.  I will definitely keep that in mind for next time.
>
> > another option is to create a separate function name, which describes
> > the variant you want.
> >
> > however, i also want to ask:  why are you using FileUtilProxy::Write and
> > not net::FileStream to write to a file?
>
> No reason.  I can totally use net::FileStream - thank you.  I reverted
> my change to the FileUtilProxy.  Sam: I reverted the change to the URL
> fetcher as well; maybe you can use FileStream as well?
>
> I did, however, add a comment to CreateTemporary to indicate that the
> resulting platform file cannot be used with Write() because I think
> it's a bit confusing.
>

I agree it is confusing.  It may still be worth making a change as you have
here.  Maybe you could just leverage PlatformFileFlags?  It seems like it
might be good for the caller of CreateTemporary to have some control over
the file flags.  There are obviously some values that should be disallowed.

By the way, FileStream is only usable if you are on a thread which has a
MessageLoopForIO.  Note: This restriction is only true on Windows, so if
you are only testing on Mac/Linux, then you will be surprised when you test
on Windows :-(

-Darin



>
> Thanks,
> noe.
>
> >
> > http://codereview.chromium.org/7066067/
> >
>
>
>
> --
> -|||--
>

[noelutz, 2011-06-03 18:39:01]

On Fri, Jun 3, 2011 at 11:31 AM, Darin Fisher <darin@chromium.org> wrote:
>
>
> On Fri, Jun 3, 2011 at 11:25 AM, Noé Lutz <noelutz@google.com> wrote:
>>
>> Thanks for your input!
>>
>> On Fri, Jun 3, 2011 at 10:51 AM,  <darin@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
>> > File base/file_util_proxy.h (right):
>> >
>> >
>> >
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
>> > base/file_util_proxy.h:73: bool aync,
>> > please avoid adding bool parameters like this.  it makes the callsite
>> > very unreadable.  you just see true/false, with no idea what it means.
>> > it is better to define an enum.
>>
>> Good point.  I will definitely keep that in mind for next time.
>>
>> > another option is to create a separate function name, which describes
>> > the variant you want.
>> >
>> > however, i also want to ask:  why are you using FileUtilProxy::Write and
>> > not net::FileStream to write to a file?
>>
>> No reason.  I can totally use net::FileStream - thank you.  I reverted
>> my change to the FileUtilProxy.  Sam: I reverted the change to the URL
>> fetcher as well; maybe you can use FileStream as well?
>>
>> I did, however, add a comment to CreateTemporary to indicate that the
>> resulting platform file cannot be used with Write() because I think
>> it's a bit confusing.
>
> I agree it is confusing.  It may still be worth making a change as you have
> here.  Maybe you could just leverage PlatformFileFlags?  It seems like it
> might be good for the caller of CreateTemporary to have some control over
> the file flags.  There are obviously some values that should be disallowed.

I can do that.  I'll change the signature of CreateTemporary which
will take PlatformFileFlags and will do some sanity checking on the
flags.

> By the way, FileStream is only usable if you are on a thread which has a
> MessageLoopForIO.

I just noticed that.  In my case it's actually more convenient if I
don't have to go through the IO thread.

> Note: This restriction is only true on Windows, so if
> you are only testing on Mac/Linux, then you will be surprised when you test
> on Windows :-(

I code on Linux.  It took me a while to figure out what was wrong with
my code :(.  Thankfully, I had a unit test.

I'll update the CL in a bit.
Thanks,
noe.


> -Darin
>
>>
>> Thanks,
>> noe.
>>
>> >
>> > http://codereview.chromium.org/7066067/
>> >
>>
>>
>>
>> --
>> -|||--
>
>



-- 
-|||--

[noelutz, 2011-06-03 21:22:47]

Please take another look.

Thanks,
noe.

[darin (slow to review), 2011-06-04 21:06:01]

http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h#newco...
base/file_util_proxy.h:71: //   base::PLATFORM_FILE_CREATE_ALWAYS |
i think having to specify PLATFORM_FILE_TEMPORARY makes the API redundant.  the
method name is CreateTemporary after all!

how about we rename file_flags to additional_file_flags (or
optional_file_flags), and then we document that CREATE_ALWAYS, WRITE, and
TEMPORARY bits comprise the default file flags.  this way the callers can pass 0
if they don't need to specify anything special.

so i would expect to see CreateTemporary(loop, 0, callback) in many cases or
CreateTemporary(loop, PLATFORM_FILE_ASYNC, callback) in cases where we want to
open the file for async IO.

finally, it occurs to me that fixing this method to make it possible to not set
PLATFORM_FILE_ASYNC doesn't really fix the original issue you were trying to
solve.  the problem you had was that the Write method cannot handle a
PlatformFile that was opened with PLATFORM_FILE_ASYNC.  maybe we should look
into modifying the Write function to support that mode as well?

[noelutz, 2011-06-06 21:59:55]

On 2011/06/04 21:06:01, darin wrote:
> http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
>
http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h#newco...
> base/file_util_proxy.h:71: //   base::PLATFORM_FILE_CREATE_ALWAYS |
> i think having to specify PLATFORM_FILE_TEMPORARY makes the API redundant. 
the
> method name is CreateTemporary after all!
> 
> how about we rename file_flags to additional_file_flags (or
> optional_file_flags), and then we document that CREATE_ALWAYS, WRITE, and
> TEMPORARY bits comprise the default file flags.  this way the callers can pass
0
> if they don't need to specify anything special.
> 
> so i would expect to see CreateTemporary(loop, 0, callback) in many cases or
> CreateTemporary(loop, PLATFORM_FILE_ASYNC, callback) in cases where we want to
> open the file for async IO.

Much much nicer.  Thanks.

> finally, it occurs to me that fixing this method to make it possible to not
set
> PLATFORM_FILE_ASYNC doesn't really fix the original issue you were trying to
> solve.  the problem you had was that the Write method cannot handle a
> PlatformFile that was opened with PLATFORM_FILE_ASYNC.  maybe we should look
> into modifying the Write function to support that mode as well?

Sure?  I'm fine with using synchronous file operations but I agree that it would
be nice if Write() supported async file operations (same for Read, actually). 
Maybe I can do that in a separate CL?

I'm not familiar with Chrome's IO code but it looks to me that we could simply
use net::FileStream to implement the proxy Write() function.  It seems silly to
re-implement all the asynchronous write logic (Read should probably do the
same).  Do you see any disadvantages in adding this dependency?  The only issue
is that the FileStream constructor takes the open flags.  To deal with that we
could maybe have two different write functions:  e.g., WriteSync() and
WriteAsync().  These two functions would virtually be identical except that the
latter would always pass in PLATFORM_FILE_ASYNC to the FileStream even if it
wasn't set by the client.  That would defer to the client the responsibility of
opening the file properly.  Thoughts?

[Sam Kerner (Chrome), 2011-06-06 22:19:27]

http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h#newco...
base/file_util_proxy.h:71: //   base::PLATFORM_FILE_CREATE_ALWAYS |
On 2011/06/04 21:06:01, darin wrote:
> i think having to specify PLATFORM_FILE_TEMPORARY makes the API redundant. 
the
> method name is CreateTemporary after all!
> 
> how about we rename file_flags to additional_file_flags (or
> optional_file_flags), and then we document that CREATE_ALWAYS, WRITE, and
> TEMPORARY bits comprise the default file flags.  this way the callers can pass
0
> if they don't need to specify anything special.
> 
> so i would expect to see CreateTemporary(loop, 0, callback) in many cases or
> CreateTemporary(loop, PLATFORM_FILE_ASYNC, callback) in cases where we want to
> open the file for async IO.
> 
> finally, it occurs to me that fixing this method to make it possible to not
set
> PLATFORM_FILE_ASYNC doesn't really fix the original issue you were trying to
> solve.  the problem you had was that the Write method cannot handle a
> PlatformFile that was opened with PLATFORM_FILE_ASYNC.  maybe we should look
> into modifying the Write function to support that mode as well?

I looked in to doing this a while ago, and I think I added a TODO around the
Write method.  If you want to do this, I can point you to the docs and show you
a unit test that can exercise it.

[noelutz, 2011-06-07 01:57:38]

On 2011/06/06 22:19:27, Sam Kerner (Chrome) wrote:
> http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
>
http://codereview.chromium.org/7066067/diff/3004/base/file_util_proxy.h#newco...
> base/file_util_proxy.h:71: //   base::PLATFORM_FILE_CREATE_ALWAYS |
> On 2011/06/04 21:06:01, darin wrote:
> > i think having to specify PLATFORM_FILE_TEMPORARY makes the API redundant. 
> the
> > method name is CreateTemporary after all!
> > 
> > how about we rename file_flags to additional_file_flags (or
> > optional_file_flags), and then we document that CREATE_ALWAYS, WRITE, and
> > TEMPORARY bits comprise the default file flags.  this way the callers can
pass
> 0
> > if they don't need to specify anything special.
> > 
> > so i would expect to see CreateTemporary(loop, 0, callback) in many cases or
> > CreateTemporary(loop, PLATFORM_FILE_ASYNC, callback) in cases where we want
to
> > open the file for async IO.
> > 
> > finally, it occurs to me that fixing this method to make it possible to not
> set
> > PLATFORM_FILE_ASYNC doesn't really fix the original issue you were trying to
> > solve.  the problem you had was that the Write method cannot handle a
> > PlatformFile that was opened with PLATFORM_FILE_ASYNC.  maybe we should look
> > into modifying the Write function to support that mode as well?
> 
> I looked in to doing this a while ago, and I think I added a TODO around the
> Write method.  If you want to do this, I can point you to the docs and show
you
> a unit test that can exercise it.

I think this CL removes the TODO you're talking about (see url_fetcher.cc
changes).  I may have some time towards the end of the quarter to do the
suggested change.  In the short them this CL fixes my issue but I gladly help
fixing the broader one.

[Sam Kerner (Chrome), 2011-06-08 19:03:54]

On 2011/06/03 17:51:45, darin wrote:
> http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
>
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
> base/file_util_proxy.h:73: bool aync,
> please avoid adding bool parameters like this.  it makes the callsite very
> unreadable.  you just see true/false, with no idea what it means.  it is
better
> to define an enum.
> 
> another option is to create a separate function name, which describes the
> variant you want.
> 
> however, i also want to ask:  why are you using FileUtilProxy::Write and not
> net::FileStream to write to a file?

I did not know about net::FileStream.  Yes, that would be a better solution. 
Will fix.

[noelutz, 2011-06-09 19:22:20]

ping?
thanks,
noé.

On 2011/06/08 19:03:54, Sam Kerner (Chrome) wrote:
> On 2011/06/03 17:51:45, darin wrote:
> > http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
> > File base/file_util_proxy.h (right):
> > 
> >
>
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
> > base/file_util_proxy.h:73: bool aync,
> > please avoid adding bool parameters like this.  it makes the callsite very
> > unreadable.  you just see true/false, with no idea what it means.  it is
> better
> > to define an enum.
> > 
> > another option is to create a separate function name, which describes the
> > variant you want.
> > 
> > however, i also want to ask:  why are you using FileUtilProxy::Write and not
> > net::FileStream to write to a file?
> 
> I did not know about net::FileStream.  Yes, that would be a better solution. 
> Will fix.

[Sam Kerner (Chrome), 2011-06-10 03:33:36]

LGTM.  Darin's LGTM is needed because he is in base/OWNERS, and I am not.

On 2011/06/09 19:22:20, noelutz wrote:
> ping?
> thanks,
> noé.
> 
> On 2011/06/08 19:03:54, Sam Kerner (Chrome) wrote:
> > On 2011/06/03 17:51:45, darin wrote:
> > > http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h
> > > File base/file_util_proxy.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7066067/diff/2004/base/file_util_proxy.h#newco...
> > > base/file_util_proxy.h:73: bool aync,
> > > please avoid adding bool parameters like this.  it makes the callsite very
> > > unreadable.  you just see true/false, with no idea what it means.  it is
> > better
> > > to define an enum.
> > > 
> > > another option is to create a separate function name, which describes the
> > > variant you want.
> > > 
> > > however, i also want to ask:  why are you using FileUtilProxy::Write and
not
> > > net::FileStream to write to a file?
> > 
> > I did not know about net::FileStream.  Yes, that would be a better solution.

> > Will fix.

[darin (slow to review), 2011-06-11 07:00:37]

LGTM

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:8: #include "base/platform_file.h"
this include is unnecessary.  it already comes from file_util_proxy.h

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.h#newco...
base/file_util_proxy.h:74: // net::FileStream.
nit: it is risky to refer to another module like this.  what happens if
net::FileStream is renamed?  will people know to look here to fix the comment? 
I'd probably just drop the "for net::FileStream" part.

[noelutz, 2011-06-13 16:52:08]

Done.  Thanks.  Will send to commit bot shortly.

noe.

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:8: #include "base/platform_file.h"
On 2011/06/11 07:00:37, darin wrote:
> this include is unnecessary.  it already comes from file_util_proxy.h

Done.

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/7066067/diff/5005/base/file_util_proxy.h#newco...
base/file_util_proxy.h:74: // net::FileStream.
On 2011/06/11 07:00:37, darin wrote:
> nit: it is risky to refer to another module like this.  what happens if
> net::FileStream is renamed?  will people know to look here to fix the comment?

> I'd probably just drop the "for net::FileStream" part.

Done.

[commit-bot: I haz the power, 2011-06-13 18:18:09]

Try job failure for 7066067-16003 on win for step pyauto_functional_tests:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[M-A Ruel, 2011-06-13 18:50:04]

On 2011/06/13 18:18:09, I haz the power (commit-bot) wrote:
> Try job failure for 7066067-16003 on win for step pyauto_functional_tests:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...


Sorry for the pyauto failure, I've removed it from the try jobs and I've checked
the commit box again.

[commit-bot: I haz the power, 2011-06-13 20:29:54]

Change committed as 88884