Simplify code for making Page-cacheability decision by merging logging code and decision making code.
Created attachment 258624 [details] Patch
Comment on attachment 258624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258624&action=review > Source/WebCore/history/PageCache.cpp:88 > -static unsigned logCanCacheFrameDecision(Frame& frame, DiagnosticLoggingClient& diagnosticLoggingClient, unsigned indentLevel) > +static bool canCacheFrame(Frame& frame, DiagnosticLoggingClient& diagnosticLoggingClient, unsigned indentLevel) > { > PCLOG("+---"); > - if (!frame.isMainFrame() && frame.loader().state() == FrameStateProvisional) { > + FrameLoader& frameLoader = frame.loader(); > + > + // Prevent page caching if a subframe is still in provisional load stage. > + // We only do this check for subframes because the main frame is reused when navigating to a new page. > + if (!frame.isMainFrame() && frameLoader.state() == FrameStateProvisional) { > PCLOG(" -Frame is in provisional load stage"); > logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::provisionalLoadKey()); > - return 1 << IsInProvisionalLoadStage; > + return false; > } It might be cleaner to separate the decision-making similar to what is done in network cache code (makeRetrieveDecision()/makeStoreDecision()/makeUseDecision()) and have a separate logging function that takes the decision as argument. What do you think?
Comment on attachment 258624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258624&action=review >> Source/WebCore/history/PageCache.cpp:88 >> } > > It might be cleaner to separate the decision-making similar to what is done in network cache code (makeRetrieveDecision()/makeStoreDecision()/makeUseDecision()) and have a separate logging function that takes the decision as argument. What do you think? Hmm, unlike the disk cache decision making code, we have a lot more reasons for failing and we want to log all the reasons for failing. Also, the PCLog() part of the logging is structured so you virtual see the frame hierarchy, if the frame is failing or not and why. For these reasons, I feel doing to logging separately from the decision making is going to make the code make complicated.
Comment on attachment 258624 [details] Patch Ok, though I bet if you implemented it would look better.
Comment on attachment 258624 [details] Patch Clearing flags on attachment: 258624 Committed r188233: <http://trac.webkit.org/changeset/188233>
All reviewed patches have been landed. Closing bug.
Comment on attachment 258624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258624&action=review > Source/WebCore/history/PageCache.cpp:-298 > -bool PageCache::canCachePageContainingThisFrame(Frame& frame) The prototype of this function was not deleted from the header file in this patch. Also the ChangLog did not mention it was deleted. Maybe it is a bug in prepare-ChangeLog.