FrameFetchContext should respect BypassingCache for main resources

FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it can respect
BypassingCache rather than checking the parent policy because
the parent always return BypassingCache in such cases.

The parent frame can have a different policy if we have an UI
to trigger a per-frame bypassing reload. But we do not and won't today.
Even we have a per-frame bypassing reload, this change should be still
reasonable.

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

Review-Url: https://codereview.chromium.org/2767413002
Cr-Commit-Position: refs/heads/master@{#460662}
Committed: https://chromium.googlesource.com/chromium/src/+/4faad2d582a5ee0ed6381d0139bc5333407769d3

[Takashi Toyoshima, 2017-03-23 06:37:30]

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:37: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:53:44]

Description was changed from

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it should respect
BypassingCache rather than the parent policy.

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

to

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it should respect
BypassingCache rather than the parent policy.

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

[Takashi Toyoshima, 2017-03-23 06:53:44]

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

[commit-bot: I haz the power, 2017-03-23 06:54:38]

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

[commit-bot: I haz the power, 2017-03-23 06:54:42]

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

[kinuko, 2017-03-23 07:26:16]

While the change looks good none of our tests catch any of these changes make me
worry a bit... did you say you have a plan to add some tests, would they cover
these?

[Takashi Toyoshima, 2017-03-23 08:06:41]

I guess the reason we do not have any test for such cases are this is not a real
use case.
This will need an UI to trigger a per-frame hard reload, but we don't have such
UIs and won't.
Otherwise, if a frame requires bypassing cache, it means the parent also
requires bypassing cache. This is the current situation I think.

So, "should respect" may be a confusing description. It can be "do not need to
check parent policy if FrameLoadType is BypassingCache because it will return
BypassingCache".

[Takashi Toyoshima, 2017-03-23 08:11:24]

Description was changed from

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it should respect
BypassingCache rather than the parent policy.

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

to

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it can respect
BypassingCache rather than checking the parent policy because
the parent always return BypassingCache in such cases.

The parent frame can have a different policy if we have an UI
to trigger a per-frame bypassing reload. But we do not and won't today.
Even we have a per-frame bypassing reload, this change should be still
reasonable.

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

[Takashi Toyoshima, 2017-03-23 08:13:21]

I updated the description.

Even so, sounds like the new behavior is preferable.
So, I will add a test for this case.

[kinuko, 2017-03-23 08:13:23]

On 2017/03/23 08:06:41, Takashi Toyoshima wrote:
> I guess the reason we do not have any test for such cases are this is not a
real
> use case.
> This will need an UI to trigger a per-frame hard reload, but we don't have
such
> UIs and won't.
> Otherwise, if a frame requires bypassing cache, it means the parent also
> requires bypassing cache. This is the current situation I think.
> 
> So, "should respect" may be a confusing description. It can be "do not need to
> check parent policy if FrameLoadType is BypassingCache because it will return
> BypassingCache".

I see, thanks for explanation. Yeah providing such rationale in the description
helps.

lgtm if you update the description

(Also could you add the reason why we don't have corresponding test in the
description for every similar CL you uploaded? That'd be very helpful)

[Takashi Toyoshima, 2017-03-29 09:31:22]

PTAL. I added a simple test in Patch Set 3

[yhirano, 2017-03-29 09:39:23]

lgtm

[Nate Chapin, 2017-03-29 18:49:47]

lgtm

[kinuko, 2017-03-30 01:44:41]

lgtm (still), thanks for adding tests!

[Takashi Toyoshima, 2017-03-30 02:44:26]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:44:56]

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-03-30 04:19:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490841866106750,
"parent_rev": "655f015cb0d07d8272eeaed0266f0eb0b7f2482d", "commit_rev":
"4faad2d582a5ee0ed6381d0139bc5333407769d3"}

[commit-bot: I haz the power, 2017-03-30 04:20:28]

Description was changed from

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it can respect
BypassingCache rather than checking the parent policy because
the parent always return BypassingCache in such cases.

The parent frame can have a different policy if we have an UI
to trigger a per-frame bypassing reload. But we do not and won't today.
Even we have a per-frame bypassing reload, this change should be still
reasonable.

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

to

==========
FrameFetchContext should respect BypassingCache for main resources

Existing code respects a parent frame's policy rather than
FrameLoadTypeReloadBypassingCache for main resources.

This isn't consistent to other sub-resources, and it can respect
BypassingCache rather than checking the parent policy because
the parent always return BypassingCache in such cases.

The parent frame can have a different policy if we have an UI
to trigger a per-frame bypassing reload. But we do not and won't today.
Even we have a per-frame bypassing reload, this change should be still
reasonable.

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

Review-Url: https://codereview.chromium.org/2767413002
Cr-Commit-Position: refs/heads/master@{#460662}
Committed:
https://chromium.googlesource.com/chromium/src/+/4faad2d582a5ee0ed6381d0139bc...
==========

[commit-bot: I haz the power, 2017-03-30 04:20:29]

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