Allow cache reuse of some requests with Cache-Control headers

Allow cache reuse of some requests with Cache-Control headers

Traditionally, we have carefully handled Cache-Control headers that were set on a ResourceResponse, but have pretended Cache-Control doesn't exist on ResourceRequests. As of http://src.chromium.org/viewvc/blink?view=rev&rev=165603, we do the smarter naive thing, and never allow resources to be reused from MemoryCache if the ResourceRequest includes a Cache-Control header.

This patch moves ResourceResponse's Cache-Control parsing to HTTPParsers and handles Cache-Control headers consistently, whether the header appears on a ResourceRequest or a ResourceResponse.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171178

[Nate Chapin, 2014-04-08 21:49:22]

abarth: PTAL

yhirano: Does this seem like a good interpretation of your intent in
http://src.chromium.org/viewvc/blink?view=revision&revision=165603 ?

[abarth-chromium, 2014-04-09 00:00:44]

LGTM

Do we have unit tests for the header parser?  If not, it might make sense to add
a few.

[yhirano, 2014-04-09 04:08:36]

https://codereview.chromium.org/229053004/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/229053004/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:953: if (cachePolicy ==
CachePolicyRevalidate || existingResource->mustRevalidateDueToCacheHeaders()) {
I think cacheControlContainsNoCache() should be checked not only for
existingResource->resourceRequest but also for request.

For example, if there is a cached resource without cache-control and a request
for the url with "Cache-Control: no-cache" is made, we should not use the cached
content while this CL does.

[Nate Chapin, 2014-04-09 19:00:07]

On 2014/04/09 00:00:44, abarth wrote:
> LGTM
> 
> Do we have unit tests for the header parser?  If not, it might make sense to
add
> a few.

I don't think so. I'll take a look to see what's involved, but I'm not sure
whether I'm well-equipped to do it. I didn't really take the time to understand
the details of the parser, just copy/pasted it and fixed compile issues.

[Nate Chapin, 2014-04-09 19:08:50]

On 2014/04/09 04:08:36, yhirano wrote:
>
https://codereview.chromium.org/229053004/diff/20001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/229053004/diff/20001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceFetcher.cpp:953: if (cachePolicy ==
> CachePolicyRevalidate || existingResource->mustRevalidateDueToCacheHeaders())
{
> I think cacheControlContainsNoCache() should be checked not only for
> existingResource->resourceRequest but also for request.
> 
> For example, if there is a cached resource without cache-control and a request
> for the url with "Cache-Control: no-cache" is made, we should not use the
cached
> content while this CL does.

Good point. Will add that.

[Nate Chapin, 2014-04-09 19:58:29]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-09 19:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/229053004/60001

[Nate Chapin, 2014-04-09 19:58:46]

On 2014/04/09 19:00:07, Nate Chapin wrote:
> On 2014/04/09 00:00:44, abarth wrote:
> > LGTM
> > 
> > Do we have unit tests for the header parser?  If not, it might make sense to
> add
> > a few.
> 
> I don't think so. I'll take a look to see what's involved, but I'm not sure
> whether I'm well-equipped to do it. I didn't really take the time to
understand
> the details of the parser, just copy/pasted it and fixed compile issues.

It wasn't bad at all, added a test.

[commit-bot: I haz the power, 2014-04-09 20:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:29:42]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[Nate Chapin, 2014-04-09 20:33:39]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:33:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/229053004/80001

[commit-bot: I haz the power, 2014-04-09 21:42:12]

Change committed as 171178