Http cache: Make sure that issuing a byte range request

Http cache: Make sure that issuing a byte range request
does not prevent caching of responses that are not 200,
and does not delete the cached response if not really needed.

For example, we should be able to cache redirects when the
request is made using a byte range.

BUG=134267
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149069

[gavinp, 2012-07-30 18:35:07]

LGTM.  Great catch!

My comments are all nits. Do what you will with them.

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
net/http/http_cache_transaction.cc:1575: 
Is it worthwhile adding a DCHECK here that captures having called
partial_->PrepareCacheValidation()... here? I guess not, since
...::BeginCacheValidation() is only ever called from
DoCompletePartialCacheValidation() or BeginPartialCacheValidation(). But I am a
big freaked out by that dependency, I wish we could DCHECK.

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
net/http/http_cache_transaction.cc:1578: // Only force revalidation for sparse
or truncated entries.
I think the "Only" here is a bit confusing: skip_validation can be false for
other reasons, right, such as net::HttpCache::PLAYBACK?

Separately, I think we still skip validation if the request is for a range that
we have cached.. 

So I propose:

// Force revalidation for sparse or truncated entries if we aren't requesting
from the cached range.

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
net/http/http_cache_transaction.cc:1605: return DoRestartPartialRequest();
So this makes me sad because if we have a range cached, and issue a partial
request that includes that range, we just ignore the range we do have cached. Is
that right?

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_unittest.cc
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_unittest.c...
net/http/http_cache_unittest.cc:2785: RemoveMockTransaction(&transaction);
Nice. This test fails very nicely without the patch.

http://codereview.chromium.org/10829069/diff/1/net/http/partial_data.cc
File net/http/partial_data.cc (right):

http://codereview.chromium.org/10829069/diff/1/net/http/partial_data.cc#newco...
net/http/partial_data.cc:277: DVLOG(2) << "UpdateFromStoredHeaders size: " <<
resource_size_;
Should this DVLOG be deleted??

[rvargas (doing something else), 2012-07-30 21:38:52]

Thanks.

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
net/http/http_cache_transaction.cc:1578: // Only force revalidation for sparse
or truncated entries.
On 2012/07/30 18:35:07, gavinp wrote:
> I think the "Only" here is a bit confusing: skip_validation can be false for
> other reasons, right, such as net::HttpCache::PLAYBACK?

but this is not about skip_validation being false, but about ignoring the
regular logic for that and deciding to check anyway.

Updated the comment.

> 
> Separately, I think we still skip validation if the request is for a range
that
> we have cached.. 
> 
> So I propose:
> 
> // Force revalidation for sparse or truncated entries if we aren't requesting
> from the cached range.

http://codereview.chromium.org/10829069/diff/1/net/http/http_cache_transactio...
net/http/http_cache_transaction.cc:1605: return DoRestartPartialRequest();
On 2012/07/30 18:35:07, gavinp wrote:
> So this makes me sad because if we have a range cached, and issue a partial
> request that includes that range, we just ignore the range we do have cached.
Is
> that right?

Not unless we are unable to conditionalize the request.

We used to discard the cached copy as soon as the strong validator check failed
(when asked for a range). Now we wait to see if we actually have to issue a
conditional request.

http://codereview.chromium.org/10829069/diff/1/net/http/partial_data.cc
File net/http/partial_data.cc (right):

http://codereview.chromium.org/10829069/diff/1/net/http/partial_data.cc#newco...
net/http/partial_data.cc:277: DVLOG(2) << "UpdateFromStoredHeaders size: " <<
resource_size_;
On 2012/07/30 18:35:07, gavinp wrote:
> Should this DVLOG be deleted??

Not sure. I've used this one to get an idea of where something is amiss. But
more often than not I use some other technique rather than longs.