Ping loads should not prevent page caching.
<rdar://problem/53901632>
Created attachment 375486 [details] Patch
Comment on attachment 375486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375486&action=review Two suggestions for refinement later; neither should block landing this exactly as is. > Source/WebCore/history/PageCache.cpp:466 > + // Stop all loads again before checking if we can still cache the page after firing the pagehide > + // event, since the page may have started ping loads in its pagehide event handler. > + for (Frame* frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) { > + if (auto* documentLoader = frame->loader().documentLoader()) > + documentLoader->stopLoading(); > + } Since this says "again" I assume there’s another copy of this loop somewhere. Could we share it rather than writing it out twice? > Source/WebCore/loader/DocumentLoader.cpp:128 > +static bool shouldPendingCachedResourceLoadPreventPageCache(CachedResource& cachedResource) I think the local could be named just "resource" since its unambiguous in the context of this sort function.
Comment on attachment 375486 [details] Patch Clearing flags on attachment: 375486 Committed r248265: <https://trac.webkit.org/changeset/248265>
All reviewed patches have been landed. Closing bug.
Comment on attachment 375486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375486&action=review > Source/WebCore/loader/DocumentLoader.cpp:139 > + return false; Similarly to prefetch, preloads should probably not prevent entering page cache. Probably not a huge risk in practice but this would be nice to cover that case as well. Something like (resource.isLinkPreload() && resource.preloadResult() == CachedResource::PreloadResult::PreloadNotReferenced) might do the trick.