Bug 147829 – Simplify code for making Page-cacheability decision

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Chris Dumez]

Simplify code for making Page-cacheability decision by merging logging code and decision making code.

[Chris Dumez]

Created attachment 258624 [details]
Patch

[Antti Koivisto]

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?

[Chris Dumez]

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.

[Antti Koivisto]

Comment on attachment 258624 [details]
Patch

Ok, though I bet if you implemented it would look better.

[WebKit Commit Bot]

Comment on attachment 258624 [details]
Patch

Clearing flags on attachment: 258624

Committed r188233: <http://trac.webkit.org/changeset/188233>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

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.