Bug 30246 – Factor HistoryController out of FrameLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Barth]

HistoryController is in charge of managing the current / previous / provisional HistoryItems.

[Adam Barth]

Created attachment 40936 [details]
Patch v1

[Adam Barth]

This patch is larger than I would have liked, but it wasn't clear to me how to do this in a smaller step.

[Eric Seidel (no email)]

Comment on attachment 40936 [details]
Patch v1

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

[Eric Seidel (no email)]

CCing loader folks so they see your patch and my comments.

[Darin Adler]

Comment on attachment 40936 [details]
Patch v1

> +void HistoryController::setCurrent(HistoryItem* item)
> +{
> +    m_current = item;
> +}

I applaud the aim of brevity, and leaving out things that are unnecessary or determined by context.

But even given that I don't think these function and data member names are clear enough when they contain adjectives like "current" and "provisional" but no nouns. I think you need a noun like "item" or "location", otherwise it's just too hard to read. Maybe we can come up with another creative way to make this brief. Is there a good single word for "current item"?

Many of the calls do include the phrase "current item", so I think just restoring the word "item" is the easiest way to do it.

I also don't think that "provisional" is really a history concept -- but I suppose there's no simple way to avoid that, though.

> +void HistoryController::clearPrevious()
> +{
> +    m_previous = 0;
> +}

This seems a bit too low-level to be a good abstraction. Maybe a different name would work better? Maybe this is something we can improve after the classes are split?

One of the problems with the name is that it does not "clear the previous item", it "clears the pointer to the previous item", which does not quite seem to be the same thing. That's a problem with using "clear" as a synonym for "set to zero".

I was going to set this to review-, but Eric already set it to review+

[Eric Seidel (no email)]

Comment on attachment 40936 [details]
Patch v1

You should feel welcome to set it to r-. :)  I kinda view any r- as trumping an r+. :)

[Darin Adler]

(In reply to comment #6)
> You should feel welcome to set it to r-. :)  I kinda view any r- as trumping an
> r+. :)

I trust Adam to read our feedback and do the right thing. I don't think the numeric sign of the review matters much in this case :-)

[Brady Eidson]

Comment on attachment 40936 [details]
Patch v1

I'm not going to be as forgiving as Eric (who r+'ed) or Darin (who noted his objections and intention to r-, but didn't change the flag)

I think the brevity is too much and hurts future readability.  Please address Darin's comments.  

Otherwise, looks good!

[Brady Eidson]

And of course the switch to r- happened while I was reviewing myself.  *sigh*

[Adam Barth]

Comment on attachment 40936 [details]
Patch v1

> Maybe we can come up with another creative way to make
> this brief. Is there a good single word for "current item"?

I'll change this to m_currentItem.

> + HistoryController::clearPrevious
>
> This seems a bit too low-level to be a good abstraction. Maybe a different name
> would work better? Maybe this is something we can improve after the classes are
> split?

Yeah, I'd like to remove the setCurrent / etc methods, but that required me to
tackle loadItem, which I wanted to defer.  I can change this to
"updateForFrameLoadCompleted" since it's only ever called at that time.

[Adam Barth]

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

[Adam Barth]

Created attachment 40952 [details]
HistoryController

[Darin Adler]

Comment on attachment 40952 [details]
HistoryController

> -PassRefPtr<HistoryItem> FrameLoader::createHistoryItem(bool useOriginal)
> +PassRefPtr<HistoryItem> HistoryController::createHistoryItem(bool useOriginal)

Elsewhere you removed the word "history" from the phrase "history item" since this is the history controller. I think you should do that here too. Except maybe there's ambiguity with back/forward item?

> -PassRefPtr<HistoryItem> FrameLoader::createHistoryItemTree(Frame* targetFrame, bool clipAtTarget)
> +PassRefPtr<HistoryItem> HistoryController::createHistoryItemTree(Frame* targetFrame, bool clipAtTarget)

Ditto.

You can consider in some cases including the entire class as a member of the owner class. The reason we don't do that in Frame itself to avoid header dependencies -- it's probably OK to have a few more memory blocks allocated to avoid recompiling every files that includes FrameLoader.h if HistoryController.h changes. But if we plan to have FrameLoader.h include HistoryController.h then I think there's no reason to use a separate memory block.

[Adam Barth]

> Elsewhere you removed the word "history" from the phrase "history item" since
> this is the history controller. I think you should do that here too. Except
> maybe there's ambiguity with back/forward item?

Will do.

> You can consider in some cases including the entire class as a member of the
> owner class.

I did this originally, but I ran into costness problems.  For example, the history access can't be const any more, but it's called from some const methods.  I can fix that with const_cast or mutable, but I wasn't sure which was the less of the two evils.

[Darin Adler]

(In reply to comment #14)
> I can fix that with const_cast or mutable, but I wasn't sure which
> was the less of the two evils.

mutable is

[Adam Barth]

Created attachment 40955 [details]
HistoryController

[Adam Barth]

I need to get on a train, but I've tried to address all your feedback in the above patch.  I also noticed that some of the methods could be made private, so I did that too.

[Adam Barth]

Committed r49413: <http://trac.webkit.org/changeset/49413>