Reuse images already loaded in the current document more aggressively

Reuse images already loaded in the current document more aggressively

Specifically, Vary headers should not prevent us from reusing an image that
already has been used in the current document.

BUG=518733, 520376
TEST=ResourceFetcherTest.VaryImage
NOPRESUBMIT=true

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

[Nate Chapin, 2015-08-14 23:33:38]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org

[Nate Chapin, 2015-08-14 23:33:39]

NOPRESUBMIT=true because this trips over a check that forbids new usage of
serverAsynchronousMockedRequest(). Per PRESUBMIT.PY, this is to prevent bad
interactions with the threaded html parser. But at this layer, there is no html
parser, so that concern is irrelevant.

https://codereview.chromium.org/1287073007/diff/1/Source/core/fetch/ResourceF...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1287073007/diff/1/Source/core/fetch/ResourceF...
Source/core/fetch/ResourceFetcher.cpp:577: if (type == Resource::Image &&
existingResource == cachedResource(request.url()))
Moved here to make it equivalent to preloading. I'm reasonably confident it's OK
to make this check higher priority than all of the things it now precedes.

[Yoav Weiss, 2015-08-15 05:13:51]

On 2015/08/14 23:33:39, Nate Chapin wrote:
> NOPRESUBMIT=true because this trips over a check that forbids new usage of
> serverAsynchronousMockedRequest(). Per PRESUBMIT.PY, this is to prevent bad
> interactions with the threaded html parser. But at this layer, there is no
html
> parser, so that concern is irrelevant.
> 
>
https://codereview.chromium.org/1287073007/diff/1/Source/core/fetch/ResourceF...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1287073007/diff/1/Source/core/fetch/ResourceF...
> Source/core/fetch/ResourceFetcher.cpp:577: if (type == Resource::Image &&
> existingResource == cachedResource(request.url()))
> Moved here to make it equivalent to preloading. I'm reasonably confident it's
OK
> to make this check higher priority than all of the things it now precedes.

LGTM.

Failing tests seem relevant though.

[Nate Chapin, 2015-08-17 20:47:09]

PTAL

2 changes:
* Moved Vary header priority down in determining RevalidationPolicy to be below
images already loading in the document, rather than moving images up to Vary.
* This changes the result of ResourceFetcherTest.VaryOnBack to use a cached
resource instead of reloading it. I think this is desirable, since it is a bit
strange that Vary: headers get to force a reload when that privilege is usually
limited to a few special cases (potential security issues with CORS loading,
technical limitations, user-initiated hard reloads, etc).

[Mike West, 2015-08-18 17:48:02]

On 2015/08/17 at 20:47:09, japhet wrote:
> PTAL
> 
> 2 changes:
> * Moved Vary header priority down in determining RevalidationPolicy to be
below images already loading in the document, rather than moving images up to
Vary.
> * This changes the result of ResourceFetcherTest.VaryOnBack to use a cached
resource instead of reloading it. I think this is desirable, since it is a bit
strange that Vary: headers get to force a reload when that privilege is usually
limited to a few special cases (potential security issues with CORS loading,
technical limitations, user-initiated hard reloads, etc).

Both of these make sense to me. Thanks for calling them out. LGTM.

[Nate Chapin, 2015-08-18 17:48:28]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-08-18 17:48:28]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1287073007/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-08-18 17:48:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287073007/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287073007/20001

[commit-bot: I haz the power, 2015-08-18 18:59:33]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200744