Add a value for end-to-end reloads in Cache policy

Add a value for ent-to-end reloads in Cache policy

This CL adds a value for end-to-end reload in the cache policy. This will allow
WebURLLoader to set the right load flag (net::LOAD_BYPASSING_CACHE) when
sending the resource request to the browser, without blink having to set any
HTPP cache header ("Pragma: no-cache", "Cache-Control: no-cache").
https://chromiumcodereview.appspot.com/308083009/ is the Chrome CL that handles the new value.

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

[clamy, 2014-05-30 18:20:07]

PTAL: this is the second patch in a series of 3 to introduce a new value in
blink cache policy enums. Here we really introduce it :). Note that an
additional patch beyond these three will have to actually set it on the right
occasion.

[ppi, 2014-06-01 23:57:04]

Makes sense overall, please find a few questions below. Thanks!

https://codereview.chromium.org/309693002/diff/1/Source/core/loader/FrameFetc...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/309693002/diff/1/Source/core/loader/FrameFetc...
Source/core/loader/FrameFetchContext.cpp:94: if (request.cachePolicy() ==
ReloadIgnoringCacheData
Is this going away with patches 4/5?

https://codereview.chromium.org/309693002/diff/1/Source/core/loader/FrameLoad...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/309693002/diff/1/Source/core/loader/FrameLoad...
Source/core/loader/FrameLoader.cpp:653: if
(request.resourceRequest().cachePolicy() == ReloadIgnoringCacheData ||
request.resourceRequest().cachePolicy() == ReloadBypassingCache)
I see that this just preserves the current behavior, but I wonder why we never
return FrameLoadTypeReloadFromOrigin here, and if it does matter at all. Wdyt?

https://codereview.chromium.org/309693002/diff/1/Source/platform/network/Reso...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/309693002/diff/1/Source/platform/network/Reso...
Source/platform/network/ResourceRequest.h:46: ReloadBypassingCache, // shift
reload
nit: maybe go for "// shift reload / end-to-end reload" to match the flag name ?

https://codereview.chromium.org/309693002/diff/1/Source/web/tests/WebFrameTes...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/309693002/diff/1/Source/web/tests/WebFrameTes...
Source/web/tests/WebFrameTest.cpp:4830:
EXPECT_TRUE(frame->dataSource()->request().cachePolicy() ==
WebURLRequest::ReloadIgnoringCacheData
I think we'd prefer to be precise about the expected value. Are all these calls
to cachePolicy() going to move from ReloadIgnoringCacheData to
ReloadBypassingCache with CLs 4 and 5?

[clamy, 2014-06-02 05:54:47]

Thanks for the review :)!

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/core/loader/Fr...
File Source/core/loader/FrameFetchContext.cpp (right):

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/core/loader/Fr...
Source/core/loader/FrameFetchContext.cpp:94: if (request.cachePolicy() ==
ReloadIgnoringCacheData
On 2014/06/01 23:57:04, ppi wrote:
> Is this going away with patches 4/5?

Yes with patch 4. Patch 5 will remove some special case in the flattening of
HTTP headers in WebURLLoaderImpl.

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/core/loader/Fr...
File Source/core/loader/FrameLoader.cpp (right):

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/core/loader/Fr...
Source/core/loader/FrameLoader.cpp:653: if
(request.resourceRequest().cachePolicy() == ReloadIgnoringCacheData ||
request.resourceRequest().cachePolicy() == ReloadBypassingCache)
On 2014/06/01 23:57:04, ppi wrote:
> I see that this just preserves the current behavior, but I wonder why we never
> return FrameLoadTypeReloadFromOrigin here, and if it does matter at all. Wdyt?

Good point. We could not return FrameLoadTypeReloadFromOrigin based on a single
FrameRequest before, because we decide that we have a
FrameLoadTypeReloadFromOrigin based on a parameter to FrameLoader::reload. But
now we can, so I have update the statement to reflect that.

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/platform/netwo...
File Source/platform/network/ResourceRequest.h (right):

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/platform/netwo...
Source/platform/network/ResourceRequest.h:46: ReloadBypassingCache, // shift
reload
On 2014/06/01 23:57:04, ppi wrote:
> nit: maybe go for "// shift reload / end-to-end reload" to match the flag name
?

Done.

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/web/tests/WebF...
File Source/web/tests/WebFrameTest.cpp (right):

https://chromiumcodereview.appspot.com/309693002/diff/1/Source/web/tests/WebF...
Source/web/tests/WebFrameTest.cpp:4830:
EXPECT_TRUE(frame->dataSource()->request().cachePolicy() ==
WebURLRequest::ReloadIgnoringCacheData
On 2014/06/01 23:57:04, ppi wrote:
> I think we'd prefer to be precise about the expected value. Are all these
calls
> to cachePolicy() going to move from ReloadIgnoringCacheData to
> ReloadBypassingCache with CLs 4 and 5?

I think I'll handle that in patch 4, where we'll know which value to expect.

[ppi, 2014-06-03 14:56:31]

lgtm, thanks!

[clamy, 2014-06-03 15:01:04]

@abarth, japhet: PTAL.
This is the second patch in a series of 5 aiming at removing the setting of
cache HTTP headers in blink (and rely on the network stack instead). Currently
blink does not distinguish between LOAD_VALIDATE_CACHE and LOAD_BYPASS_CACHE in
its caching policy, and relies on setting "Pragma: no-cache" and "Cache-control:
no-cache headers instead. This patch introduces a new value for the cache
policy. In the third patch, WebURLLoaderImpl uses it to set the right load
flags.

@ppi: Thanks!

[Nate Chapin, 2014-06-03 16:18:42]

LGTM

+eseidel for public/ review

[eseidel, 2014-06-03 16:44:15]

rslgtm

[clamy, 2014-06-03 16:44:52]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-06-03 16:45:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/clamy@chromium.org/309693002/20001

[commit-bot: I haz the power, 2014-06-03 16:48:24]

Change committed as 175386