| Summary: | Prepare frames for history navigation | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gavin Peters <gavinp> | ||||||
| Component: | New Bugs | Assignee: | Gavin Peters <gavinp> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, beidson, creis, fishd, gavinp, japhet, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Gavin Peters
2011-08-16 11:49:29 PDT
Created attachment 104075 [details]
Patch
Comment on attachment 104075 [details]
Patch
By advancing the state machine for a new Frame like this, we avoid getting explicit reloads on history navigation when a back/forward navigation happens to cross a renderer boundary
fishd, WDYT? Comment on attachment 104075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104075&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:905 > + FrameLoaderStateMachine* stateMachine = m_frame->loader()->stateMachine(); The state machine feels like an internal detail of the FrameLoader. I suspect it may be better to add a method on FrameLoader that is expected to perform this fixup. I wonder if we shouldn't move some of the setCurrentItem calls behind such a method too. The challenge is to come up with a good name that properly describes the work. (In reply to comment #2) > (From update of attachment 104075 [details]) > By advancing the state machine for a new Frame like this, we avoid getting explicit reloads on history navigation when a back/forward navigation happens to cross a renderer boundary Yes, I think this is the correct fixup to perform when we find that there is no current HistoryItem. One idea is to express all of this in terms of some kind of alternative to Frame::init(), or perhaps as a method that can be called after Frame::init(). Maybe doing this fixup inside loadHistoryItem is the wrong thing. Maybe Chromium should call some extra function after creating the WebView. Maybe instead of initializeMainFrame we should have an initializeMainFrameWithHistoryItem? Or, maybe initializeMainFrame should take a WebHistoryItem parameter? Created attachment 104266 [details]
Patch
Comment on attachment 104266 [details]
Patch
Thanks for the review Darin.
I've elected to add a method to the FrameLoader, and call it from loadFromHistory in the WebFrameImpl. I thought that this was simple enough, and accomplished the goal of having an understandable method that conses up a frame suitable for history navigation.
WDYT?
Comment on attachment 104266 [details] Patch Clearing flags on attachment: 104266 Committed r93296: <http://trac.webkit.org/changeset/93296> All reviewed patches have been landed. Closing bug. |