Only make Range requests when the desired range doesn't cover the whole file.

Fix BufferedResourceLoader so it only makes Range requests when a subset of the file is needed.

BUG=74975
TEST=none

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

[acolwell GONE FROM CHROMIUM, 2011-04-07 22:56:48]

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source.cc:575: if (error == net::OK) {
loader_->partial_response() is not needed here anymore because the loader will
return an error code if a range was requested and we didn't get a partial
response.

[Ami GONE FROM CHROMIUM, 2011-04-08 03:02:10]

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source.cc:638:
host()->SetTotalBytes(total_bytes_);
Do you not need to check that host()!=NULL?

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:169: ON_CALL(*loader_,
range_supported())
Is there a test here for the case of a lying server (claims to support range but
returns 200 instead of 206)?

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:211: // whole file (ie. 0-).
s/)/ or 0-max)/

[scherkus (not reviewing), 2011-04-11 17:41:54]

also wondering about the "lying server" case but in instances like that I'd like
to blame the server :)

micro-nit + one question but other than that LGTM w/ nits and possible unit test
additions etc etc

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:385: // If we didn't know the
|instance_size_| we do now.
pedantic nit: the rest of this file seems to put if-statement comments before
the if as opposed to inside the if

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:649: last_byte_position_ ==
kPositionNotSpecified);
is it worth checking last_byte_position_ == content_length_ ?

if content_length_ is still kPositionNotSpecified, then we're good
if content_length_ is properly set, then we're also good (however I do find this
extremely unintuitive so perhaps better to not handle this case)

[scherkus (not reviewing), 2011-04-11 17:41:55]

also wondering about the "lying server" case but in instances like that I'd like
to blame the server :)

micro-nit + one question but other than that LGTM w/ nits and possible unit test
additions etc etc

[acolwell GONE FROM CHROMIUM, 2011-04-18 22:09:56]

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source.cc:638:
host()->SetTotalBytes(total_bytes_);
On 2011/04/08 03:02:10, Ami Fischman wrote:
> Do you not need to check that host()!=NULL?

Done.

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_d...
webkit/glue/media/buffered_data_source_unittest.cc:169: ON_CALL(*loader_,
range_supported())
On 2011/04/08 03:02:10, Ami Fischman wrote:
> Is there a test here for the case of a lying server (claims to support range
but
> returns 200 instead of 206)?

Done. Added ServerLiesAboutRangeSupport test to simulate this situation.

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:385: // If we didn't know the
|instance_size_| we do now.
On 2011/04/11 17:41:55, scherkus wrote:
> pedantic nit: the rest of this file seems to put if-statement comments before
> the if as opposed to inside the if

Done.

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:649: last_byte_position_ ==
kPositionNotSpecified);
On 2011/04/11 17:41:55, scherkus wrote:
> is it worth checking last_byte_position_ == content_length_ ?
> 
> if content_length_ is still kPositionNotSpecified, then we're good
> if content_length_ is properly set, then we're also good (however I do find
this
> extremely unintuitive so perhaps better to not handle this case)

I don't think this is worth checking. It would cause inconsistent behavior if
the the range was passed in the constructor and then a Read() for the same range
was issued. In the first request, it would use the range because we don't know
how long the file is yet. In the second request the range would not be used
because the file size is known. I can see this causing confusion at some point
down the line.

I suppose I could change the name of the method to something a little more
specific like IsUnspecifiedOrZeroToEndRange(). That seems a little long winded,
but more accurate.

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6815012/diff/2001/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:211: // whole file (ie. 0-).
On 2011/04/08 03:02:10, Ami Fischman wrote:
> s/)/ or 0-max)/
This method doesn't return true if 0-max is specified so I don't think this is
comment applies. I did update the comment to be more precise though and indicate
that it only returns true for an unspecified range or the 0- range.