Bug 43704

Created attachment 63863 [details] Patch

Ping?

I think that solution to these problems is in CSS code, not in loader code. But since I don't know CSS code much, I may well be wrong.

Comment on attachment 63863 [details] Patch Yeah, I don't think the loader should know whether we're in the middle of printing. That seems wrong.

Created attachment 67163 [details] Patch 2

Hi, Alexey, Adam, Thank you for the reviews. Can you take another look? I've written a new patch based on your comments. (In reply to comment #4) > (From update of attachment 63863 [details]) > Yeah, I don't think the loader should know whether we're in the middle of printing. That seems wrong.

Ping?

Ping again?

What is the difference between the new bit and m_allowStaleResources?

Comment on attachment 67163 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=67163&action=review > WebKit/mac/WebView/WebHTMLView.mm:3886 > + document->cachedResourceLoader()->validateCachedDocumentResources(false); We may want to make a RAII class to do this false/true setting automatically. Just makes it impossible to not pair them correctly and get into a bad state.

Created attachment 75661 [details] Use RAII

Thank you for the review. Introduced an RAII class.

The question from comment 9 still stands.

Created attachment 75770 [details] Renamed the cache validation suppression bit

Attachment 75770 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'HEAD'. If any of these errors are false positives, please file a bug against check-webkit-style.

Sorry for not responding sooner. m_allowStaleResources suppresses cache validation entirely, i.e., across documents. m_allowStaleResourcesWithinDocument (renamed from m_validateCachedDocumentResources) suppresses cache validation within the document. If a web font has been downloaded for a document for continuous media (screen), it should not be cache-validated and reloaded for paged media (print). But if the web font has been downloaded for another document, cache validation should happen. Example 1: - MyDocument uses FontW for both continuous and paged media. In printing MyDocument, FontW cached for continuous media should be used without validating the cache. Example 2: - MyDocument uses FontC for continuous media and FontP, for paged. - AnotherDocument uses FontP. In printing MyDocument, FontP cached for AnotherDocument must not be used without first validating the cache. (Cache validation won't happen if we only use m_allowStaleResources.) (In reply to comment #13) > The question from comment 9 still stands.

Thank you. CC'ing Antti, who is likely the best reviewer for this patch. I can't say that I'm fully convinced that this is a loader issue, but you may be right in treating is as such. Technically, it's not great to have two booleans with partially overlapping meaning - an enum with three values could be a better fit.

Ping?

Comment on attachment 75770 [details] Renamed the cache validation suppression bit Why does it try to reload resources when switching to printing? Why can't you use the existing m_allowStaleResources? A separate m_allowStaleResourcesWithinDocument doesn't make that much sense as CachedResourceLoader is per-document anyway. You need to make a new patch, the code you are modifying has changed substantially, r- for now.

Created attachment 78091 [details] Use only m_allowStaleResources

Antti, Thank you for the review. (In reply to comment #19) > (From update of attachment 75770 [details]) > Why does it try to reload resources when switching to printing? It is because style sheets are evaluated again in switching to printing. If web fonts are used in the sheets, they are reloaded. > > Why can't you use the existing m_allowStaleResources? A separate m_allowStaleResourcesWithinDocument doesn't make that much sense as CachedResourceLoader is per-document anyway. I misunderstood the behavior. Changed to use m_allowStaleResources. > > You need to make a new patch, the code you are modifying has changed substantially, r- for now. Updated the patch. Can you take another look?

> Why does it try to reload resources when switching to printing? That's the same question that I had; see also bug 27971. But I don't know if this can actually be fixed in CSS code.

Are you sure this bug actually occurs in current TOT? In principle this test in CachedResourceLoader::determineRevalidationPolicy() should prevent reloading: // Avoid loading the same resource multiple times for a single document, even if the cache policies would tell us to. if (m_validatedURLs.contains(existingResource->url())) return Use;

If it doesn't it would be interesting to see why.

Hi, Antti, You are right. r74807 seems to have fixed the loading behavior and the printing issue is now gone. I marked my patch as obsolete and close this bug. (In reply to comment #24) > If it doesn't it would be interesting to see why.

(In reply to comment #25) > Hi, Antti, > > You are right. > r74807 seems to have fixed the loading behavior and the printing issue is now gone. > > I marked my patch as obsolete and close this bug. > > (In reply to comment #24) > > If it doesn't it would be interesting to see why. Actually we should be reloading the resource in case of Cache-control: no-store (but are not due to bug 52044) in which case your patch is still needed. The patch looks good to me. It would be better if your RAII type would save and restore the existing state so it could be safely used in nested context.

Created attachment 78779 [details] Patch

Hi, Antti, Changed to save and use previous state. Can you take another look? (In reply to comment #26) > (In reply to comment #25) > > Hi, Antti, > > > > You are right. > > r74807 seems to have fixed the loading behavior and the printing issue is now gone. > > > > I marked my patch as obsolete and close this bug. > > > > (In reply to comment #24) > > > If it doesn't it would be interesting to see why. > > Actually we should be reloading the resource in case of Cache-control: no-store (but are not due to bug 52044) in which case your patch is still needed. > > The patch looks good to me. It would be better if your RAII type would save and restore the existing state so it could be safely used in nested context.

Comment on attachment 78779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78779&action=review I'm sad about this patch not having tests, but it seems like the consensus is that we should accept this patch. > Source/WebCore/loader/cache/CachedResourceLoader.h:146 > + ResourceCacheValidationSuppressor(CachedResourceLoader* loader) : m_loader(loader), m_previousState(false) The initializers should be on their own lines. > Source/WebCore/loader/cache/CachedResourceLoader.h:148 > + if (m_loader) { Shouldn't we ASSERT that m_loader is non-NULL? What situation would we get into where we wanted to use a null loader? > Source/WebCore/page/Frame.cpp:506 > + // In setting printing, we should not validate resources already cached for the document. > + // See https://bugs.webkit.org/show_bug.cgi?id=43704 Having a test would be better than having this comment.

Created attachment 92756 [details] Update and clean up

The commit-queue encountered the following flaky tests while processing attachment 92756 [details]: http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com) fast/dom/onerror-img.html bug 51019 The commit-queue is continuing to process your patch.

Comment on attachment 92756 [details] Update and clean up Rejecting attachment 92756 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1 Last 500 characters of output: =43704&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Updating working directory Processing patch 92756 from bug 43704. NOBODY (OOPS!) found in /Projects/CommitQueue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Projects/CommitQueue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Updating OpenSource Current branch new_master is up to date. Full output: http://queues.webkit.org/results/8640426

Adam, can you take another look?

Comment on attachment 92756 [details] Update and clean up r=me

Comment on attachment 92756 [details] Update and clean up Clearing flags on attachment: 92756 Committed r86601: <http://trac.webkit.org/changeset/86601>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 92756 [details]: http/tests/appcache/origin-usage.html bug 60928 The commit-queue is continuing to process your patch.