Add write and read/write support to FileStream (renamed from FileInputStream)...

* Add write and read/write support to FileStream (renamed from FileInputStream).
* Moved net/disk_cache/os_file to base/platform_file.


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

[Erik does not do reviews, 2008-10-28 20:33:59]

I haven't tested on Linux/Mac yet (try server doesn't like svn cp/mv), but I
figured I'd get the review kicked off since you'll likely have other feedback.

[darin (slow to review), 2008-10-29 20:52:22]

LGTM with the following adjustments:

http://codereview.chromium.org/8843/diff/252/267
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/252/267#newcode30
Line 30: enum ReadWrite {
AccessMode might be a better name for this enum.

Then you could have ACCESS_READ, ACCESS_WRITE, and ACCESS_READ_WRITE

http://codereview.chromium.org/8843/diff/252/267#newcode50
Line 50: int Open(const std::wstring& path, bool asynchronous_mode, ReadWrite
rw);
nit: would be nice to put the AccessMode parameter second.

http://codereview.chromium.org/8843/diff/252/267#newcode90
Line 90: // must be passed to this method.  In asynchronous mode, if the read
could
s/read/write/

http://codereview.chromium.org/8843/diff/252/261
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/8843/diff/252/261#newcode88
Line 88: flags = O_WRONLY | O_CREAT | O_TRUNC;
i predict consumers wanting control over truncation, but we can cross that
bridge when we get to it.

http://codereview.chromium.org/8843/diff/252/263
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/8843/diff/252/263#newcode145
Line 145: attrs_and_flags = FILE_FLAG_WRITE_THROUGH;
are you sure about setting this flag?  that will be very costly in terms of
performance right?  perhaps it would be better to not specify that flag, and
then instead add a method to fsync so that consumers can have some control.

[Erik does not do reviews, 2008-10-30 18:00:58]

New snapshot uploaded.  I'll submit when the tree reopens.

http://codereview.chromium.org/8843/diff/252/261
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/8843/diff/252/261#newcode88
Line 88: flags = O_WRONLY | O_CREAT | O_TRUNC;
On 2008/10/29 20:52:22, darin wrote:
> i predict consumers wanting control over truncation, but we can cross that
> bridge when we get to it.

Yep.  I was going for simple initially.  I added a comment to the header file
for WRITE_ONLY so that this is more clear.

http://codereview.chromium.org/8843/diff/252/263
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/8843/diff/252/263#newcode145
Line 145: attrs_and_flags = FILE_FLAG_WRITE_THROUGH;
On 2008/10/29 20:52:22, darin wrote:
> are you sure about setting this flag?  that will be very costly in terms of
> performance right?  perhaps it would be better to not specify that flag, and
> then instead add a method to fsync so that consumers can have some control.

You're right.  I didn't read the flag thoroughly enough.  Do you think any other
flags will help the write only case?  It didn't look like it to me, so I set it
to 0.

[darin (slow to review), 2008-10-30 18:13:55]

http://codereview.chromium.org/8843/diff/57/72
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/57/72#newcode9
Line 9: // TODO(erikkay): any reason we shouldn't support multiple async
operations?
the API contract makes this hard to do.  what is the file cursor set to if you
call Read again after a previous Read returned ERR_IO_PENDING?  the API says
that the number of bytes read by a call to Read is only reported upon
completion.

if we wanted to support multiple overlapping reads/writes, then we'd probably
want Read/Write methods to take an offset parameter.

http://codereview.chromium.org/8843/diff/57/72#newcode31
Line 31: ACCESS_READ_ONLY  = 0,
bikeshed nit: why ACCESS_READ_ONLY and not the shorter ACCESS_READ?  i suggested
that not only because it is shorter but also so that ACCESS_READ_WRITE resembled
the combination of _READ and _WRITE.

http://codereview.chromium.org/8843/diff/57/72#newcode50
Line 50: int Open(const std::wstring& path, AccessMode rw, bool
asynchronous_mode);
style nit: google style recommends that variable names be actual words unless
the abbreviation is extremely commonplace.  i think "rw" doesn't rise to that
level.  "access_mode" seems like a better choice for this parameter name.

http://codereview.chromium.org/8843/diff/57/68
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/8843/diff/57/68#newcode145
Line 145: // TODO(erikkay): do any of the flags (SEQUENTIAL?) help here?
you should ask ricardo about this.

http://codereview.chromium.org/8843/diff/57/68#newcode147
Line 147: } else { // ACCESS_READ_WRITE
style nit: need two spaces in front of the "//"

[Erik does not do reviews, 2008-10-30 18:40:21]

http://codereview.chromium.org/8843/diff/57/72
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/57/72#newcode9
Line 9: // TODO(erikkay): any reason we shouldn't support multiple async
operations?
On 2008/10/30 18:13:55, darin wrote:
> the API contract makes this hard to do.  what is the file cursor set to if you
> call Read again after a previous Read returned ERR_IO_PENDING?  the API says
> that the number of bytes read by a call to Read is only reported upon
> completion.
> 
> if we wanted to support multiple overlapping reads/writes, then we'd probably
> want Read/Write methods to take an offset parameter.

I agree we'd need to add a position argument to Read/Write. In that world, we'd
banish the use of the current position since seek position is part of the
OVERLAPPED structure.

http://codereview.chromium.org/8843/diff/57/72#newcode31
Line 31: ACCESS_READ_ONLY  = 0,
On 2008/10/30 18:13:55, darin wrote:
> bikeshed nit: why ACCESS_READ_ONLY and not the shorter ACCESS_READ?  i
suggested
> that not only because it is shorter but also so that ACCESS_READ_WRITE
resembled
> the combination of _READ and _WRITE.

Well, that was why I was explicit.  It kind of looks like it could be the
combination of the two, and I wanted to be explicit that this was an enum and
not a bitfield.  I could make it a bitfield if you think that's clearer.

[rvargas (doing something else), 2008-10-30 20:10:09]

http://codereview.chromium.org/8843/diff/76/286
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/8843/diff/76/286#newcode145
Line 145: // TODO(erikkay): do any of the flags (SEQUENTIAL?) help here?
Not really. Both flags are only hints to the OS (sequential and random access).
In general, it's better not to give a hint unless we know for sure what the
access pattern is going to be. When there is no hint, the OS will adjust what it
does based on the observed pattern. If there is a hint, the process may be
skewed on the wrong direction and it makes it harder to recover and go to the
right algorithm.

http://codereview.chromium.org/8843/diff/76/286#newcode150
Line 150: attrs_and_flags = FILE_FLAG_RANDOM_ACCESS;
As a general comment, now that we are making this class much more general, maybe
we should instead include the code from os_file.h that is already fiddling with
flags to open a file. We'll just have to add an extra flag for overlapped.

[Erik does not do reviews, 2008-10-31 18:23:34]

Per our offline discussion, I made the following changes:
* moved net/disk_cache/os_file to base/platform_file
* changed file_stream to use platform_file to create the underlying file
* changed PlatformFile to default to read/write sharing, with flags to disable
sharing
* changed PlatformFile to take an async flag

Obviously, this wound up adding a bunch of other files to the CL.

[darin (slow to review), 2008-10-31 19:36:35]

LGreat!

http://codereview.chromium.org/8843/diff/330/156
File base/platform_file.h (right):

http://codereview.chromium.org/8843/diff/330/156#newcode21
Line 21: const PlatformFile INVALID_HANDLE_VALUE = -1;
Q: is this constant actually necessary?  maybe it should be
kInvalidPlatformFileValue?

http://codereview.chromium.org/8843/diff/330/156#newcode31
Line 31: PLATFORM_FILE_NOSHARE_READ = 64,  // NOSHARE is opposite of Windows
SHARE
nit: google style would probably say that you should write NO_SHARE since they
are separate words.  you could also use EXCLUSIVE if you wanted a single word.

http://codereview.chromium.org/8843/diff/330/158
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/158#newcode17
Line 17: bool* created) {
Q: should there be a TODO about supporting NOSHARE_{READ,WRITE} ?  or should
that be something we never support at this level?

http://codereview.chromium.org/8843/diff/330/143
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/330/143#newcode104
Line 104: int open_flags_;
why do we need the open_flags_ member?

http://codereview.chromium.org/8843/diff/330/137
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/137#newcode83
Line 83: open_flags_ = open_flags;
why do we need to store open_flags_ ?

http://codereview.chromium.org/8843/diff/330/159
File net/disk_cache/file_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/159#newcode47
Line 47: return (-1 != platform_file_);
is this where you want kInvalidPlatformFileValue ?

[rvargas (doing something else), 2008-10-31 20:16:15]

nice.

http://codereview.chromium.org/8843/diff/330/156
File base/platform_file.h (right):

http://codereview.chromium.org/8843/diff/330/156#newcode21
Line 21: const PlatformFile INVALID_HANDLE_VALUE = -1;
On 2008/10/31 19:36:35, darin wrote:
> Q: is this constant actually necessary?  maybe it should be
> kInvalidPlatformFileValue?

This constant was actually introduced by mmoss, but probably we should improve
the name :)

