[Webkit-unassigned] [Bug 30246] Factor HistoryController out of FrameLoader

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30246





--- Comment #11 from Adam Barth <abarth at webkit.org>  2009-10-09 09:57:37 PDT ---
(In reply to comment #3)
> When does the Frame -> FrameLoader connection break?

As far as I know, this connection never breaks.  FrameLoader is just a
component of Frame that's broken into it's own object.

> What about FrameLoader -> DocumentLoader?  Could those broken connections cause
> new NULL pointer derefs with this change?

If these cases exist, then they would already be bugs.  I haven't changed
anything here.

> 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());

We should handle this in a separate bug.

> 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...

We can do this, but I'd prefer to it in another patch.

> 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.

We can do that, but then it will ask for m_loader->frame()->page(), which seems
like the same thing but backwards.

> Yeah, it's a little strange that HistoryController has an m_frame pointer, but
> that Frame does not have a HistoryController pointer:

I considered putting the HistoryController on Frame, but it's used almost
exclusively by the FrameLoader.  It seems like more a part of FrameLoader than
of Frame, but these things are hard to tell because the objects all have the
same lifetime.

> 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.

We can paper over this by changing the names of the accessors, but the code is
going to do the same thing.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.