Don't revalidate all resources for FrameLoadTypeSame

Don't revalidate all resources for FrameLoadTypeSame

The main resource of the top-loading frame probably need to be revalidated, but the rest of the page can probably prefer the cache.

Also, simplify setting of load type FrameLoadTypeSame.

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

[Nate Chapin, 2015-06-29 21:30:53]

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

[dcheng, 2015-07-01 20:54:12]

https://codereview.chromium.org/1213913003/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1213913003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:737: return
FrameLoadTypeRedirectWithLockedBackForwardList;
I don't think I completely understand this line.

1) Can we add a comment about what
FrameLoadTypeRedirectWithLockedBackForwardList is? Or what a locked back/forward
list is?

2) What does this specific case need to map to redirect with locked back/forward
list?

https://codereview.chromium.org/1213913003/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1195: && (request.originDocument() ||
loadType != FrameLoadTypeRedirectWithLockedBackForwardList || url !=
m_documentLoader->urlForHistory())
I don't think I understand this either. I guess this is sort of covered under
the "explicitly reloading" bits in the comment at line 1190, but I'm not really
sure either. Does that comment need to be updated?

[Nate Chapin, 2015-07-15 18:24:03]

Ok, I think this is ready for another review, now that it does something
completely different from last time :)

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContext.cpp:170: for (Frame* f = frame(); f; f =
f->tree().parent()) {
This function is no longer recursive. Everything above this line only applies to
the current frame (or the request). If none of those early returns trigger, we
walk up the frame tree (starting with the current frame) looking for an ancestor
that has a load type that modifies cache policy.

[dcheng, 2015-07-16 06:55:47]

lgtm

is this covered by any tests? =)

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContext.cpp:170: for (Frame* f = frame(); f; f =
f->tree().parent()) {
On 2015/07/15 at 18:24:03, Nate Chapin wrote:
> This function is no longer recursive. Everything above this line only applies
to the current frame (or the request). If none of those early returns trigger,
we walk up the frame tree (starting with the current frame) looking for an
ancestor that has a load type that modifies cache policy.

Incidentally, is this OK to potentially skip ancestors in the walk up the tree?

[Nate Chapin, 2015-07-16 18:05:41]

ResourceRequestCachePolicy is not well tested at the moment. We have some tests
to verify that we correctly set FrameLoadType. I guess we could unit test the
logic in FrameFetchContext directly if so desired.

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1213913003/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContext.cpp:170: for (Frame* f = frame(); f; f =
f->tree().parent()) {
On 2015/07/16 06:55:47, dcheng wrote:
> On 2015/07/15 at 18:24:03, Nate Chapin wrote:
> > This function is no longer recursive. Everything above this line only
applies
> to the current frame (or the request). If none of those early returns trigger,
> we walk up the frame tree (starting with the current frame) looking for an
> ancestor that has a load type that modifies cache policy.
> 
> Incidentally, is this OK to potentially skip ancestors in the walk up the
tree?

In general, either (1) an iframe load is a navigation unto itself, and its
parent will be FrameLoadTypeStandard (i.e., the "i'm not loading right now"
state), or (2) The FrameLoadType will be the same all the way down. So I think
this is OK.

[dcheng, 2015-07-17 18:44:05]

lgtm

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:326: class
MockFrameLoaderClientWithParent : public EmptyFrameLoaderClient {
Nit: these are probably more of a Fake or a Stub than a Mock.

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:328:
MockFrameLoaderClientWithParent(Frame* parent)
explicit

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:352: virtual void SetUp()
override instead of virtual here

[Nate Chapin, 2015-07-17 18:54:07]

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:326: class
MockFrameLoaderClientWithParent : public EmptyFrameLoaderClient {
On 2015/07/17 18:44:05, dcheng wrote:
> Nit: these are probably more of a Fake or a Stub than a Mock.

I can never remember which is which :( Stubbed.

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:328:
MockFrameLoaderClientWithParent(Frame* parent)
On 2015/07/17 18:44:05, dcheng wrote:
> explicit

Done.

https://codereview.chromium.org/1213913003/diff/120001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:352: virtual void SetUp()
On 2015/07/17 18:44:05, dcheng wrote:
> override instead of virtual here

Done.

[Nate Chapin, 2015-07-17 18:54:14]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-07-17 18:54:14]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1213913003/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2015-07-17 18:55:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213913003/140001

[commit-bot: I haz the power, 2015-07-17 19:49:48]

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

[commit-bot: I haz the power, 2015-07-17 19:49:49]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[Nate Chapin, 2015-07-17 22:34:02]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-07-17 22:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1213913003/#ps160001
(title: "Fix oilpan + assert failures")

[commit-bot: I haz the power, 2015-07-17 22:34:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213913003/160001

[commit-bot: I haz the power, 2015-07-18 00:24:44]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199143