http://codereview.chromium.org/8843/diff/330/143
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/330/143#newcode9
Line 9: // TODO(erikkay): support multiple simultaneous async operations (add
offset arg
Do we really want to do this?. I'd be tempted to say that for this use case we
have disk_cache/file. (sorry if I'm missing something said previously)

http://codereview.chromium.org/8843/diff/330/143#newcode81
Line 81: // If opened with |asynchronous_mode| set to true, then a non-null
callback
nit: This comment needs updating.

http://codereview.chromium.org/8843/diff/330/143#newcode100
Line 100: // the FileStream was opened with asynchronous_mode set to true.
same here

http://codereview.chromium.org/8843/diff/330/137
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/137#newcode85
Line 85: if (file_ == -1) {
kInvalidPlatformFile ?

http://codereview.chromium.org/8843/diff/330/137#newcode94
Line 94: return file_ != -1;
kInvalidPlatformFile ?

[Erik does not do reviews, 2008-10-31 20:39:02]

Assuming no objections, I'll submit this snapshot.

http://codereview.chromium.org/8843/diff/330/156
File base/platform_file.h (right):

http://codereview.chromium.org/8843/diff/330/156#newcode21
Line 21: const PlatformFile INVALID_HANDLE_VALUE = -1;
On 2008/10/31 19:36:35, darin wrote:
> Q: is this constant actually necessary?  maybe it should be
> kInvalidPlatformFileValue?

I had lost track of this one.  Fixed.

http://codereview.chromium.org/8843/diff/330/156#newcode31
Line 31: PLATFORM_FILE_NOSHARE_READ = 64,  // NOSHARE is opposite of Windows
SHARE
On 2008/10/31 19:36:35, darin wrote:
> nit: google style would probably say that you should write NO_SHARE since they
> are separate words.  you could also use EXCLUSIVE if you wanted a single word.

changed to EXCLUSIVE

http://codereview.chromium.org/8843/diff/330/158
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/158#newcode17
Line 17: bool* created) {
On 2008/10/31 19:36:35, darin wrote:
> Q: should there be a TODO about supporting NOSHARE_{READ,WRITE} ?  or should
> that be something we never support at this level?

Yeah, I'm not sure it will ever make sense at this level, but I added a TODO to
answer that question.

http://codereview.chromium.org/8843/diff/330/143
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/330/143#newcode9
Line 9: // TODO(erikkay): support multiple simultaneous async operations (add
offset arg
On 2008/10/31 20:16:15, rvargas wrote:
> Do we really want to do this?. I'd be tempted to say that for this use case we
> have disk_cache/file. (sorry if I'm missing something said previously)

I removed the comment.

http://codereview.chromium.org/8843/diff/330/143#newcode104
Line 104: int open_flags_;
On 2008/10/31 19:36:35, darin wrote:
> why do we need the open_flags_ member?

I was using it to DCHECK Read and Write if the file wasn't opened properly. 
It's also possible that the POSIX impl could use to support the
NOSHARE/EXCLUSIVE stuff transparently.

http://codereview.chromium.org/8843/diff/330/137
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/137#newcode83
Line 83: open_flags_ = open_flags;
On 2008/10/31 19:36:35, darin wrote:
> why do we need to store open_flags_ ?

POSIX impl currently doesn't use it.  I'm initializing it for correctness in
case it does later.

http://codereview.chromium.org/8843/diff/330/159
File net/disk_cache/file_posix.cc (right):

http://codereview.chromium.org/8843/diff/330/159#newcode47
Line 47: return (-1 != platform_file_);
On 2008/10/31 19:36:35, darin wrote:
> is this where you want kInvalidPlatformFileValue ?

yeah, I couldn't find where this was defined before, so I ditched it.  After
fixing kInvalidPlatformFileValue, I switched to that.

[darin (slow to review), 2008-10-31 20:41:58]

http://codereview.chromium.org/8843/diff/330/143
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/330/143#newcode104
Line 104: int open_flags_;
but that's something you can do by null checking async_context_.  see the
comment above async_context_.

[darin (slow to review), 2008-10-31 20:46:59]

LGTM

http://codereview.chromium.org/8843/diff/330/143
File net/base/file_stream.h (right):

http://codereview.chromium.org/8843/diff/330/143#newcode104
Line 104: int open_flags_;
doh, nevermind.  LGTM

[rvargas (doing something else), 2008-10-31 20:55:21]

LGTM