[Webkit-unassigned] [Bug 49838] Image Subresource Loads Should Not Prevent Page Cache Entry

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49838





--- Comment #7 from Joseph Pecoraro <joepeck at webkit.org>  2010-11-29 15:16:35 PST ---
(From update of attachment 74432)
View in context: https://bugs.webkit.org/attachment.cgi?id=74432&action=review

>> WebCore/history/PageCache.cpp:259
>> +        && (documentLoader->mainDocumentError().isNull() || (documentLoader->mainDocumentError().isCancellation() && documentLoader->subresourceLoadersArePageCacheAcceptable()))
> 
> What tells us that cancellation is specifically due to the subresource issue? It seems like any cancellation would return true here, not just the one you mention in the comment. Are cancellations solely due to leaving the page before subresources finish loading, or are there other causes of cancellation?

subresourceLoadersArePageCacheAcceptable only accesses a boolean set when there was a cancellation through stopLoading, and is later checked if the page can enter the page cache. I briefly looked around for other causes of cancellations, but I didn't make any conclusions. I guess another possibility would be the user explicitly stopping a load. In that case, should back/forward cause the user to go back to the partial load or not? However that only seems possible in a situation where the main resource hasn't completed loading yet, and this case I tried to only deal with subresource loads after the page has completed loading. I'll investigate this some more.

>> WebCore/loader/DocumentLoader.cpp:79
>> +    const ResourceLoaderSet copy = loaders;
> 
> We normally don’t use const for local variables like this one.
> 
> Rather than copying the set it would be more efficient to instead use a local vector and the copyToVector function.

Sounds good. This copied the form of the two static functions above it (cancelAll and setAllDefersLoading). I'll change those as well.

>> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:389
>> +        ResourceRequestBase::TargetType type = request.targetType();
> 
> This can be ResourceRequest::TargetType. The use of ResourceRequestBase is an internal implementation detail that we normally should ignore.

I'll look for an alternative to targetType. As Alexey mentioned offline, the choice of "image loads only" seems arbitrary.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.