Bug 30037

Created attachment 40563 [details] Patch v1

Comment on attachment 40563 [details] Patch v1 This patch isn't right. updateHistoryForAnchorScroll shouldn't go in this object.

Created attachment 40567 [details] PageController

CCing Julie because this is undoubtably going to break the Chromium WebKit canary when it lands. We'll need to add some "->controller()" action to the Chromium port too.

Comment on attachment 40567 [details] PageController The object named Page is already a page controller, so I don't think this is the right name for a new object. On the other hand it has frequently been a problem that there is a loader for each frame, but no corresponding loader for the page. Could you describe the distinction between what would go on the page and what on the page controller? Maybe that can give us a better idea what its name should be.

My main goal for this patch is to remove these methods from FrameLoader because they have nothing to do with FrameLoader: - bool canGoBackOrForward(int distance) const; - void goBackOrForward(int distance); - int getHistoryLength(); They ought to be methods of some page-scoped object. Looking around, they seem related to these methods of Page: - bool goBack(); - bool goForward(); - void goToItem(HistoryItem*, FrameLoadType); I was going to add them to the Page object, but I saw this comment: - // FIXME: The following three methods don't fall under the responsibilities of the Page object - // They seem to fit a hypothetical Page-controller object that would be akin to the - // Frame-FrameLoader relationship. They have to live here now, but should move somewhere that - // makes more sense when that class exists. I didn't want to add to the problem described by this comment, so I created the hypothetical Page-controller object. If you'd prefer we just moved the methods from FrameLoader to Page, I'd be happy to do that (and it would make the patch MUCH smaller). We can sort out whether to create the hypothetical Page-controller object later.

Created attachment 40576 [details] Patch v1

Here's what that patch would look like. Even if we do decide we want something like a page controller, we should probably do that in a separate patch.

Comment on attachment 40576 [details] Patch v1 Clearing flags on attachment: 40576 Committed r49067: <http://trac.webkit.org/changeset/49067>

All reviewed patches have been landed. Closing bug.