[Webkit-unassigned] [Bug 50254] Canceled frame loads can corrupt back forward list

[bugzilla-daemon at webkit.org]

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


Mihai Parparita <mihaip at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78057|review?                     |review-
               Flag|                            |




--- Comment #5 from Mihai Parparita <mihaip at chromium.org>  2011-01-06 10:57:40 PST ---
(From update of attachment 78057)
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?

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