Do not initiate revalidation of Resource with redirects from MemoryCache

Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971
Committed: https://crrev.com/64ad58e1d8451857f6a763651727c9d39375ea3b
Cr-Commit-Position: refs/heads/master@{#397064}

[hiroshige, 2016-05-27 06:15:54]

Description was changed from

==========
Do not revalidate Resource with redirects

BUG=613971
==========

to

==========
Do not revalidate Resource with redirects

BUG=613971
==========

[hiroshige, 2016-05-27 06:15:58]

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

[commit-bot: I haz the power, 2016-05-27 06:16:32]

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

[commit-bot: I haz the power, 2016-05-27 06:53:27]

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

[commit-bot: I haz the power, 2016-05-27 06:53:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-05-27 07:03:40]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, kinuko@chromium.org, yhirano@chromium.org

[hiroshige, 2016-05-27 08:01:50]

PTAL.

See the crbug issue for details/background.
I want to merge this to beta/stable.

[kinuko, 2016-05-27 12:20:24]

lgtm

(looks reasonable to me, but would like to hear Nate's opinion too)

[Nate Chapin, 2016-05-27 17:00:58]

https://codereview.chromium.org/2011283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2011283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:965: if
(!redirectChain().isEmpty())
How does this interact with canReuseRedirectChain()?

[hiroshige, 2016-05-30 06:28:50]

https://codereview.chromium.org/2011283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2011283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:965: if
(!redirectChain().isEmpty())
On 2016/05/27 17:00:57, Nate Chapin wrote:
> How does this interact with canReuseRedirectChain()?

This CL prohibits |Revalidate| for Resource with redirects, but not |Use| for
such a Resource.
Therefore, canReuseRedirectChain() still can return true for Resource with
redirects (meaning "we can |Use|"), while canUseCacheValidator() always returns
false (meaning "we cannot |Revalidate|").
In such case,
- We can |Use| the Resource without revalidating (when other conditions met).
- We can NOT |Revalidate| the Resource, because of Chromium/Blink's
implementation.
- We can |Reload| the Resource.

[yhirano, 2016-05-30 07:52:29]

lgtm

[Nate Chapin, 2016-05-31 19:50:48]

LGTM, but once this is no longer a pressing issue, can we take a closer look to
see if we can remove this restriction?

[kinuko, 2016-06-01 00:25:19]

On 2016/05/31 19:50:48, Nate Chapin wrote:
> LGTM, but once this is no longer a pressing issue, can we take a closer look
to
> see if we can remove this restriction?

Yes, I assume this is a shorter term bandaid that gives us more time to think
about better longer term solution.

[hiroshige, 2016-06-01 03:01:49]

Description was changed from

==========
Do not revalidate Resource with redirects

BUG=613971
==========

to

==========
Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971
==========

[hiroshige, 2016-06-01 03:17:17]

On 2016/06/01 00:25:19, kinuko wrote:
> On 2016/05/31 19:50:48, Nate Chapin wrote:
> > LGTM, but once this is no longer a pressing issue, can we take a closer look
> to
> > see if we can remove this restriction?
> 
> Yes, I assume this is a shorter term bandaid that gives us more time to think
> about better longer term solution.

Yes. Filed crbug.com/616338.

[hiroshige, 2016-06-01 03:18:00]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-06-01 03:18:26]

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

[commit-bot: I haz the power, 2016-06-01 05:15:58]

Description was changed from

==========
Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971
==========

to

==========
Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971
==========

[commit-bot: I haz the power, 2016-06-01 05:15:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-01 05:17:05]

Description was changed from

==========
Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971
==========

to

==========
Do not initiate revalidation of Resource with redirects from MemoryCache

BUG=613971

Committed: https://crrev.com/64ad58e1d8451857f6a763651727c9d39375ea3b
Cr-Commit-Position: refs/heads/master@{#397064}
==========

[commit-bot: I haz the power, 2016-06-01 05:17:06]

Patchset 1 (id:??) landed as
https://crrev.com/64ad58e1d8451857f6a763651727c9d39375ea3b
Cr-Commit-Position: refs/heads/master@{#397064}