[webkit-reviews] review denied: [Bug 50254] Canceled frame loads can corrupt back forward list : [Attachment 76713] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd 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 76713: Patch
https://bugs.webkit.org/attachment.cgi?id=76713&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76713&action=review

> WebCore/loader/FrameLoader.cpp:2390
> +			  
page->mainFrame()->loader()->history()->cancelCurrentItem();

The name cancelCurrentItem is not so great.  You aren't really cancelling it.
You are cancelling / reverting the switch to it.

Also, it seems like you might need to cancel more than just the current item
of the top-level frame.  What if m_frame is actually a grandchild of the main
frame?	What about other sibling frames that aren't navigating?  R- because
of this issue.

I wonder if we shouldn't be making use of HistoryController::m_provisionalItem
more.  It seems like we shouldn't commit to changing m_currentItem until we
know that it is going to stick.  Although that might be a much larger change
:-/