[webkit-reviews] review denied: [Bug 79269] Separate PageCachePolicy from PageCache action : [Attachment 129318] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Gavin Peters
<gavinp at chromium.org>'s request for review:
Bug 79269: Separate PageCachePolicy from PageCache action
https://bugs.webkit.org/show_bug.cgi?id=79269

Attachment 129318: Patch
https://bugs.webkit.org/attachment.cgi?id=129318&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129318&action=review


> Source/WebCore/history/PageCachePolicy.cpp:125
> +    framedata.responseDenies =
frame->loader()->documentLoader()->response().cacheControlContainsNoCache()

.responseDenies doesn't really fit as a name.

> Source/WebCore/history/PageCachePolicy.cpp:140
> +    return canCacheFrameImpl(framedata);

It seems entirely unnecessary to break up the gathering of all this data from
the decision-making based on the data gathered.  The current code is "one big
if-statement" that returns early to avoid lots of extra work, and this approach
regresses that.

> Source/WebCore/history/PageCachePolicy.cpp:186
> +bool PageCachePolicy::canCacheFrameImpl(const FrameData& framedata)
> +{
> +    if (!framedata.hasDocumentLoader) {
> +	   policyLog("	 -There is no DocumentLoader object");
> +	   return false;
> +    }

These changes to split out the data gathering then intertwine the debug-only
logging code and the actual release-build logic accomplishes 3 things:
1 - Makes it harder for the debug logging to get out of sync with the actual
logic.	This is good.
2 - Makes it so release builds can't short circuit during all of that data
gathering when the first disqualifying condition is reached.  This is bad.
3 - Makes the code harder to follow.  This is ugly.

I strongly urge you to reconsider your approach.