[webkit-reviews] review denied: [Bug 50254] Canceled frame loads can corrupt back forward list : [Attachment 78057] 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 50254: Canceled frame loads can corrupt back forward list
https://bugs.webkit.org/show_bug.cgi?id=50254

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

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

> WebCore/loader/HistoryController.cpp:243
> +    recursiveGoToProvisionalItem(targetItem, currentItem, type);

recursiveSetProvisionalItem seems like a more appropriate name, since it
doesn't actually do any navigating/"going."

> WebCore/loader/HistoryController.cpp:423
> +    if (m_provisionalItem) {

Seems better to have a !m_provisionalItem early return, so that the rest
function body isn't indented by an extra level.

> WebCore/loader/HistoryController.cpp:601
> +// a match (by URL), we set the provisional item and recurse.  Otherwise we
will reload

This comment should probably be updated or removed, we no longer match by URL
(it's by item sequence number and frame name now).

> WebCore/loader/HistoryController.cpp:643
> +    if (item != fromItem 

The logic to see if the items/frames match seems complex enough that it
shouldn't be repeated in recursiveGoToProvisionalItem and recursiveGoToItem,
and instead moved to a itemTransitionNeedsLoad(item1, item2) helper method
(possibly with a better name than that).

> LayoutTests/http/tests/navigation/forward-and-cancel.html:16
> +    layoutTestController.queueLoad("javascript:frames[0].clickLink();");

Do you actually need to use the javascript: URI scheme, or can this be
queueNonLoadingScript("frames[0].clickLink()")?

> LayoutTests/http/tests/navigation/forward-and-cancel.html:27
> +   
layoutTestController.queueLoadingScript("layoutTestController.waitUntilDone();"
);

Do you need to use queueLoadingScript, or can you just call
layoutTestController.waitUntilDone() directly at the top, where you call
dumpAsText and other setup code?