Http Cache: First pass of byte-range requests support....

Http Cache: First pass of byte-range requests support.

This is the first pass to implement support for range
requests and the asociated sparse cache entry.

It is disabled by default, and requires ENABLE_RANGE_SUPPORT
to be defined in order to activate the code: all the code
is compiled in, but Start() bypasses the cache for range
requests, and OnNetworkInfoAvailable ignores 206.

Big parts are still not implemented. Most notably we are
not modifying the response headers that we send to the
user, so the content-range and content-length info
are not correct.

BUG=12258
TEST=Unit tests.


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

[rvargas (doing something else), 2009-06-06 00:55:22]

Darin,

I just thought it was better to have smaller reviews so that's why this is far
from complete (but it has most of the mechanics in). I could also move the
helper class to a separate file, but it feels very tied to the transaction
implementation.

[rvargas (doing something else), 2009-06-09 03:35:14]

http://codereview.chromium.org/118345/diff/1002/14
File net/http/http_cache.cc (right):

http://codereview.chromium.org/118345/diff/1002/14#newcode1077
Line 1077: if (partial_.get() && !partial_->IsCurrentRangeCached()) {
Should we remove support for this case instead (just age, no etag)?

[darin (slow to review), 2009-06-11 17:23:42]

OK, LGTM

http://codereview.chromium.org/118345/diff/1002/14
File net/http/http_cache.cc (right):

http://codereview.chromium.org/118345/diff/1002/14#newcode134
Line 134: class PartialData {
nit: it would be good to pull this class out of this .cc file so that it can be
easily unit tested.  It doesn't seem to have any direct dependencies on other
stuff in this file.

http://codereview.chromium.org/118345/diff/1002/14#newcode966
Line 966: } else if (rv < 0) {
nit: no else after return

http://codereview.chromium.org/118345/diff/1002/14#newcode1077
Line 1077: if (partial_.get() && !partial_->IsCurrentRangeCached()) {
firefox only supports etags, but i think it is OK to support sending the
last-modified timestamp too.

it is important to note that If-range is only useful instead of
If-unmodified-since / If-match provided we are OK receiving a 200 instead of a
206 in response to our request.

http://codereview.chromium.org/118345/diff/1002/13
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/118345/diff/1002/13#newcode443
Line 443: if (!request->extra_headers.empty()) {
nit: how about returning early here instead to avoid indentation?

[rvargas (doing something else), 2009-06-11 22:30:51]

Thanks. All comments addressed.

> it is important to note that If-range is only useful instead of
> If-unmodified-since / If-match provided we are OK receiving a 200 instead of a
> 206 in response to our request.

Yes, I'm assuming for now that we'll handle 200 and let the caller cancel the
request if that's not the desired behavior. We'll see how this goes after adding
all the error handling.

[darin (slow to review), 2009-06-11 22:34:24]

OK, LGTM