Bug 200418 – Ping loads should not prevent page caching

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Ping loads should not prevent page caching.

[Radar WebKit Bug Importer]

<rdar://problem/53901632>

[Chris Dumez]

Created attachment 375486 [details]
Patch

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 375486 [details]
Patch

Clearing flags on attachment: 375486

Committed r248265: <https://trac.webkit.org/changeset/248265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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.