PlzNavigate: Fix wrong "Cache-Control" header.

PlzNavigate: Fix wrong "Cache-Control" header.

This CL modify the FrameLoadType of frames such that the
ReloadBypassingCache load type gets propagated to every subframe during
a reload.

It fixes ReloadCacheControlBrowserTest.BypassingReload test with
PlzNavigate. Every ReloadCacheControlbroserTest are passing now.

BUG=671545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

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

[arthursonzogni, 2017-05-05 15:53:00]

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

[commit-bot: I haz the power, 2017-05-05 15:53:34]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-05 15:55:08]

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

[arthursonzogni, 2017-05-05 15:55:22]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-05 15:55:32]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-05 16:11:52]

arthursonzogni@chromium.org changed reviewers:
+ japhet@chromium.org, nasko@chromium.org, toyoshim@chromium.org

[arthursonzogni, 2017-05-05 16:11:53]

I am speculatively publishing this CL. I don't have the results of "git cl try"
yet.

If the patch needs change, Nasko is interested in taking it and landing it today
if he can. That's why I publish it now instead of Monday.

If the bots are green. Please take a look. Thanks in advance!
--Arthur

https://codereview.chromium.org/2863683003/diff/20001/content/browser/loader/...
File content/browser/loader/reload_cache_control_browsertest.cc (right):

https://codereview.chromium.org/2863683003/diff/20001/content/browser/loader/...
content/browser/loader/reload_cache_control_browsertest.cc:181: ASSERT_EQ(8UL,
request_log_.size());
I broke it into two steps: initial load and reload. I was a little bit confused
the first time I look at the code. This change has nothing to do with this CL
however.

[commit-bot: I haz the power, 2017-05-05 17:10:36]

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

[commit-bot: I haz the power, 2017-05-05 17:10:36]

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

[Charlie Reis, 2017-05-05 18:00:52]

Description was changed from

==========
PlzNavigate: Fix wrong "Cache-Control" header.

This CL modify the FrameLoadType of frames such that the
ReloadBypassingCache load type gets propagated to every subframe during
a reload.

It fixes ReloadCacheControlBrowserTest.BypassingReload test with
PlzNavigate. Every ReloadCacheControlbroserTest are passing now.

BUG=671545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix wrong "Cache-Control" header.

This CL modify the FrameLoadType of frames such that the
ReloadBypassingCache load type gets propagated to every subframe during
a reload.

It fixes ReloadCacheControlBrowserTest.BypassingReload test with
PlzNavigate. Every ReloadCacheControlbroserTest are passing now.

BUG=671545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

[Charlie Reis, 2017-05-05 18:00:52]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2017-05-06 00:08:57]

https://codereview.chromium.org/2863683003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1718: case
kFrameLoadTypeInitialInChildFrame:
This looks wrong to me.  Why is kFrameLoadTypeInitialHistoryLoad excluded and
kFrameLoadTypeInitialInChildFrame included?  I think it should be the reverse
for deciding when to look up a history item for the frame.

[arthursonzogni, 2017-05-08 08:40:47]

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

[commit-bot: I haz the power, 2017-05-08 08:41:32]

Dry run: 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-05-08 09:22:36]

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

[commit-bot: I haz the power, 2017-05-08 09:22:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[arthursonzogni, 2017-05-09 09:30:12]

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

[arthursonzogni, 2017-05-09 09:30:24]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-09 09:30:34]

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-05-09 10:57:25]

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1715: // If we're moving in
the back/forward list, we might want to replace the
Modified code for bypassing cache look good to me, but may I ask a question to
understand existing code here?

Whey here we could not prefer cached content for history navigation as we are
doing in the legacy code path?

Also, is it fine that we have minor logic differences between this code path and
DetermineWebCachePolicy in FrameFetchContext.cpp? E.g., not checking http
method, not checking conditional request.

[commit-bot: I haz the power, 2017-05-09 12:46:00]

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

[commit-bot: I haz the power, 2017-05-09 12:46:01]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-05-09 12:49:06]

https://codereview.chromium.org/2863683003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1718: case
kFrameLoadTypeInitialInChildFrame:
On 2017/05/06 00:08:56, Charlie Reis wrote:
> This looks wrong to me.  Why is kFrameLoadTypeInitialHistoryLoad excluded and
> kFrameLoadTypeInitialInChildFrame included?  I think it should be the reverse
> for deciding when to look up a history item for the frame.

You are right. I made an error. The two case should be the ones in
IsBackForwardLoadType(load_type).

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1715: // If we're moving in
the back/forward list, we might want to replace the
On 2017/05/09 10:57:25, Takashi Toyoshima wrote:
> Modified code for bypassing cache look good to me, but may I ask a question to
> understand existing code here?
> 
> Whey here we could not prefer cached content for history navigation as we are
> doing in the legacy code path?
> 
> Also, is it fine that we have minor logic differences between this code path
and
> DetermineWebCachePolicy in FrameFetchContext.cpp? E.g., not checking http
> method, not checking conditional request.

I will let other peoples answer the questions because I am not really used to
this code.

FYI: With PlzNavigate, we are no more using the ResourceFetcher for loading the
frame. Thus the FrameFetchContext is not used for determining the right cache
flags. That's the main reason why it was not working before this patch.
Do you know what prevents us to determine the right FrameLoadType and the right
CachePolicy in the FrameLoader?

[Charlie Reis, 2017-05-10 05:56:41]

