Supports single range request with file protocol...

Supports single range request with file protocol
Added range request support in URLRequestFileJob to respect
"Range" HTTP header. Fail with ERR_REQUESTED_RANGE_NOT_SATISFIABLE
if range is bad.
The following range request modes are supported:
1. Fully specified: bytes=x-y
2. With first byte position only: bytes=x-
3. With suffix length: bytes=-y

Multiple ranges in a single request is not supported as we need to multipart encoding..

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

[rvargas (doing something else), 2009-04-27 22:39:45]

http://codereview.chromium.org/92149/diff/2011/3010
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/92149/diff/2011/3010#newcode142
Line 142: DCHECK(remaining_bytes_ >= 0);
nit:DCHECK_GE

http://codereview.chromium.org/92149/diff/2011/3010#newcode152
Line 152: DCHECK(remaining_bytes_ >= 0);
nit DCHECK_GE

http://codereview.chromium.org/92149/diff/2011/3010#newcode196
Line 196: // cause we need to do multipart encoding here.
nit: because

http://codereview.chromium.org/92149/diff/2011/3010#newcode234
Line 234: if (!byte_range_.ComputeBounds(file_info.size, &lower_bound_,
&upper_bound_)) {
nit: 80 chars?

http://codereview.chromium.org/92149/diff/2011/3010#newcode240
Line 240: // Do the seek at the beginning.
nit: Do you mean at the beginning of the request?. Maybe just "Seek to the start
offset" is more clear.

http://codereview.chromium.org/92149/diff/2011/3010#newcode249
Line 249: DCHECK(remaining_bytes_ > 0);
nit: shouldn't we do something for remaining_bytes_ == 0? (preferably avoiding
the seek).

http://codereview.chromium.org/92149/diff/2011/3011
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/92149/diff/2011/3011#newcode15
Line 15: #include "net/http/http_util.h"
nit: we don't need this one here, do we?

http://codereview.chromium.org/92149/diff/2011/3011#newcode49
Line 49: int64 upper_bound_;
It seems like it would be better to have lower_bound_ and upper-bound_ be part
of byte_range_, so this object only deals with one member that holds
range-related info.

To do this, HttpByteRange::ComputeBounds() would have to "fix" its internal
members instead of returning values to external variables. After that, this
object could find out about the upper_bound by calling
byte_range_.last_byte_position(). It doesn't make a very significant difference
here, but if you really want to implement multiple ranges it would make things
easier.
The only problem would be if you want to keep ComputeBounds() as const for some
other use case.

http://codereview.chromium.org/92149/diff/2011/3013
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/92149/diff/2011/3013#newcode632
Line 632: const size_t buffer_size = 400000;
Do we really need to generate that much IO for this test?

http://codereview.chromium.org/92149/diff/2011/3013#newcode648
Line 648: std::string(buffer.get()).substr(first_byte_position, content_length);
nit: Avoid the extra object by constructing the string directly from
buffer[first_byte_pos]

http://codereview.chromium.org/92149/diff/2011/3013#newcode676
Line 676: const size_t buffer_size = 400000;
nit: same thing here.

[Alpha Left Google, 2009-04-28 02:21:09]

http://codereview.chromium.org/92149/diff/2011/3010
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/92149/diff/2011/3010#newcode142
Line 142: DCHECK(remaining_bytes_ >= 0);
On 2009/04/27 22:39:45, rvargas wrote:
> nit:DCHECK_GE

Done.

http://codereview.chromium.org/92149/diff/2011/3010#newcode152
Line 152: DCHECK(remaining_bytes_ >= 0);
On 2009/04/27 22:39:45, rvargas wrote:
> nit DCHECK_GE

Done.

http://codereview.chromium.org/92149/diff/2011/3010#newcode196
Line 196: // cause we need to do multipart encoding here.
On 2009/04/27 22:39:45, rvargas wrote:
> nit: because

Done.

