Bug 66322 - Prepare frames for history navigation
Summary: Prepare frames for history navigation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-16 11:49 PDT by Gavin Peters
Modified: 2011-08-18 04:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2011-08-16 11:51 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2011-08-17 16:14 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2011-08-16 11:49:29 PDT
Advance frame state before history navigation in new frame
Comment 1 Gavin Peters 2011-08-16 11:51:22 PDT
Created attachment 104075 [details]
Patch
Comment 2 Gavin Peters 2011-08-16 11:52:04 PDT
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
Comment 3 Gavin Peters 2011-08-16 11:52:31 PDT
fishd, WDYT?
Comment 4 Darin Fisher (:fishd, Google) 2011-08-16 15:25:59 PDT
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.
Comment 5 Darin Fisher (:fishd, Google) 2011-08-16 15:30:16 PDT
(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?
Comment 6 Gavin Peters 2011-08-17 16:14:54 PDT
Created attachment 104266 [details]
Patch
Comment 7 Gavin Peters 2011-08-17 16:18:36 PDT
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 8 WebKit Review Bot 2011-08-18 04:10:58 PDT
Comment on attachment 104266 [details]
Patch

Clearing flags on attachment: 104266

Committed r93296: <http://trac.webkit.org/changeset/93296>
Comment 9 WebKit Review Bot 2011-08-18 04:11:03 PDT
All reviewed patches have been landed.  Closing bug.