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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 40952: HistoryController
https://bugs.webkit.org/attachment.cgi?id=40952&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -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.