In BlobURLRequestJob, open files asynchronously to avoid blocking the IO thread

In BlobURLRequestJob, open files asynchronously to avoid blocking the IO thread
(and tripping thread restriction asserts).

The bug was found while trying to get FileWriter ui_tests to pass
(see http://codereview.chromium.org/6609040/).

This change also changes all callers to pass in a file_thread_proxy so
the class can assume it's there and pass it to FileUtilProxy.

Finally, it adds a BlobURLRequestJob test case that reads a file larger
than the buffer size, to better exercise the job's behavior when ReadRawData() 
is called multiple times.

BUG=75548
TEST=test_shell_tests,ui_tests

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

[adamk, 2011-03-09 23:35:54]

Oops, there's a major bug in this, since I failed to handle the case of a file
bigger than the read buffer...reworking now.  And hopefully adding a unittest to
catch this case.

[adamk, 2011-03-10 00:44:44]

On 2011/03/09 23:35:54, Adam Klein wrote:
> Oops, there's a major bug in this, since I failed to handle the case of a file
> bigger than the read buffer...reworking now.  And hopefully adding a unittest
to
> catch this case.

Okay, ready for a look now.

[jianli, 2011-03-10 22:50:42]

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job.cc:254: int
BlobURLRequestJob::ComputeBytesToRead() {
Since we do not change anything in this method, we should make it as const
method.

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job.cc:292: return DispatchReadFile(item,
bytes_to_read);
Since the produced bytes_to_read is not immediately used in DispatchReadFile, we
can postpone calling ComputeBytesToRead to when it is needed. That is, you can
remove bytes_to_read parameter from ReadBytes and DispatchReadFile and call
ComputeBytesToRead inside ReadBytes and ReadFile.

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job_unittest.cc:292: std::string large_data;
Might be more efficient to call reserve to preallocate for kBufferSize * 5
capacity.

[adamk, 2011-03-10 23:06:12]

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job.cc:254: int
BlobURLRequestJob::ComputeBytesToRead() {
On 2011/03/10 22:50:42, jianli wrote:
> Since we do not change anything in this method, we should make it as const
> method.

Done.

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job.cc:292: return DispatchReadFile(item,
bytes_to_read);
On 2011/03/10 22:50:42, jianli wrote:
> Since the produced bytes_to_read is not immediately used in DispatchReadFile,
we
> can postpone calling ComputeBytesToRead to when it is needed. That is, you can
> remove bytes_to_read parameter from ReadBytes and DispatchReadFile and call
> ComputeBytesToRead inside ReadBytes and ReadFile.

Well, we already have it here, and it's needed above in order to check for
bytes_to_read == 0.   And DispatchToRead() does pass it to ReadFile().  So
passing it to Dispatch saves us one call to Compute.

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
webkit/blob/blob_url_request_job_unittest.cc:292: std::string large_data;
On 2011/03/10 22:50:42, jianli wrote:
> Might be more efficient to call reserve to preallocate for kBufferSize * 5
> capacity.

Seems slightly like overkill for a unittest, but I've added it, as it will save
a few allocations/copies.

[jianli, 2011-03-10 23:20:26]

On Thu, Mar 10, 2011 at 3:06 PM, <adamk@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
> File webkit/blob/blob_url_request_job.cc (right):
>
>
>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
> webkit/blob/blob_url_request_job.cc:254: int
> BlobURLRequestJob::ComputeBytesToRead() {
> On 2011/03/10 22:50:42, jianli wrote:
>
>> Since we do not change anything in this method, we should make it as
>>
> const
>
>> method.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
> webkit/blob/blob_url_request_job.cc:292: return DispatchReadFile(item,
> bytes_to_read);
> On 2011/03/10 22:50:42, jianli wrote:
>
>> Since the produced bytes_to_read is not immediately used in
>>
> DispatchReadFile, we
>
>> can postpone calling ComputeBytesToRead to when it is needed. That is,
>>
> you can
>
>> remove bytes_to_read parameter from ReadBytes and DispatchReadFile and
>>
> call
>
>> ComputeBytesToRead inside ReadBytes and ReadFile.
>>
>
> Well, we already have it here, and it's needed above in order to check
> for bytes_to_read == 0.   And DispatchToRead() does pass it to
> ReadFile().  So passing it to Dispatch saves us one call to Compute.
>
> Yes, you're right. My initial intention is to avoid
calling ComputeBytesToRead again in DidOpen because we're not able to
pass bytes_to_read from DispatchReadFile to DidOpen. Will it be a good idea
to save bytes_to_read as a member variable to avoid the re-computation for
first-time read?


>
>
>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
> File webkit/blob/blob_url_request_job_unittest.cc (right):
>
>
>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
> webkit/blob/blob_url_request_job_unittest.cc:292: std::string
> large_data;
> On 2011/03/10 22:50:42, jianli wrote:
>
>> Might be more efficient to call reserve to preallocate for kBufferSize
>>
> * 5
>
>> capacity.
>>
>
> Seems slightly like overkill for a unittest, but I've added it, as it
> will save a few allocations/copies.
>
>
> http://codereview.chromium.org/6612051/
>

[adamk, 2011-03-11 00:00:11]

On Thu, Mar 10, 2011 at 3:19 PM, Jian Li <jianli@chromium.org> wrote:

>
>
> On Thu, Mar 10, 2011 at 3:06 PM, <adamk@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>> File webkit/blob/blob_url_request_job.cc (right):
>>
>>
>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>> webkit/blob/blob_url_request_job.cc:254: int
>> BlobURLRequestJob::ComputeBytesToRead() {
>> On 2011/03/10 22:50:42, jianli wrote:
>>
>>> Since we do not change anything in this method, we should make it as
>>>
>> const
>>
>>> method.
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>> webkit/blob/blob_url_request_job.cc:292: return DispatchReadFile(item,
>> bytes_to_read);
>> On 2011/03/10 22:50:42, jianli wrote:
>>
>>> Since the produced bytes_to_read is not immediately used in
>>>
>> DispatchReadFile, we
>>
>>> can postpone calling ComputeBytesToRead to when it is needed. That is,
>>>
>> you can
>>
>>> remove bytes_to_read parameter from ReadBytes and DispatchReadFile and
>>>
>> call
>>
>>> ComputeBytesToRead inside ReadBytes and ReadFile.
>>>
>>
>> Well, we already have it here, and it's needed above in order to check
>> for bytes_to_read == 0.   And DispatchToRead() does pass it to
>> ReadFile().  So passing it to Dispatch saves us one call to Compute.
>>
>> Yes, you're right. My initial intention is to avoid
> calling ComputeBytesToRead again in DidOpen because we're not able to
> pass bytes_to_read from DispatchReadFile to DidOpen. Will it be a good idea
> to save bytes_to_read as a member variable to avoid the re-computation for
> first-time read?
>

Done, stored bytes_to_read_ in a member variable.  It strikes me as a little
unclean, but it's no worse than the rest of the object state that's already
being kept in member variables.


>
>
>>
>>
>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>> File webkit/blob/blob_url_request_job_unittest.cc (right):
>>
>>
>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>> webkit/blob/blob_url_request_job_unittest.cc:292: std::string
>> large_data;
>> On 2011/03/10 22:50:42, jianli wrote:
>>
>>> Might be more efficient to call reserve to preallocate for kBufferSize
>>>
>> * 5
>>
>>> capacity.
>>>
>>
>> Seems slightly like overkill for a unittest, but I've added it, as it
>> will save a few allocations/copies.
>>
>>
>> http://codereview.chromium.org/6612051/
>>
>
>

[michaeln, 2011-03-11 06:03:47]

just a drive-by comment, jian is looking already so when it  lgt_him it lgtm

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
webkit/blob/blob_url_request_job.cc:116: void
BlobURLRequestJob::ResolveFile(const FilePath& file_path) {
nit: maybe DCHECK(file_thread_proxy_)

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
webkit/blob/blob_url_request_job.cc:369: DidRead(rv);
ah... is this what your new large test is about... if this also fixes a bug with
reading large blobs maybe mention that in the CL description too.

[jianli, 2011-03-11 19:18:13]

LGTM.

On Thu, Mar 10, 2011 at 4:00 PM, Adam Klein <adamk@chromium.org> wrote:

> On Thu, Mar 10, 2011 at 3:19 PM, Jian Li <jianli@chromium.org> wrote:
>
>>
>>
>> On Thu, Mar 10, 2011 at 3:06 PM, <adamk@chromium.org> wrote:
>>
>>>
>>>
>>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>>> File webkit/blob/blob_url_request_job.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>>> webkit/blob/blob_url_request_job.cc:254: int
>>> BlobURLRequestJob::ComputeBytesToRead() {
>>> On 2011/03/10 22:50:42, jianli wrote:
>>>
>>>> Since we do not change anything in this method, we should make it as
>>>>
>>> const
>>>
>>>> method.
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>>> webkit/blob/blob_url_request_job.cc:292: return DispatchReadFile(item,
>>> bytes_to_read);
>>> On 2011/03/10 22:50:42, jianli wrote:
>>>
>>>> Since the produced bytes_to_read is not immediately used in
>>>>
>>> DispatchReadFile, we
>>>
>>>> can postpone calling ComputeBytesToRead to when it is needed. That is,
>>>>
>>> you can
>>>
>>>> remove bytes_to_read parameter from ReadBytes and DispatchReadFile and
>>>>
>>> call
>>>
>>>> ComputeBytesToRead inside ReadBytes and ReadFile.
>>>>
>>>
>>> Well, we already have it here, and it's needed above in order to check
>>> for bytes_to_read == 0.   And DispatchToRead() does pass it to
>>> ReadFile().  So passing it to Dispatch saves us one call to Compute.
>>>
>>> Yes, you're right. My initial intention is to avoid
>> calling ComputeBytesToRead again in DidOpen because we're not able to
>> pass bytes_to_read from DispatchReadFile to DidOpen. Will it be a good idea
>> to save bytes_to_read as a member variable to avoid the re-computation for
>> first-time read?
>>
>
> Done, stored bytes_to_read_ in a member variable.  It strikes me as a
> little unclean, but it's no worse than the rest of the object state that's
> already being kept in member variables.
>
>
>>
>>
>>>
>>>
>>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>>> File webkit/blob/blob_url_request_job_unittest.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/6612051/diff/4001/webkit/blob/blob_url_request...
>>> webkit/blob/blob_url_request_job_unittest.cc:292: std::string
>>> large_data;
>>> On 2011/03/10 22:50:42, jianli wrote:
>>>
>>>> Might be more efficient to call reserve to preallocate for kBufferSize
>>>>
>>> * 5
>>>
>>>> capacity.
>>>>
>>>
>>> Seems slightly like overkill for a unittest, but I've added it, as it
>>> will save a few allocations/copies.
>>>
>>>
>>> http://codereview.chromium.org/6612051/
>>>
>>
>>
>

[adamk, 2011-03-14 17:42:14]

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
webkit/blob/blob_url_request_job.cc:116: void
BlobURLRequestJob::ResolveFile(const FilePath& file_path) {
On 2011/03/11 06:03:47, michaeln wrote:
> nit: maybe DCHECK(file_thread_proxy_)

Added that DCHECK to the constructor (that'll make it useful, as it will
actually include the errant caller in the stack trace).

http://codereview.chromium.org/6612051/diff/10003/webkit/blob/blob_url_reques...
webkit/blob/blob_url_request_job.cc:369: DidRead(rv);
On 2011/03/11 06:03:47, michaeln wrote:
> ah... is this what your new large test is about... if this also fixes a bug
with
> reading large blobs maybe mention that in the CL description too.

Added something to the CL description.  There wasn't a bug in the original code,
but my initial version of this change _did_ have a bug.  The test is a good idea
to leave in the CL nonetheless (unless you think I should split it into another
change).