[webkit-reviews] review granted: [Bug 204305] Speculative loading sometimes happens too early and is missing login cookies : [Attachment 383784] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 204305: Speculative loading sometimes happens too early and is missing
login cookies
https://bugs.webkit.org/show_bug.cgi?id=204305

Attachment 383784: Patch

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




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 383784
  --> https://bugs.webkit.org/attachment.cgi?id=383784
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383784&action=review

> Source/WebKit/NetworkProcess/cache/NetworkCache.h:143
> +    void retrieve(const WebCore::ResourceRequest&, const GlobalFrameID&,
bool isTopMainResourceLoad, RetrieveCompletionHandler&&);

This information seems to be already available in ResourceRequest as oddly
named isTopSite() bit. Can we avoid duplicating information?

Also the parameter should be an enum.

> Source/WebKit/NetworkProcess/cache/NetworkCache.h:148
> +    void registerMainResourceLoadResponse(const WebCore::ResourceRequest&,
const WebCore::ResourceResponse&, const GlobalFrameID&);

This should probably be in #if ENABLE(NETWORK_CACHE_SPECULATIVE_REVALIDATION)
section. 

This sort of stuff points to speculative load being implemented in a wrong
layer. Cache API tries not to be a collection of random functions, but things
that actually make sense for a class called Cache. This doesn't really belong.