Bug 30037 – Factor PageController out of FrameLoader and Page

• Component: Page Loading
• Status: RESOLVED FIXED

[Adam Barth]

Removes a few methods that have nothing to do with loading frames.

[Adam Barth]

Created attachment 40563 [details]
Patch v1

[Adam Barth]

Comment on attachment 40563 [details]
Patch v1

This patch isn't right.  updateHistoryForAnchorScroll shouldn't go in this object.

[Adam Barth]

Created attachment 40567 [details]
PageController

[Adam Barth]

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.

[Darin Adler]

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.

[Adam Barth]

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.

[Adam Barth]

Created attachment 40576 [details]
Patch v1

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 40576 [details]
Patch v1

Clearing flags on attachment: 40576

Committed r49067: <http://trac.webkit.org/changeset/49067>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.