[Webkit-unassigned] [Bug 54517] Ensure loading has stopped in HistoryController::goToItem

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54517





--- Comment #11 from Charles Reis <creis at chromium.org>  2011-02-18 12:14:33 PST ---
(From update of attachment 82860)
View in context: https://bugs.webkit.org/attachment.cgi?id=82860&action=review

>> Source/WebCore/page/Page.cpp:341

> 
> This feels pretty icky. Since we're not under the gun to get this into M10, I'd like to explore a cleaner solution.  The current check for the back-forward scheme is done by Chromium's implementation of FrameLoaderClient::shouldGoToHistoryItem. Adding a FrameLoaderClient::shouldStopLoadingForHistoryItem seems reasonable.
> 
> Additionally, having all this history logic in Page seems strange. Putting all these checks (lines 335 to 343) in a static HistoryController::shouldStopLoadingForHistoryItemTransition(currentItem, item) method may be cleaner.

Good ideas.  I've switched these over and updated all the clients.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:895
>> +    // Use Page::goToItem to ensure stopAllLoaders is called first (except for
> 
> Can we hide HistoryController::goToItem so that all calls go through Page (make it private and make Page a friend?)

Done.

>> LayoutTests/http/tests/navigation/resources/forward-to-fragment-fires-onload-2.html:31
>> +    setTimeout(function() {
> 
> Shouldn't be necessary to do this in a timeout, history.forward() is already async.

Actually, the test doesn't work unless it's in a timeout.  It appears to be important to have the history.forward() happen after the page finishes parsing.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.