[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 #47 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #46)
> Comment on attachment 272315 [details]
> Go back to initial approach
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272315&action=review
> 
> r=me with changes.

Thank you very much!

> > Source/WebCore/history/HistoryItem.h:262
> > +    // Item was created from a session restore.
> 
> I don't think this comment is useful.

Ok.

> > Source/WebCore/history/HistoryItem.h:263
> > +    bool m_restoredFromSession { false };
> 
> we need a prefix for boolean members, I suggest m_wasRestoredFromSession.

Ok.

> Also, could you please move this new member next to the other boolean
> members above?

Sure.

> > Source/WebCore/loader/FrameLoader.cpp:3340
> > +            if (!item.wasRestoredFromSession())
> 
> I would do the #ifdef iOS logic here.

Ok.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2234
> > +void WebPage::restoreSessionInternal(const Vector<BackForwardListItemState>& itemStates, bool restoredByAPIRequest)
> 
> Do we really need a new method? Maybe we could just add the extra parameter
> to restoreSession() ?

Yes, because it's a message handler called from generated code.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2238
> > +#if !PLATFORM(IOS)
> 
> I would prefer if we moved the IOS ifdefing to
> FrameLoader::loadDifferentDocumentItem() and set the wasRestoredFromSession
> flag here no matter the platform. Otherwise, we end up with a boolean that
> has an inaccurate value on iOS.

Makes sense.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2239
> > +        // iOS always wants stale data after session restore.
> 
> s/wants/allows

Ok.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2248
> > +    restoreSessionInternal(itemStates, true);
> 
> Could you please make the boolean argument an enum for readability? e.g.
> enum class WasRestoredByAPIRequest { No, Yes };

Sure, my original patch did that indeed, but I was not happy with the names I used.

-- 
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/20160226/b5b5b3a7/attachment.html>