[webkit-reviews] review granted: [Bug 39382] Factor PageCache code out of FrameLoader into a PageCacheController : [Attachment 56643] Hopefully with all the necessary #includes this time

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 39382: Factor PageCache code out of FrameLoader into a PageCacheController
https://bugs.webkit.org/show_bug.cgi?id=39382

Attachment 56643: Hopefully with all the necessary #includes this time
https://bugs.webkit.org/attachment.cgi?id=56643&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This looks good, although it's a bit too complicated for me to say 100% that
it's correct.  Some minor comments below.

WebCore/history/PageCache.cpp:53
 +  #ifndef NDEBUG
I think we want a space after this line.

WebCore/history/PageCache.cpp:220
 +  #endif 
Similarly, I think we want a space before this line.

WebCore/history/PageCache.cpp:354
 +	if (RefPtr<CachedPage> cachedPage = item->m_cachedPage.get()) {
Why is this a RefPtr?  If we need this reference, then we'll get in trouble on
line 359 below where we return a raw pointer.  Oh, I see.  In the case where we
return the raw pointer, we don't remove the CachedPage, but in the case where
we do, we'll end up with the last reference and delete the CachedPage when we
return.  Boo.  I'm hoping to see these as minus lines later in the patch.

WebCore/loader/FrameLoader.cpp:2157
 +	Page* page = m_frame->page();
I'm not sure it's worth storing this in a temporary, but it probably doesn't
matter.

WebCore/loader/FrameLoader.cpp:2181
 +	    cachedPage->restore(m_frame->page());
We have a |page| temporary we could use here.

WebCore/loader/FrameLoader.cpp: 
 +		return;
I see.	So the trickiness with RefPtr is new...  Might be worth adding a
comment explaining what's going one.  PageCache isn't very well tested by
DumpRenderTree, so we need to be careful when changing this code.  Have you
looked in manual-tests to see if there are any good page cache tests you can
run manually?