[webkit-reviews] review granted: [Bug 200418] Ping loads should not prevent page caching : [Attachment 375486] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 200418: Ping loads should not prevent page caching
https://bugs.webkit.org/show_bug.cgi?id=200418

Attachment 375486: Patch

https://bugs.webkit.org/attachment.cgi?id=375486&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 375486
  --> https://bugs.webkit.org/attachment.cgi?id=375486
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.