Reland 132134 with memory fix - Use LocalFileReader in FileSystemURLRequestJob

Reland 132134 with memory fix - Use LocalFileReader in FileSystemURLRequestJob

LocalFileReader change https://chromiumcodereview.appspot.com/10065011
with memory fix http://codereview.chromium.org/10067031/

BUG=113300, 114999, 123302
TEST=FileSystemURLRequestJobTest.*

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

[kinuko, 2012-04-13 10:09:21]

Merged the reverted change and the test fix for leak.

[kinuko, 2012-04-13 12:11:06]

On 2012/04/13 10:09:21, kinuko wrote:
> Merged the reverted change and the test fix for leak.

valgrind results look good now.  Ok to reland?

[adamk, 2012-04-13 15:19:22]

lgtm

On Fri, Apr 13, 2012 at 5:11 AM, <kinuko@chromium.org> wrote:

> On 2012/04/13 10:09:21, kinuko wrote:
>
>> Merged the reverted change and the test fix for leak.
>>
>
> valgrind results look good now.  Ok to reland?
>
>
http://codereview.chromium.**org/10080001/<http://codereview.chromium.org/100...
>

[michaeln, 2012-04-13 19:08:36]

lgtm

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
File webkit/fileapi/file_system_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
webkit/fileapi/file_system_url_request_job_unittest.cc:105:
MessageLoop::current()->RunAllPending();
Would it be sufficient to just add the call to RunAllPending() and not have the
extra pending_job_ reference?

[kinuko, 2012-04-16 02:29:40]

Thanks!

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
File webkit/fileapi/file_system_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
webkit/fileapi/file_system_url_request_job_unittest.cc:105:
MessageLoop::current()->RunAllPending();
On 2012/04/13 19:08:36, michaeln wrote:
> Would it be sufficient to just add the call to RunAllPending() and not have
the
> extra pending_job_ reference?

I expected so, but it seems we need to explicitly call Kill() before running
pending jobs to make sure we clean up everything in every test.

[tzik, 2012-04-16 02:57:55]

lgtm

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
File webkit/fileapi/file_system_url_request_job.cc (right):

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
webkit/fileapi/file_system_url_request_job.cc:101: base::Unretained(this)));
Can we expect LocalFileReader::Read() not to return rv>=0?
If so, could you add comment to LocalFileReader::Read()?

[kinuko, 2012-04-16 08:30:41]

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
File webkit/fileapi/file_system_url_request_job.cc (right):

http://codereview.chromium.org/10080001/diff/1/webkit/fileapi/file_system_url...
webkit/fileapi/file_system_url_request_job.cc:101: base::Unretained(this)));
On 2012/04/16 02:57:56, tzik wrote:
> Can we expect LocalFileReader::Read() not to return rv>=0?
> If so, could you add comment to LocalFileReader::Read()?

Oops, good catch this was actually not always true on Windows.  I revived the
code for rv>=0 cases both in FileSystemURLRequestJob and BlobURLRequestJob. 
(It'd great if we have a specialized test for such cases but I'm not sure if it
could be tested in a deterministic way.)