[Webkit-unassigned] [Bug 153230] Network cache: old pages returned by disk cache on history navigation after session is restored

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153230

--- Comment #15 from Chris Dumez <cdumez at apple.com> ---
(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 270655 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270655&action=review
> > 
> > What I would suggest would be:
> > 1. Revert http://trac.webkit.org/changeset/181815 so that our API has a
> > "allowStaleContent" parameter again (which I find clearer that the approach
> > in this patch)
> 
> The problem of that approach is that API needs to decide, but we don't know
> if the BF navigation we are doing is for an item restored from the session
> or not. Also, changing the frame load type not only affects the disk cache,
> it would mena that the page cache wouldn't be used no? If an item is
> restored from the session, but it's in the page or memory cache, we want to
> use that, not to go to the disk cache or network.

Are you trying to get a different behavior from the one before <http://trac.webkit.org/changeset/181815> then? I thought the idea was to restore that behavior.

The page cache only matters on actual BackForward navigation. In this case, we are talking about changing the load type only in the case of session restoration. Therefore, I don't think we should worry about the PageCache (or memory cache for that matter). In any case, your issue seems to be with the behavior change introduced in r181815 so I would suggest going back to that behavior on non-iOS platforms before attempting further behavior changes.

> 
> > 2. Add a setting to decide in restoreSession() if we want to allow stale
> > content or not.
> > 3. Set this setting to True on iOS and False elsewhere.
> > 
> > > Source/WebCore/history/HistoryItem.h:206
> > > +    void setShouldRevalidateInDiskCacheOnly(bool revalidateInDiskCache) { m_shouldRevalidateInDiskCacheOnly = revalidateInDiskCache; }
> > 
> > I think the naming here is not very clear. Also, I am not sure why we need
> > to add a data member to HistoryItem.
> 
> To know from the FrameLoader if the item we are going to load wants to use
> the disk cache unconditionally or not. We need to flag the item to know what
> to do when it's loaded, if you go to current item right after restoring the
> session, you know you don't want stale data from the disk cache, but if that
> session contained more BF items, and then you go back, you don't want stale
> data from the disk cache in that case either. If we flag the items when they
> are created in the web process, we have that info when they are loaded.
> 
> > > Source/WebKit2/WebProcess/WebCoreSupport/SessionStateConversion.h:41
> > > +    APIRequest
> > 
> > I personally don't find this to be clear.
> 
> We can find better names.
> 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2187
> > > +    restoreSessionInternal(itemStates, SessionRestoreMode::APIRequest);
> > 
> > I think allowing stale content on session restore should be based on a
> > setting. I know that we want to keep allowing stale content on iOS for
> > example.
> 
> Ok, I can add a setting then, but if it's not something you want to change
> via API ans we are going to use the same values always for every port, maybe
> adding another setting is not the best approach. We can just have a method
> to query the behavior and add an implementation on every port.

I don't know what's the best policy here. Maybe Sam can pitch in.
No matter how, I'd like to maintain the current behavior for iOS.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160204/7e949401/attachment-0001.html>