[webkit-reviews] review requested: [Bug 49838] Image Subresource Loads Should Not Prevent Page Cache Entry : [Attachment 75599] [PATCH] Addressed Some Comments

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has asked	for review:
Bug 49838: Image Subresource Loads Should Not Prevent Page Cache Entry
https://bugs.webkit.org/show_bug.cgi?id=49838

Attachment 75599: [PATCH] Addressed Some Comments
https://bugs.webkit.org/attachment.cgi?id=75599&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
 • Addressed Darin's style review comments.
 • To not use TargetType I look up the CacheResource and check if it is a
CacheImage (isImage)
    - this means I have to move my code a little earlier in stopLoading because
the resource could be evicted

> 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?

I looked at a lot of code that could cause cancellations. Lots of the code
doesn't deal with history
navigation at all, much of it deals with provisional document loaders. It is a
bit unfortunate in
those cases that this makes a loop through the resource loaders, but it breaks
early. I haven't
found a better place for this code either, which is why it remains in
DocumentLoader::stopLoading.

As far as I can tell, its okay for this code to run on any cancellation, even
the main document via
the user canceling the load. This could just be my opinion, but if I stopped a
page from loading
and it had a missing image, I still wouldn't mind if I clicked a link and came
back to the page
and it had the same missing image. But maybe this kind of behavior should be
opt-in.