[webkit-reviews] review granted: [Bug 30246] Factor HistoryController out of FrameLoader : [Attachment 40936] Patch v1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 30246: Factor HistoryController out of FrameLoader
https://bugs.webkit.org/show_bug.cgi?id=30246

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
3797	  if (documentLoader()->urlForHistory().isEmpty())
 3833	  if (m_frame->loader()->documentLoader()->urlForHistory().isEmpty())

I still kinda think m_current and m_previous, etc, should be m_currentItem and
m_previousItem, but they're also OK as is.  Likewise current() to
currentItem().	This could be done in a separate change if you agree.

When does the Frame -> FrameLoader connection break?
What about FrameLoader -> DocumentLoader?  Could those broken connections cause
new NULL pointer derefs with this change?

We need a LOG macro that knows how to handle %S for String or something:
LOG(PageCache, "Not restoring page for %s from back/forward cache because cache
entry has expired", history()->provisional()->url().string().ascii().data());

Some places, like in:
4253 void HistoryController::updateForStandardLoad()

grab m_frame->loader() a zillion times.  Seems that HistoryController should
have a frameLoader() method do that that?  And maybe a documentLoader() method?
frameLoaderClient() too?  Or in updateForStandardLoad we could use a
frameLoader local variable...

HistoryController still grabs at FrameLoader a lot.  I wonder if it makes more
sense for it to have an m_frameLoader instead of an m_frame.  Certainly if
m_frame ever lost its loader() pointer HistoryController would be *very*
unhappy.

Yeah, it's a little strange that HistoryController has an m_frame pointer, but
that Frame does not have a HistoryController pointer:
4373		
parentFrame->loader()->history()->m_current->setChildItem(createHistoryItem(tru
e));
Seems like a strange love triangle going on here.  Someone's gonna get hurt.

OK.  As far as I can tell you have not caused any harm with this change. :) 
This sort of split is HUGELY HELPFUL.  I'm not sure the architecture is perfect
yet, but I know you're workign on this more and I'm happy to accept this as a
first-step.  I think my primary concern with this patch is the History ->
m_frame, Frame -> FrameLoader, FrameLoader -> History triangle.  Seems like
History wants to be either tied to FrameLoader or Frame, but not in a triangle
like this.

r=me as is, but please consider the above comments for this and future work on
this new class.