[webkit-reviews] review denied: [Bug 79206] REGRESSION(r106388): Form state is restored to a wrong document : [Attachment 129382] Patch 4

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 79206: REGRESSION(r106388): Form state is restored to a wrong document
https://bugs.webkit.org/show_bug.cgi?id=79206

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129382&action=review


I like this approach!

But have a few problems with this patch:

> Source/WebCore/loader/FrameLoader.cpp:3174
> +    m_requestedHistoryItem = item;

m_requestedHistoryItem is set in ::loadItem(), but never cleared unless a new
loadItem takes place.

Nominally this doesn't seem so bad - Just an extra HistoryItem hanging around
that will be destroyed when the FrameLoader is.

But HistoryItems can be surprisingly heavyweight objects, and I think we should
clear it out once we don't need it anymore.  Perhaps as the very last part of
didFinishLoad?

> Source/WebCore/loader/FrameLoader.cpp:3187
> +bool FrameLoader::didLoadWithLoadItem() const
> +{
> +    return m_requestedHistoryItem.get() == history()->currentItem();
> +}

This method name makes me cringe.  Plus, what the method does seems
unnecessarily specific to this problem.

I would suggest one of two things:
1 - Make it an accessor for the requestedItem, and have HistoryController do
the specific comparison it needs.
2 - Rename it "isRequestedItemCurrent" or "wasCurrentItemRequested"

In fact, I hate both of those names too...  I'd personally do number 1...  ;)