Since http://trac.webkit.org/changeset/53871, the iframe may be moved around in the DOM without being re-loaded. When this happens, we need to update frame tree and make the contentFrame to point to a new Page.
Patch forthcoming.

Attachment 47780[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/Frame.cpp:1601: Missing space before ( in if( [whitespace/parens] [5]
Total errors found: 1
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 47781[details]
Updated patch
Consider addressing the items below before landing.
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +2010-01-30 Dmitry Titov <dimich@chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + When live iframe element is moved between pages, it still depends on old page.
s/When live/When a live/
s/on old/on the old/
> diff --git a/LayoutTests/fast/frames/iframe-reparenting-new-page.html b/LayoutTests/fast/frames/iframe-reparenting-new-page.html
It looks like the test could easily be in the script only form which seems preferred when possible.
> +<html>
> +<script>
...
> +<body onload="test()">
> +The test verifies that loaded iframe may be passed between different pages without stopping firing timer events. It used window.open to open 'page 1' which has iframe in it. It then passes iframe to 'page 2' and closes window of 'page 1'. The test verifies that the timer in iframe that is initialized when iframe is loaded continues firing after iframe is re-parented and original window was closed. If test passes, you'll see a few log entries and then "FINISH".
This test verifies that the loaded iframe may be passed between different pages without stopping firing timer events. It used window.open to open 'page 1' which has an iframe in it. It then passes the iframe to 'page 2' and closes the window of 'page 1'.
This test verifies that the timer in an iframe that is initialized when the iframe is loaded continues firing after the iframe is re-parented and the original window was closed. If test passes, you'll see a few log entries and then "FINISH".
The second "This test verifies" seems like overkill. I would delete all the text before that.
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-01-30 Dmitry Titov <dimich@chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + When live iframe element is moved between pages, it still depends on old page.
When a live... it still depends on the old page.
> + https://bugs.webkit.org/show_bug.cgi?id=34382
> +
> + Test: fast/frames/iframe-reparenting-new-page.html
> +
> + * html/HTMLFrameElementBase.cpp:
> + (WebCore::HTMLFrameElementBase::setName): Move the code setting the frame name into a separate function.
> + (WebCore::HTMLFrameElementBase::setNameAndOpenURL):
> + (WebCore::HTMLFrameElementBase::updateLiveFrame): Update frame tree, reset page in the contentFrame and re-set the name.
> + (WebCore::HTMLFrameElementBase::insertedIntoDocument):
> + * html/HTMLFrameElementBase.h:
> + * page/Frame.cpp:
> + (WebCore::Frame::setPage):
> + * page/Frame.h: Add setPage method. It is only currently used when alive iframe is moved between pages (so the existing Frame should get a new Page)
Please add a period. Also, I think ChangeLog entries typically line wrap at ~80-90 columns (unlike nearly all other files).
> diff --git a/WebCore/html/HTMLFrameElementBase.cpp b/WebCore/html/HTMLFrameElementBase.cpp> +// Used when live frame is moved in DOM, potentially to another page.
Is the term "live frame" used elsewhere? What does it mean?
> diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
> +void Frame::setPage(Page* page)
> +{
Shouldn't this ASSERT(ownerElement) ?
> + if (m_page == page)
> + return;
> +
> + if (m_page)
> + m_page->decrementFrameCount();
> +
> + m_page = page;
> +
> + if (page)
> + page->incrementFrameCount();
> +}
> +

Created attachment 47891[details]
Updated for review notes.
Fixed all - typos, added ASSERT, converted test to a script test.
There are quite a lot of changes, especially in the test - could you please take another look? Thanks!

Created attachment 48543[details]
Patch, fixed crashes.
The crashes in Chromium and QT were caused by their WebKit layer assuming the Frame always belongs to a given Page. When iframe is passed between documents, it can also be passed between windows, which requires corresponding update in WebKit layer.
This is acieved by adding FrameLoaderClient::adoptFrame(Frame*) method which is called when Frame is moved in DOM.

Attachment 48543[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/Frame.cpp:1666: One line control clauses should not use braces. [whitespace/braces] [4]
Ignoring "WebKit/qt/Api/qwebframe.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.h": this file is exempt from the style guide.
Total errors found: 1
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 48547[details]
Same patch, with style fix.
1. I would remove all notImplemented(); I believe these imply that there is a missing implementation, but that is not the case here. The ports with empty methods simply don't need an implementation as far as we know.
2. I'm concerned about the change in WebKit/qt/Api/qwebframe.h. Does this mean that the public api in qt is changing? It looks like Kenneth Rohde Christiansen < kenneth@webkit.org> has been looking at these changes. Perhaps you could get him to look at this part of your patch.
r=me after kenneth gives an ok (and it would be nice to address the other things mentioned).
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-02-10 Dmitry Titov <dimich@chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + When a live iframe element is moved between pages, it still depends on the old page.
> + https://bugs.webkit.org/show_bug.cgi?id=34382
> +
> + Test: fast/frames/iframe-reparenting-new-page.html
> +
> + * html/HTMLFrameElementBase.cpp:
> + (WebCore::HTMLFrameElementBase::setName):
> + Move the code setting the frame name into a separate function.
> +
> + (WebCore::HTMLFrameElementBase::setNameAndOpenURL):
> + (WebCore::HTMLFrameElementBase::updateOnReparenting):
> + Called on the frame that was just re-parented and inserted into another Document.
> + Simply invoke Frame::updateOnreparenting(newParentFrame);
updateOnReparenting
^ capital R
In WebKit/chromium/ChangeLog
> + if Frame moves between Pages the client of corresponding WebFrame
> + should be replaced as well.
add a "," after Pages.

Comment on attachment 48547[details]
Same patch, with style fix.
> /*!
> + The frame is moved between pages - replace the page.
> +
> + \note This can not be done on a main frame of the page.
> +*/
Needs \since 4.7 and it needs to block the Qt 4.7 API tracker:
https://bugs.webkit.org/show_bug.cgi?id=31552> +void QWebFrame::setPage(QWebPage* newPage)
> +{
> + Q_ASSERT(d->page);
> + Q_ASSERT(newPage);
> + Q_ASSERT(page()->mainFrame() != this);
> + if (d->page != newPage) {
> + d->page = newPage;
> + // The QWebFrame is created as a child of QWebPage. That adds it to QObject's internal childrent list and
> + // ensures it will be deleted when parent QWebPage is deleted. Re-parent.
> + setParent(newPage);
> + }
> +}
What is the use case? Can this only happen through the public Qt C++ API, or can it happen in any other way? IF so we might need a signal.

> > +void QWebFrame::setPage(QWebPage* newPage)
As this is called from FrameLoaderClientQt, we probably need a signal as well.
I'm not sure we want this as a public API, maybe put it in QWebFramePrivate for now?
Simon?

Indeed, this can not happen via public WebKit API from outside - this happens as a result of re-parenting iframe element from one document to another.
As discussed on IRC, I think I should move setPage() to QWebFramePrivate and emit a signal on QWebFrame when the page associated with it changes.
I'll have an updated patch shortly, after building/testing on Qt.

FrameLoaderClient::adoptFrame seems incorrectly named. it sounds like it is a command to the FrameLoaderClient to perform the "adopt frame" operation. In reality, it appears to be a notification to the WebKit layer that a frame was moved from one document to another. A better name might be didAdoptFrame or didTransferFrameToNewDocument.

I think we will need a similar notification on WebFrameClient to let the embedder know that the frame moved from one document to another.
Also, what happens to session history when a frame moves between documents in separate Pages? What if some session history entries correspond only to navigations within the frame that was moved. Should those session history navigations also move from the old Page to the new Page?

Created attachment 48600[details]
Updated patch.
Thanks all for great feedback. Updated patch:
- [Qt] removed public method from QWebFrame, it should not be a new API. Moved it to QWebFramePrivate
- [Qt] now emit signal, QWebFrame::webPageChanged() when frame moves between pages.
- [Chromium] added virtual WebFrameClient::didTransferChildFrameToNewDocument(WebDocument) to signal the embedder the frame was reparented.
- renamed FrameLoaderClient::adoptFrame(Frame*) to FrameLoaderClient::didTransferChildFrameToNewDocument(Document*), renamed corresponding Frame method as well.
- FrameLoaderClient::didTransferChildFrameToNewDocument(Document*) is now called on the frame loader client of the transferred frame, rather then on the client of the new parent frame.
- removed notImplemented() as suggested by Dave, I agree there is nothing 'not implemented' there as we know.
- fiksed some typos.
- test is unchanged
I'll wait for r+ from Dave and also OK from Kenneth or Simon and Darin Fisher, on Qt and Chromium code in particular.

Attachment 48600[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:40: Alphabetical sorting problem. [build/include_order] [4]
Ignoring "WebKit/qt/Api/qwebframe_p.h": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.h": this file is exempt from the style guide.
Total errors found: 1
If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #21)
> Also, what happens to session history when a frame moves between documents in
> separate Pages? What if some session history entries correspond only to
> navigations within the frame that was moved. Should those session history
> navigations also move from the old Page to the new Page?
That is going to be a subject of the separate test+fix. I think the page that looses iframe should do the same as today when iframe is removed, and the one that receives it should probably gain no new history. I'll need to figure out more details on how the session history works today in WebKit. Spec says each nested browsing context has its own session history which is simple but I guess it's not that simple in current implementation. Will address it in a separate patch. Thanks a lot for headsup.

Comment on attachment 48605[details]
Updated patch.
> +++ b/WebKit/chromium/public/WebFrameClient.h
...
> +#include "WebDocument.h"
nit: no need to include WebDocument.h. just forward declare the type.
> + // The child frame was transferred to a new page, due to DOM operation.
> + // It's parent frame, document and containing view has changed.
> + virtual void didTransferChildFrameToNewDocument(WebDocument) { }
all WebFrameClient methods must start with a WebFrame* that is the subject
of the notification.
one thing is a bit confusing about this method: how do i find out the
WebFrame that i was transferred from?
also, i realized after we discussed this patch that an even better name
might be didReparentFrame. and, since the old parent seems like useful
information to pass (the new parent is available at WebFrame::parent),
how about this signature:
void didReparent(WebFrame*, WebFrame* oldParent);
You could place this next to willClose since it is a similar sort of
notification.
I'm surprised that you call transferChildFrameToNewDocument on each
child frame of the frame being reparented. I'm guessing this is b/c
you want to have the WebKit layer update the WebFrameClients for each
of the child frames. That might be better left to the FrameLoaderClient
implementation since it will know if that is necessary. As is, the
notification isn't really correct: the child frame didn't get moved to
a new document, and it didn't get reparented.
-Darin

(In reply to comment #27)
> I'm surprised that you call transferChildFrameToNewDocument on each
> child frame of the frame being reparented. I'm guessing this is b/c
> you want to have the WebKit layer update the WebFrameClients for each
> of the child frames. That might be better left to the FrameLoaderClient
> implementation since it will know if that is necessary. As is, the
> notification isn't really correct: the child frame didn't get moved to
> a new document, and it didn't get reparented.
Child Frames should get a new Page pointer. This is the main reason to iterate child frames in Frame::transferChildFrameTonewDocument. So this loop exists for both WebCore and WebKit reasons and as such it's reasonable to have it in Frame rather then in FrameLoaderClient.
Speaking about WebFrameClient notification, I see it's a bit awkward . Do we really need it (in chromium WebKit layer)? What would we do in it? The signature you proposed:
void didReparent(WebFrame*, WebFrame* oldParent);
doesn't also look ideal because for child frames it doesn't make a lot of sense - the child frames are not really reparented, and their parent frame does not change.
How about we just don't have it until we have a need for it?

(In reply to Darin Fisher's comment #27)
I don't think I phrased my response well. Here is another attempt:
I agree the transferChildFrameToNewDocument() seems to do more then its name says. For example, it recurses into children, while they do not get transferred to a new document.
Perhaps the better way would be to split out the recursive Frame::setPage(Page newPage) which would set Page pointer in a frame subtree and call it from transferChildFrameToNewDocument. Then, add a loop in Chromium's FrameLoaderClientImpl::didTransferChildFrameToNewDocument to update client pointers, as you suggested. This way, each function will do what it names says...
Separately, we might remove WebFrameClient external notification until the moment we'll actually need an implementation of it - it may give us better understanding what parameters it needs at that time.
Do you think it's a reasonable plan?

Created attachment 48680[details]
Updated according to comments.
I think I've got all comments in. Changes:
- [Qt] webPageChanged -> pageChanged and moved page setting code into setPage(), as requested.
- [Qt] realized that main frame has QWebPage as QObject parent, while child frames have their parent's QWebFrames as QObject parent - adjusted code for that.
- [Chromium] - chatted with Darin and removed WebFrameClient notification until the time we realize we need it.
- FrameLoaderClient::didTransferChildFrameToNewDocument lost "Document*" parameter, since the new document can always be pulled from frame.
- test did not change.

(In reply to comment #37)
> Landed: http://trac.webkit.org/changeset/54938
Hi Dmitry,
This change adds public API to Qt (QWebFrame::pageChanged()) without documentation. Ideally it should also have a test under WebKit/qt/tests/qwebframe.
Could you please add the missing doc and test, or suggest a wording (describing when one would want to connect to this signal) so I can add it.
Thanks!