LGTM with nits.  Apologies that I don't follow the back/forward discussion--
feel free to follow up on that separately.

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1715: // If we're moving in
the back/forward list, we might want to replace the
On 2017/05/09 12:49:06, arthursonzogni wrote:
> On 2017/05/09 10:57:25, Takashi Toyoshima wrote:
> > Modified code for bypassing cache look good to me, but may I ask a question
to
> > understand existing code here?
> > 
> > Whey here we could not prefer cached content for history navigation as we
are
> > doing in the legacy code path?
> > 
> > Also, is it fine that we have minor logic differences between this code path
> and
> > DetermineWebCachePolicy in FrameFetchContext.cpp? E.g., not checking http
> > method, not checking conditional request.
> 
> I will let other peoples answer the questions because I am not really used to
> this code.
> 
> FYI: With PlzNavigate, we are no more using the ResourceFetcher for loading
the
> frame. Thus the FrameFetchContext is not used for determining the right cache
> flags. That's the main reason why it was not working before this patch.
> Do you know what prevents us to determine the right FrameLoadType and the
right
> CachePolicy in the FrameLoader?

Sorry, I don't follow any of these questions.  This CL doesn't appear to change
the back/forward cache behavior, which seems good to me.  (We use
kUseProtocolCachePolicy for kFrameLoadTypeBackForward and
kFrameLoadTypeInitialHistoryLoad both before and after the CL.)  If there's a
reason to change that, it should probably be done separately from this CL.

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1729: // If we're in a
middle of a reload. The FrameLoadType is propagated to
nit: Drop "If"

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1730: // its children only
if it is a ReloadByPassingCache, else it becomes a
nit: Don't capitalize P.

[arthursonzogni, 2017-05-10 07:56:04]

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

[commit-bot: I haz the power, 2017-05-10 07:56:55]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-10 08:05:13]

Thanks for the review Charlie! I fixed the Nits in the latest CL.

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1729: // If we're in a
middle of a reload. The FrameLoadType is propagated to
On 2017/05/10 05:56:40, Charlie Reis wrote:
> nit: Drop "If"

Done.

https://codereview.chromium.org/2863683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1730: // its children only
if it is a ReloadByPassingCache, else it becomes a
On 2017/05/10 05:56:40, Charlie Reis wrote:
> nit: Don't capitalize P.

Done.

[commit-bot: I haz the power, 2017-05-10 10:07:53]

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

[commit-bot: I haz the power, 2017-05-10 10:07:54]

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

[arthursonzogni, 2017-05-10 11:04:08]

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

[commit-bot: I haz the power, 2017-05-10 11:04:35]

Dry run: 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-05-10 12:30:45]

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

[commit-bot: I haz the power, 2017-05-10 12:30:46]

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

[Takashi Toyoshima, 2017-05-11 05:31:56]

This change itself lgtm.

Probably, we need to make CachePolicy determination logic more consistent among
two code paths.
Can you leave a TODO that says we need to factor out cache policy determination
logic from FrameFetchContext, and share the same logic between here and
FrameFetchContext.

[arthursonzogni, 2017-05-11 08:54:42]

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

[commit-bot: I haz the power, 2017-05-11 08:55:15]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-11 08:57:53]

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

[commit-bot: I haz the power, 2017-05-11 08:58:17]

Dry run: CQ is trying da patch.

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

[arthursonzogni, 2017-05-11 09:01:21]

Thanks Takashi!

Please Nate, I still need an LGTM from a blink owner.

[commit-bot: I haz the power, 2017-05-11 12:00:35]

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

[commit-bot: I haz the power, 2017-05-11 12:00:36]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-05-11 18:48:27]

lgtm

[nasko, 2017-05-11 18:48:42]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-11 18:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
toyoshim@chromium.org
Link to the patchset: https://codereview.chromium.org/2863683003/#ps100001
(title: "Add TODO")

[commit-bot: I haz the power, 2017-05-11 18:49:43]

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-05-11 19:06:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494528522992630,
"parent_rev": "3d449e8c4d349c0e4540a1d804920190ca76377c", "commit_rev":
"e7a445657ac1fa61217eb9097cd83a5a009a0a50"}

[commit-bot: I haz the power, 2017-05-11 19:07:06]

Description was changed from

==========
PlzNavigate: Fix wrong "Cache-Control" header.

This CL modify the FrameLoadType of frames such that the
ReloadBypassingCache load type gets propagated to every subframe during
a reload.

It fixes ReloadCacheControlBrowserTest.BypassingReload test with
PlzNavigate. Every ReloadCacheControlbroserTest are passing now.

BUG=671545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix wrong "Cache-Control" header.

This CL modify the FrameLoadType of frames such that the
ReloadBypassingCache load type gets propagated to every subframe during
a reload.

It fixes ReloadCacheControlBrowserTest.BypassingReload test with
PlzNavigate. Every ReloadCacheControlbroserTest are passing now.

BUG=671545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

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

[commit-bot: I haz the power, 2017-05-11 19:07:10]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e7a445657ac1fa61217eb9097cd8...

[kinuko, 2017-05-12 03:36:36]

On 2017/05/11 05:31:56, Takashi Toyoshima wrote:
> This change itself lgtm.
> 
> Probably, we need to make CachePolicy determination logic more consistent
among
> two code paths.
> Can you leave a TODO that says we need to factor out cache policy
determination
> logic from FrameFetchContext, and share the same logic between here and
> FrameFetchContext.

Hm, it's interesting fact that we won't be using this code after PlzNavigate
launch, and actually lots of main-frame specific code could go away... (just
thinking out loud, and wanted to note it for myself)