Bug 66322 – Prepare frames for history navigation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gavin Peters]

Advance frame state before history navigation in new frame

[Gavin Peters]

Created attachment 104075 [details]
Patch

[Gavin Peters]

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

[Gavin Peters]

fishd, WDYT?

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

(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?

[Gavin Peters]

Created attachment 104266 [details]
Patch

[Gavin Peters]

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?

[WebKit Review Bot]

Comment on attachment 104266 [details]
Patch

Clearing flags on attachment: 104266

Committed r93296: <http://trac.webkit.org/changeset/93296>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.