[webkit-reviews] review canceled: [Bug 48812] FrameLoader::checkLoadCompleteForThisFrame uses wrong history item : [Attachment 74305] Patch

[bugzilla-daemon at webkit.org]

Charles Reis <creis at chromium.org> has canceled Charles Reis
<creis at chromium.org>'s request for review:
Bug 48812: FrameLoader::checkLoadCompleteForThisFrame uses wrong history item
https://bugs.webkit.org/show_bug.cgi?id=48812

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

------- Additional Comments from Charles Reis <creis at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74305&action=review

>> WebCore/loader/FrameLoader.cpp:2380
>> +			if (pdl->triggeringAction().historyItem() ==
page->backForward()->currentItem()) {
> 
> Can we make use HistoryController::currentItem() or perhaps
HistoryController::provisionalItem()?
> It seems that in the back/forward navigation case,
HistoryController::goToItem sets
> HistoryController::m_provisionalItem, and then when we commit the load, that
gets promoted to
> HistoryController::m_currentItem.  If another load comes in after that, then
the BackForwardList's
> currentItem would change to correspond to the new provisional HistoryItem.
> 
> I'm also a bit confused as to why this code only assigns |item| when the
current frame is the
> main frame.  This makes me wonder if calling BackForwardList::setCurrentItem
here is really
> the right thing to do.  If a navigation only occurs in a subframe, we'd still
want to
> potentially update the BackForwardList.

Yes, I think history()->provisionalItem() should be the same as
backForward()->currentItem() here, and it's a bit clearer to understand that
way.  I've updated the comment as well to make this easier to follow.  I'll
upload that once the tests pass.

I'm less sure about the frame issue.  I'll take a look while the tests are
running, but we may want to tackle that after this fix goes in.