Bug 30037 - Factor PageController out of FrameLoader and Page
Summary: Factor PageController out of FrameLoader and Page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29947
  Show dependency treegraph
 
Reported: 2009-10-02 21:48 PDT by Adam Barth
Modified: 2009-10-14 23:08 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (10.29 KB, patch)
2009-10-02 21:49 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
PageController (47.29 KB, patch)
2009-10-02 23:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch v1 (11.31 KB, patch)
2009-10-03 08:46 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-10-02 21:48:18 PDT
Removes a few methods that have nothing to do with loading frames.
Comment 1 Adam Barth 2009-10-02 21:49:54 PDT
Created attachment 40563 [details]
Patch v1
Comment 2 Adam Barth 2009-10-02 22:21:42 PDT
Comment on attachment 40563 [details]
Patch v1

This patch isn't right.  updateHistoryForAnchorScroll shouldn't go in this object.
Comment 3 Adam Barth 2009-10-02 23:48:24 PDT
Created attachment 40567 [details]
PageController
Comment 4 Adam Barth 2009-10-02 23:51:57 PDT
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 5 Darin Adler 2009-10-03 07:17:43 PDT
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.
Comment 6 Adam Barth 2009-10-03 08:21:49 PDT
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.
Comment 7 Adam Barth 2009-10-03 08:46:23 PDT
Created attachment 40576 [details]
Patch v1
Comment 8 Adam Barth 2009-10-03 08:48:22 PDT
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 9 Adam Barth 2009-10-03 17:17:18 PDT
Comment on attachment 40576 [details]
Patch v1

Clearing flags on attachment: 40576

Committed r49067: <http://trac.webkit.org/changeset/49067>
Comment 10 Adam Barth 2009-10-03 17:17:22 PDT
All reviewed patches have been landed.  Closing bug.