[webkit-reviews] review denied: [Bug 48809] Update correct content state during back/forward navigations : [Attachment 73802] 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 48809: Update correct content state during back/forward navigations
https://bugs.webkit.org/show_bug.cgi?id=48809

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

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

> WebCore/loader/FrameLoader.cpp:1164
> +    m_client->dispatchDidNavigateWithinPage();

Moving this here seems wrong to me.  This notification is meant to be
generated after the within-page navigation completes.  I'm concerned
that checkCompleted and checkLoadCompleted change our state in ways
that should be observable from dispatchDidNavigateWithinPage.  For
example, it looks like checkCompleted may generate a readystatechange
event on the document.

It still seems to me that the right solution is to not clear m_previousItem.
Given your change to setCurrentItem, does the forward navigation issue go
away by any chance?  Can you try to unravel what is going on there?

I can point to places in HistoryController that assume m_previousItem will
be valid.  See HistoryController::createItemTree.