http://codereview.chromium.org/92149/diff/2011/3010#newcode234
Line 234: if (!byte_range_.ComputeBounds(file_info.size, &lower_bound_,
&upper_bound_)) {
On 2009/04/27 22:39:45, rvargas wrote:
> nit: 80 chars?

Done.

http://codereview.chromium.org/92149/diff/2011/3010#newcode240
Line 240: // Do the seek at the beginning.
On 2009/04/27 22:39:45, rvargas wrote:
> nit: Do you mean at the beginning of the request?. Maybe just "Seek to the
start
> offset" is more clear.

Done.

http://codereview.chromium.org/92149/diff/2011/3010#newcode249
Line 249: DCHECK(remaining_bytes_ > 0);
On 2009/04/27 22:39:45, rvargas wrote:
> nit: shouldn't we do something for remaining_bytes_ == 0? (preferably avoiding
> the seek).

the seek is avoided if |first_byte_position| == 0, this DCHECK should be >=
anyway..

Done.

http://codereview.chromium.org/92149/diff/2011/3011
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/92149/diff/2011/3011#newcode15
Line 15: #include "net/http/http_util.h"
On 2009/04/27 22:39:45, rvargas wrote:
> nit: we don't need this one here, do we?

Done.

http://codereview.chromium.org/92149/diff/2011/3011#newcode49
Line 49: int64 upper_bound_;
On 2009/04/27 22:39:45, rvargas wrote:
> It seems like it would be better to have lower_bound_ and upper-bound_ be part
> of byte_range_, so this object only deals with one member that holds
> range-related info.
> 
> To do this, HttpByteRange::ComputeBounds() would have to "fix" its internal
> members instead of returning values to external variables. After that, this
> object could find out about the upper_bound by calling
> byte_range_.last_byte_position(). It doesn't make a very significant
difference
> here, but if you really want to implement multiple ranges it would make things
> easier.
> The only problem would be if you want to keep ComputeBounds() as const for
some
> other use case.

I think ComputeBounds to have side effect is a good idea, I'll do it.

http://codereview.chromium.org/92149/diff/2011/3013
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/92149/diff/2011/3013#newcode632
Line 632: const size_t buffer_size = 400000;
On 2009/04/27 22:39:45, rvargas wrote:
> Do we really need to generate that much IO for this test?
Can be less, do it with 4k.

http://codereview.chromium.org/92149/diff/2011/3013#newcode648
Line 648: std::string(buffer.get()).substr(first_byte_position, content_length);
On 2009/04/27 22:39:45, rvargas wrote:
> nit: Avoid the extra object by constructing the string directly from
> buffer[first_byte_pos]

Done.

[rvargas (doing something else), 2009-04-28 17:28:54]

LGTM

http://codereview.chromium.org/92149/diff/2011/3010
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/92149/diff/2011/3010#newcode249
Line 249: DCHECK(remaining_bytes_ > 0);
On 2009/04/28 02:21:09, Alpha wrote:
> On 2009/04/27 22:39:45, rvargas wrote:
> > nit: shouldn't we do something for remaining_bytes_ == 0? (preferably
avoiding
> > the seek).
> 
> the seek is avoided if |first_byte_position| == 0, this DCHECK should be >=
> anyway..
> 
> Done.

What I meant was that if remaining_bytes == 0, there is nothing to do. I guess
that doing an extra seek is not that bad for an uncommon case.

[Alpha Left Google, 2009-04-28 18:41:54]

On 2009/04/28 17:28:54, rvargas wrote:
> LGTM
> 
> http://codereview.chromium.org/92149/diff/2011/3010
> File net/url_request/url_request_file_job.cc (right):
> 
> http://codereview.chromium.org/92149/diff/2011/3010#newcode249
> Line 249: DCHECK(remaining_bytes_ > 0);
> On 2009/04/28 02:21:09, Alpha wrote:
> > On 2009/04/27 22:39:45, rvargas wrote:
> > > nit: shouldn't we do something for remaining_bytes_ == 0? (preferably
> avoiding
> > > the seek).
> > 
> > the seek is avoided if |first_byte_position| == 0, this DCHECK should be >=
> > anyway..
> > 
> > Done.
> 
> What I meant was that if remaining_bytes == 0, there is nothing to do. I guess
> that doing an extra seek is not that bad for an uncommon case.

Done too.

[rvargas (doing something else), 2009-04-28 22:59:54]

OK