Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2167623002:
Reload: propagate FrameLoadType to child frames
|
Created:
4 years, 5 months ago by Takashi Toyoshima Modified:
4 years, 4 months ago CC:
chromium-reviews, Yoav Weiss, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, kinuko+watch, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReload: propagate FrameLoadType to child frames
Blink propagates FrameLoadType to child frames only if the type is for
history navigations. As a result, when users triger a reload,
sub-resources initiated in child frames are handled as a normal load.
To reload sub-resources in child frames correctly, FrameLoadType should
be propagated if the type is for reloads too.
BUG=332602
Patch Set 1 #Patch Set 2 : add some comments #
Total comments: 4
Patch Set 3 : remove non-essential changes #Messages
Total messages: 14 (5 generated)
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reload: propagate FrameLoadType to child frames Blink propagates FrameLoadType to child frames only if the type is for history navigations. As a result, when users triger a reload, sub-resources initiated in child frames are handled as a normal load. To reload sub-resources in child frames correctly, FrameLoadType should be propagated if the type is for reloads too. BUG=332602 ========== to ========== Reload: propagate FrameLoadType to child frames Blink propagates FrameLoadType to child frames only if the type is for history navigations. As a result, when users triger a reload, sub-resources initiated in child frames are handled as a normal load. To reload sub-resources in child frames correctly, FrameLoadType should be propagated if the type is for reloads too. BUG=332602 ==========
toyoshim@chromium.org changed reviewers: + clamy@chromium.org, hiroshige@chromium.org
Let me ask non-owners review. hiroshige: Can you take a look in terms of memory cache? This does not touch core/fetch, but FrameFetchContext and others would affect memory cache revalidatoin. clamy: Does this change make sence in terms of PlzNavigate?
clamy@chromium.org changed reviewers: + creis@chromium.org
On 2016/07/20 10:36:05, toyoshim wrote: > Let me ask non-owners review. > > hiroshige: Can you take a look in terms of memory cache? This does not touch > core/fetch, but FrameFetchContext and others would affect memory cache > revalidatoin. > > clamy: Does this change make sence in terms of PlzNavigate? I think it works with PlzNavigate. +creis: does this change make sense with the subframe navigation entries?
On 2016/07/22 14:08:02, clamy wrote: > On 2016/07/20 10:36:05, toyoshim wrote: > > Let me ask non-owners review. > > > > hiroshige: Can you take a look in terms of memory cache? This does not touch > > core/fetch, but FrameFetchContext and others would affect memory cache > > revalidatoin. > > > > clamy: Does this change make sence in terms of PlzNavigate? > > I think it works with PlzNavigate. > > +creis: does this change make sense with the subframe navigation entries? Thanks for checking. I do see a problem with UseSubframeNavigationEntries, which PlzNavigate depends on. More discussion below. https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1552: loadType = frame()->loader().loadType(); I don't think this will work for the new SiteIsolationPolicy::UseSubframeNavigationEntries() path, which we're about to turn on. In that path, RenderFrameImpl::historyItemForNewChildFrame() always returns null, and we punt the navigation to the browser process in decidePolicyForNavigation. The browser process looks to see if there's a history item for the frame and then asks an appropriate process (not necessarily this one) to load it. If it found a history item, RenderFrameImpl::NavigateInternal sets the load type based on request_params.is_history_navigation_in_new_child. As a side note, this seems like a possibly significant change to how we behaved before, even in the default navigation path. The try jobs on patch set 1 indicated some layout test failures as well, so I'm concerned about what impact it has. Can you investigate those, and add a test that covers the bug you're fixing?
https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:223: // intentional. Not blocking this CL, but do we have any documents/crbug/code locations that explains the intention? https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:264: // is intentional. ditto.
https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2167623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:223: // intentional. How about public/web/WebFrameLoadType.h? core/loader/FrameLoaderTypes.h points this header in the comment, and it explains ReloadMainResource is equivalent to Standard for sub-resources. I can add comment to point these headers, and probably it would be better to split these fixes to separate CL? Changes in this file is not directly related to the bug, but just cleanup sort of things.
To focus on the point, I split unessential changes to https://codereview.chromium.org/2181093004/ and https://codereview.chromium.org/2186863002/
I still could not figure out which code make such difference in each frame's history. Charlie: Do you know how each frame history is updated on the default navigation path?
On 2016/08/08 11:49:53, toyoshim wrote: > I still could not figure out which code make such difference in each frame's > history. > Charlie: Do you know how each frame history is updated on the default navigation > path? If it helps, I've just turned off the old default navigation path in https://crrev.com/410150. There's a few regressions we're tracking and trying to fix, but so far I'm hoping we won't need to revert back to the old path. That would give us one less case to worry about here. (In general, the old path used to use HistoryEntryToPageState to convert a whole tree of HistoryItems into a single PageState, encapsulating the state of the frame tree. We did that at both commit time and when sending UpdateState IPCs.) |
