FrameFetchContext should not always respect conditional requests

FrameFetchContext should not always respect conditional requests

Existing logic respects conditional requests for sub-resources always.
But we should respect WebCachePolicy for the frame.

This old code might affect history navigation performance badly because
it validates cache entries for conditional resources on history navigation.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2771803002
Cr-Commit-Position: refs/heads/master@{#461685}
Committed: https://chromium.googlesource.com/chromium/src/+/c6f5a1533c9beb6bf91b01bcd998f810c0ec98ce

[Takashi Toyoshima, 2017-03-23 06:51:26]

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

[commit-bot: I haz the power, 2017-03-23 06:51:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Takashi Toyoshima, 2017-03-23 06:54:06]

Description was changed from

==========
FrameFetchContext should not always respect conditional requests

Existing logic respects conditional requests for sub-resources always.
But we should respect WebCachePolicy for the frame.

This old code might affect history navigation performance badly because
it validates cache entries for conditional resources on history navigation.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
FrameFetchContext should not always respect conditional requests

Existing logic respects conditional requests for sub-resources always.
But we should respect WebCachePolicy for the frame.

This old code might affect history navigation performance badly because
it validates cache entries for conditional resources on history navigation.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2017-03-23 06:54:07]

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

[commit-bot: I haz the power, 2017-03-23 07:14:07]

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

[commit-bot: I haz the power, 2017-03-23 07:14:08]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-28 21:39:31]

LGTM, but please add a unit test if at all possible.

[Nate Chapin, 2017-03-28 21:45:54]

https://codereview.chromium.org/2771803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2771803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:422:
request.isConditional()) {
Hmm...one other thought. Can this clause be merged be merged in to
determineWebCachePolicy(), or would that end up being messy?

[kinuko, 2017-03-29 05:54:00]

lgtm, I agree with Nate that it might be nice if we merge it in
determineWebCachePolicy but if it doesn't look messy

[Takashi Toyoshima, 2017-03-29 10:56:17]

https://codereview.chromium.org/2771803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2771803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:422:
request.isConditional()) {
Currently, determineWCP requires many parameters to be called. So just having
this logic here is better today.
If you mean merging this to determineFrameWCP, that would be easy, but currently
the function is designed to be called to determine per-frame WCP that should not
count resource specific conditions in. So, probably.

I want to revisit this once other TODOs are removed. determineWCP could be
simpler than now, and would have room to merge this.

I added some comments to determineWCP and determineFrameWCP to clarify current
design.

[Takashi Toyoshima, 2017-04-03 05:22:00]

If there is no objection for the Patch Set 3, I'd submit it.

[Takashi Toyoshima, 2017-04-04 10:44:43]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-04-04 10:44:44]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2771803002/#ps40001
(title: "tests")

[commit-bot: I haz the power, 2017-04-04 10:44:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 12:15:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491302683746090,
"parent_rev": "756f4ddeb12a503550c6409934a0a8ac80354276", "commit_rev":
"c6f5a1533c9beb6bf91b01bcd998f810c0ec98ce"}

[commit-bot: I haz the power, 2017-04-04 12:16:42]

Description was changed from

==========
FrameFetchContext should not always respect conditional requests

Existing logic respects conditional requests for sub-resources always.
But we should respect WebCachePolicy for the frame.

This old code might affect history navigation performance badly because
it validates cache entries for conditional resources on history navigation.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
FrameFetchContext should not always respect conditional requests

Existing logic respects conditional requests for sub-resources always.
But we should respect WebCachePolicy for the frame.

This old code might affect history navigation performance badly because
it validates cache entries for conditional resources on history navigation.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2771803002
Cr-Commit-Position: refs/heads/master@{#461685}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6f5a1533c9beb6bf91b01bcd998...
==========

[commit-bot: I haz the power, 2017-04-04 12:16:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c6f5a1533c9beb6bf91b01bcd998...