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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Darin Adler <darin at apple.com>  2009-10-09 09:43:16 PDT ---
(From update of attachment 40936)
> +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+

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