[webkit-reviews] review denied: [Bug 54517] Ensure loading has stopped in HistoryController::goToItem : [Attachment 82860] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Charles Reis
<creis at chromium.org>'s request for review:
Bug 54517: Ensure loading has stopped in HistoryController::goToItem
https://bugs.webkit.org/show_bug.cgi?id=54517

Attachment 82860: Patch
https://bugs.webkit.org/attachment.cgi?id=82860&action=review

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82860&action=review

> Source/WebCore/page/Page.cpp:341
> +#if PLATFORM(CHROMIUM)

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.

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

>
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.