Reload cache control: remove redundant header insertion from Blink

Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0", "If-Modified-Since",
and "If-None-Match" headers for resource revalidation.
But we also use WebCachePolicy::ValidatingCacheData in such cases
so that //net handles such headers.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

Beforehand, I added some content_browsertests to verify the header
for several reloads.
https://codereview.chromium.org/1905873002/
https://codereview.chromium.org/1993603003/

And this change actually removes the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81250145552ba0/third_party/WebKit/WebCore/loader/FrameLoader.cpp

BUG=602900
TEST=content_browsertests --gtest_filter='Reload*.*'

[Takashi Toyoshima, 2016-05-18 03:53:17]

Description was changed from

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0" header for resource
revalidation. But we also use WebCachePolicy::ValidatingCacheData
in such cases, and should not add such header for requests that
have other cache policies.

If request has ValidatingCacheDqata flag, net uses "max-age=0", too.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

BUG=602900
==========

to

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0" header for resource
revalidation. But we also use WebCachePolicy::ValidatingCacheData
in such cases, and should not add such header for requests that
have other cache policies.

If request has ValidatingCacheData flag, net uses "max-age=0", too.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='ReloadCacheControlBrowserTest.*'
==========

[Takashi Toyoshima, 2016-05-18 05:12:06]

Description was changed from

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0" header for resource
revalidation. But we also use WebCachePolicy::ValidatingCacheData
in such cases, and should not add such header for requests that
have other cache policies.

If request has ValidatingCacheData flag, net uses "max-age=0", too.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='ReloadCacheControlBrowserTest.*'
==========

to

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0" header for resource
revalidation. But we also use WebCachePolicy::ValidatingCacheData
in such cases so that //net uses "max-age=0".

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

I added some content_browsertests to verify the header for reloads.
https://codereview.chromium.org/1905873002/

And this change actually remove the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='ReloadCacheControlBrowserTest.*'
==========

[Takashi Toyoshima, 2016-05-18 05:12:06]

toyoshim@chromium.org changed reviewers:
+ hiroshige@chromium.org, japhet@chromium.org

[Takashi Toyoshima, 2016-05-18 05:13:13]

Copy hiroshige's comments from https://codereview.chromium.org/1905873002/#msg23
----
ResourceRequest's http headers are observable in Blink (while the header added
in net// layer is not).
This can cause behavior changes, but as long as I briefly checked for Blink code
(search "Cache-Control" or "HTTPNames::Cache_Control" under
third_party/Webkit/Source/), there seems no such cases.
So the Blink code looks good.

I'm not sure about separation of cache-related logic in Chromium and Blink: this
CL makes Blink to set if-modified-since etc. but not to set cache-control.

https://codereview.chromium.org/1905873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1905873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:546: const
AtomicString& eTag = resource->response().httpHeaderField(HTTPNames::ETag);
How about adding a comment that says "cache-control: max-age=0" header is added
in net// layer?

[Takashi Toyoshima, 2016-05-18 09:41:25]

hiroshige: let me check if we can also remove other headers modification.
I guess If_Modified_Since and If_None_Match are handled in //net too.

[Takashi Toyoshima, 2016-05-18 10:33:47]

Yes, we can remove them safely too.
I remove them too and add some browser tests to cover both headers.

Hiroshige, can you take another look on Patch Set 5?

[Takashi Toyoshima, 2016-05-19 05:32:35]

Note: this isn't urgent, and I'll need to fix build errors on android.

[Takashi Toyoshima, 2016-05-19 08:10:39]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 08:10:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987973002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987973002/120001

[Takashi Toyoshima, 2016-05-19 08:13:54]

Description was changed from

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0" header for resource
revalidation. But we also use WebCachePolicy::ValidatingCacheData
in such cases so that //net uses "max-age=0".

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

I added some content_browsertests to verify the header for reloads.
https://codereview.chromium.org/1905873002/

And this change actually remove the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='ReloadCacheControlBrowserTest.*'
==========

to

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0", "If-Modified-Since",
and "If-Non-Match" headers for resource revalidation.
But we also use WebCachePolicy::ValidatingCacheData in such cases
so that //net handles such headers.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

Beforehand, I added some content_browsertests to verify the header
for several reloads.
https://codereview.chromium.org/1905873002/
https://codereview.chromium.org/1993603003/

And this change actually removes the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='Reload*.*'
==========

[Takashi Toyoshima, 2016-05-19 08:14:25]

Description was changed from

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0", "If-Modified-Since",
and "If-Non-Match" headers for resource revalidation.
But we also use WebCachePolicy::ValidatingCacheData in such cases
so that //net handles such headers.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

Beforehand, I added some content_browsertests to verify the header
for several reloads.
https://codereview.chromium.org/1905873002/
https://codereview.chromium.org/1993603003/

And this change actually removes the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='Reload*.*'
==========

to

==========
Reload cache control: remove redundant header insertion from Blink

Now Blink inserts "Cache-Control: max-age=0", "If-Modified-Since",
and "If-None-Match" headers for resource revalidation.
But we also use WebCachePolicy::ValidatingCacheData in such cases
so that //net handles such headers.

So we can safely remove this insertion from Blink, and it would help
to avoid unexpected cache control flag mismatches.

Beforehand, I added some content_browsertests to verify the header
for several reloads.
https://codereview.chromium.org/1905873002/
https://codereview.chromium.org/1993603003/

And this change actually removes the insertion code from Blink.

This is historical code that exists before Chrome was born,
there WebKit does not assume our net like backend for networking.

This is one origin though it has moved around in tens of refactorings
https://chromium.googlesource.com/chromium/src/+/21a8f9925eacbc8016e44bb1bd81...

BUG=602900
TEST=content_browsertests --gtest_filter='Reload*.*'
==========

[Takashi Toyoshima, 2016-05-19 08:27:20]

I double-checked code that refer ResourceRequest's headers.
Probably, I should modify ResourceRequest::cacheControlContainsNoCache() to
consider cache policy.

[hiroshige, 2016-05-31 11:44:46]

Some more questions:

1. Does //net adds if-modified-since headers etc. according to the corresponding
entry in the disk cache?
If so, does this CL remove if-modified-since headers if the corresponding entry
in the disk cache doesn't exist (e.g. already evicted) while MemoryCache
contains a Resource to revalidate?

2. Does //net adds if-modified-since headers etc. after a redirect is received?
For example, when [URL A] returns 308 (or another redirect status) to [URL B]
during revalidation:
Currently, IIUC ResourceFetcher adds if-modified-since header and the
if-modified-since header is added both the requests to [URL A] and [URL B].
After this CL, is the if-modified-since header added to the request to [URL B]?

https://codereview.chromium.org/1987973002/diff/140001/content/browser/loader...
File content/browser/loader/reload_cache_control_browsertest.cc (right):

https://codereview.chromium.org/1987973002/diff/140001/content/browser/loader...
content/browser/loader/reload_cache_control_browsertest.cc:164:
IN_PROC_BROWSER_TEST_F(ReloadCacheControlBrowserTest, BypassingReload) {
Memo: this test doesn't call ResourceFetcher::initializeRevalidation().