Add FileReader interface and implement FileSystemFileReader

Add FileReader interface and implement FileSystemFileReader.

- Split LocalFileReader into FileReader interface and LocalFileReader implementation
- Adding FileSystemFileReader as a default implementation for FileSystem files

The FileSystemFileReader implementation uses CreateSnapshotFile which may force downloading the entire file (as the initial generic version).  This would need to be changed in remote filesystem implementation.

BUG=114999
TEST=FileSystemURLRequestJobTest, BlobURLRequestJobTest

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

[michaeln, 2012-04-13 20:24:51]

lgtm, definitely like the direction your going with making a very abstract
reader interface

i have a few questions/comments but nothing much really

unittests for LocalFileReader and FileSystemFileReader would be good to have. We
get some coverage thru blob_url and filesystem_url request job test coverage,
but these classes look very testable in isolation.

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/file_reader.h
File webkit/blob/file_reader.h (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/file_reader.h#n...
webkit/blob/file_reader.h:28: // it internally opens (and seeks) the file if it
is not opened yet.
The middle two lines in this doc comment seem out of place on this abstract
interface. Consider moving them to the overriden methods on LocalFileReader and
FileSystemFileReader.

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
File webkit/blob/local_file_reader.cc (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.cc:206: weak_factory_.InvalidateWeakPtrs();
why this new call to InvalidateWeakPtrs, just curious

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
File webkit/blob/local_file_reader.h (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.h:58: // Otherwise, a negative error code is
returned (rv < 0 cases).
Looks like your changing the semantics of this method. Now it always returns the
full length of file_path whereas before it returned file_len - initial_offset.
Wondering why?

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.h:61: protected:
Are there any derivatives of this class that need access to these data members?
If not... keep it private?

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_reader.cc (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:74: const base::Closure&
post_task_closure,
? maybe call this the 'read_closure' or 'read_continuation_closure'

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:85:
weak_factory_.InvalidateWeakPtrs();
Same question about why call invalidate, is it important to do so?

The reason i'm asking is because i think it would be easy to neglect doing so
(in future changes)... if its important to do so code comments might help, and
if its not important to, maybe just don't do it to avoid a few lines of code
that could be confusing in the future.

[michaeln, 2012-04-13 21:14:59]

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_reader.cc (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:62: has_pending_operation_ = true;
maybe clarify this referes to a pending snapshot and not a pending read...
has_pending_snapshot_operation_

[kinuko, 2012-04-16 10:24:21]

As for the unit tests, given that I seem to be introducing some bugs around the
change (oops) I'll be adding some tests for LocalFileReader and
FileSystemFileReader.

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/file_reader.h
File webkit/blob/file_reader.h (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/file_reader.h#n...
webkit/blob/file_reader.h:28: // it internally opens (and seeks) the file if it
is not opened yet.
On 2012/04/13 20:24:51, michaeln wrote:
> The middle two lines in this doc comment seem out of place on this abstract
> interface. Consider moving them to the overriden methods on LocalFileReader
and
> FileSystemFileReader.

Done.

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
File webkit/blob/local_file_reader.cc (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.cc:206: weak_factory_.InvalidateWeakPtrs();
On 2012/04/13 20:24:51, michaeln wrote:
> why this new call to InvalidateWeakPtrs, just curious

Felt it may make things a bit clearer, but since we have another flag
(has_pending_open_) we wouldn't need this.  Dropped.

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
File webkit/blob/local_file_reader.h (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.h:58: // Otherwise, a negative error code is
returned (rv < 0 cases).
On 2012/04/13 20:24:51, michaeln wrote:
> Looks like your changing the semantics of this method. Now it always returns
the
> full length of file_path whereas before it returned file_len - initial_offset.
> Wondering why?

I thought always returning the file length would be more intuitive or less error
prone.  (On the other hand I'm feeling that we may want to revisit here if we
want to expose a method like this, GetLength() or Available() or something, on
FileReader.)

http://codereview.chromium.org/10038019/diff/7005/webkit/blob/local_file_read...
webkit/blob/local_file_reader.h:61: protected:
On 2012/04/13 20:24:51, michaeln wrote:
> Are there any derivatives of this class that need access to these data
members?
> If not... keep it private?

Nope, fixed.

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_reader.cc (right):

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:62: has_pending_operation_ = true;
On 2012/04/13 21:14:59, michaeln wrote:
> maybe clarify this referes to a pending snapshot and not a pending read...
> has_pending_snapshot_operation_

Done.

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:74: const base::Closure&
post_task_closure,
On 2012/04/13 20:24:51, michaeln wrote:
> ? maybe call this the 'read_closure' or 'read_continuation_closure'

Done.

http://codereview.chromium.org/10038019/diff/7005/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_reader.cc:85:
weak_factory_.InvalidateWeakPtrs();
On 2012/04/13 20:24:51, michaeln wrote:
> Same question about why call invalidate, is it important to do so?
> 
> The reason i'm asking is because i think it would be easy to neglect doing so
> (in future changes)... if its important to do so code comments might help, and
> if its not important to, maybe just don't do it to avoid a few lines of code
> that could be confusing in the future. 

Makes sense, dropped the line.

[michaeln, 2012-04-18 00:25:30]

still lgtm :)