Bug 34382

Summary:

When a live iframe element is moved between pages, it still depends on the old page.

Product:

WebKit

Reporter:

Dmitry Titov <dimich>

Component:

WebCore Misc.

Assignee:

Dmitry Titov <dimich>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dbates, dglazkov, eric, fishd, hausmann, kenneth, kent.hansen, levin, nicholas.young, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

31552

Attachments:

Description	Flags
Patch.	dimich: commit-queue-
Updated patch	none
Updated for review notes.	none
Patch, fixed crashes.	dimich: commit-queue-
Same patch, with style fix.	none
Updated patch.	dimich: commit-queue-
Updated patch.	fishd: review-, dimich: commit-queue-
Updated according to comments.	dimich: commit-queue-
Updated patch	none

Dmitry Titov

Reported 2010-01-30 18:26:05 PST

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.

Attachments
Patch. (10.81 KB, patch) 2010-01-30 18:45 PST, Dmitry Titov	dimich: commit-queue-	Details Formatted Diff Diff
Updated patch (10.70 KB, patch) 2010-01-30 18:52 PST, Dmitry Titov	no flags	Details Formatted Diff Diff
Updated for review notes. (11.33 KB, patch) 2010-02-01 17:56 PST, Dmitry Titov	no flags	Details Formatted Diff Diff
Patch, fixed crashes. (33.43 KB, patch) 2010-02-10 19:31 PST, Dmitry Titov	dimich: commit-queue-	Details Formatted Diff Diff
Same patch, with style fix. (33.42 KB, patch) 2010-02-10 22:20 PST, Dmitry Titov	no flags	Details Formatted Diff Diff
Updated patch. (35.55 KB, patch) 2010-02-11 18:10 PST, Dmitry Titov	dimich: commit-queue-	Details Formatted Diff Diff
Updated patch. (36.06 KB, patch) 2010-02-11 18:56 PST, Dmitry Titov	fishd: review- dimich: commit-queue-	Details Formatted Diff Diff
Updated according to comments. (35.13 KB, patch) 2010-02-12 16:47 PST, Dmitry Titov	dimich: commit-queue-	Details Formatted Diff Diff
Updated patch (35.16 KB, patch) 2010-02-12 17:46 PST, Dmitry Titov	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Dmitry Titov

Comment 1 2010-01-30 18:45:39 PST

Created attachment 47780 [details] Patch.

WebKit Review Bot

Comment 2 2010-01-30 18:47:57 PST

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.

Dmitry Titov

Comment 3 2010-01-30 18:52:18 PST

Created attachment 47781 [details] Updated patch Fixed style.

David Levin

Comment 4 2010-02-01 16:56:56 PST

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(); > +} > +

Dmitry Titov

Comment 5 2010-02-01 17:56:46 PST

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!

Daniel Bates

Comment 6 2010-02-01 20:08:19 PST

This appears to have been committed in changeset 54194 <http://trac.webkit.org/changeset/54194>.

Daniel Bates

Comment 7 2010-02-01 20:11:43 PST

Changeset 54194 <http://trac.webkit.org/changeset/54194> has caused a regression. The test case fast/frames/iframe-reparenting.html is crashing on the Qt bot. For the stderr text, see <http://build.webkit.org/results/Qt%20Linux%20Release/r54194%20%286817%29/fast/frames/iframe-reparenting-stderr.txt>. For the build results in general, see <http://build.webkit.org/results/Qt%20Linux%20Release/r54194%20%286817%29/>. And for the layout test results, see <http://build.webkit.org/builders/Qt%20Linux%20Release/builds/6817/steps/layout-test/logs/stdio>.

Nick Young

Comment 8 2010-02-01 21:08:56 PST

Something weird is happing with DRT when running those test cases. The crash is actually in fast/frames/iframe-reparenting-new-page.html. Better Backtrace: #0 0xb7ae61e7 in QWebFrame::page () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #1 0xb78ceeed in WebCore::HistoryController::saveScrollPositionAndViewStateToItem () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #2 0xb78bd933 in WebCore::FrameLoader::detachFromParent () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #3 0xb78bdaf5 in WebCore::FrameLoader::detachChildren () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #4 0xb78bd93b in WebCore::FrameLoader::detachFromParent () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #5 0xb7af98e3 in QWebPage::~QWebPage () from /home/ynichola/depot/webkit/trunk/WebKitBuild/Release/lib/libQtWebKit.so.4 #6 0x0805d4dc in WebCore::WebPage::~WebPage () #7 0xb5f40c68 in QObjectPrivate::deleteChildren (this=0x81aa268) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qobject.cpp:1980 #8 0xb5f492c9 in ~QObject (this=0x81aa538) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qobject.cpp:977 #9 0xb5f40145 in qDeleteInEventHandler (o=0x81aa538) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qobject.cpp:4006 #10 0xb5f436ca in QObject::event (this=0x81aa538, e=0x81aa570) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qobject.cpp:1225 #11 0xb621d40e in QApplicationPrivate::notify_helper (this=0x813b220, receiver=0x81aa538, e=0x81aa570) at /home/ynichola/depot/qt/qt-multimedia/src/gui/kernel/qapplication.cpp:4298 #12 0xb621d880 in QApplication::notify (this=0xbfed7b44, receiver=0x81aa538, e=0x81aa570) at /home/ynichola/depot/qt/qt-multimedia/src/gui/kernel/qapplication.cpp:3702 #13 0xb5f2c198 in QCoreApplication::notifyInternal (this=0xbfed7b44, receiver=0x81aa538, event=0x81aa570) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.cpp:704 #14 0xb621a3db in QCoreApplication::sendEvent (receiver=0x81aa538, event=0x81aa570) at ../../include/QtCore/../../../../../depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.h:215 #15 0xb5f2c722 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x8141840) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.cpp:1345 #16 0xb5f2c9b7 in QCoreApplication::sendPostedEvents (receiver=0x0, event_type=0) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.cpp:1238 #17 0xb63109d6 in QCoreApplication::sendPostedEvents () at ../../include/QtCore/../../../../../depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.h:220 #18 0xb5f67e19 in postEventSourceDispatch (s=0x81437b0) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qeventdispatcher_glib.cpp:276 #19 0xb52722f9 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0 #20 0xb527587b in ?? () from /usr/lib/libglib-2.0.so.0 #21 0xb52759f8 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0 #22 0xb5f66de2 in QEventDispatcherGlib::processEvents (this=0x814bae0, flags={i = -1074955816}) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qeventdispatcher_glib.cpp:412 #23 0xb630eea2 in QGuiEventDispatcherGlib::processEvents (this=0x814bae0, flags={i = -1074955768}) at /home/ynichola/depot/qt/qt-multimedia/src/gui/kernel/qguieventdispatcher_glib.cpp:204 #24 0xb5f28512 in QEventLoop::processEvents (this=0xbfed7aa8, flags={i = -1074955692}) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qeventloop.cpp:149 #25 0xb5f28784 in QEventLoop::exec (this=0xbfed7aa8, flags={i = -1074955600}) at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qeventloop.cpp:201 #26 0xb5f2caed in QCoreApplication::exec () at /home/ynichola/depot/qt/qt-multimedia/src/corelib/kernel/qcoreapplication.cpp:981 #27 0xb6220570 in QApplication::exec () at /home/ynichola/depot/qt/qt-multimedia/src/gui/kernel/qapplication.cpp:3577 #28 0x0806886a in main () Initial inspection seems to indicate an invalid private data pointer is being dereferenced. Will investigate a little more.

Dmitry Titov

Comment 9 2010-02-01 22:48:24 PST

Also new test crashes on Chromium mac and linux: http://build.chromium.org/buildbot/try-server/builders/layout_mac/builds/1147/steps/webkit_tests/logs/stdio Reverting....

Dmitry Titov

Comment 10 2010-02-01 22:54:39 PST

Reverted in http://trac.webkit.org/changeset/54207

Eric Seidel (no email)

Comment 11 2010-02-08 15:13:25 PST

Comment on attachment 47781 [details] Updated patch Cleared David Levin's review+ from obsolete attachment 47781 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Eric Seidel (no email)

Comment 12 2010-02-08 15:13:33 PST

Comment on attachment 47891 [details] Updated for review notes. Cleared David Levin's review+ from obsolete attachment 47891 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Dmitry Titov

Comment 13 2010-02-10 19:31:10 PST

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.

WebKit Review Bot

Comment 14 2010-02-10 19:34:12 PST

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.

Dmitry Titov

Comment 15 2010-02-10 22:20:57 PST

Created attachment 48547 [details] Same patch, with style fix. Fixed style issue.

David Levin

Comment 16 2010-02-11 12:04:31 PST

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.

Kenneth Rohde Christiansen

Comment 17 2010-02-11 12:40:17 PST

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.

Kenneth Rohde Christiansen

Comment 18 2010-02-11 12:44:26 PST

> > +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?

Dmitry Titov

Comment 19 2010-02-11 13:22:43 PST

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.

Darin Fisher (:fishd, Google)

Comment 20 2010-02-11 13:38:02 PST

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.

Darin Fisher (:fishd, Google)

Comment 21 2010-02-11 13:40:05 PST

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?

Dmitry Titov

Comment 22 2010-02-11 18:10:48 PST

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.

WebKit Review Bot

Comment 23 2010-02-11 18:11:57 PST

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.

WebKit Review Bot

Comment 24 2010-02-11 18:28:09 PST

Attachment 48600 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/259410

Dmitry Titov

Comment 25 2010-02-11 18:56:26 PST

Created attachment 48605 [details] Updated patch. Ugh. not the right patch. Now hopefully the right one.

Dmitry Titov

Comment 26 2010-02-11 19:02:51 PST

(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.

Darin Fisher (:fishd, Google)

Comment 27 2010-02-12 00:13:28 PST

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

Dmitry Titov

Comment 28 2010-02-12 01:54:49 PST

(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?

Simon Hausmann

Comment 29 2010-02-12 04:30:07 PST

Comment on attachment 48605 [details] Updated patch. > diff --git a/WebKit/qt/Api/qwebframe.h b/WebKit/qt/Api/qwebframe.h > index 25f6c9b..2ec6f64 100644 > --- a/WebKit/qt/Api/qwebframe.h > +++ b/WebKit/qt/Api/qwebframe.h > @@ -217,6 +217,8 @@ Q_SIGNALS: > void loadStarted(); > void loadFinished(bool ok); > > + void webPageChanged(); I'd prefer "pageChanged()" as the name of the signal, for consistency with the page property. > + if (oldPage != newPage) { > + m_webFrame->d->setPage(newPage); > + // The QWebFrame is created as a child of QWebPage. That adds it to QObject's internal children list and > + // ensures it will be deleted when parent QWebPage is deleted. Re-parent. > + m_webFrame->setParent(newPage); > + emit m_webFrame->webPageChanged(); I would do all these three things inside QWebFramePrivate::setPage, so that it basically becomes: void QWebFramePrivate::setPage(const QWebPage* newPage) { page = newPage; setParent(newPage); emit q->pageChanged(); } Otherwise the Qt parts look good to me! :)

Dmitry Titov

Comment 30 2010-02-12 10:08:42 PST

(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?

Dmitry Titov

Comment 31 2010-02-12 16:47:16 PST

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.

WebKit Review Bot

Comment 32 2010-02-12 17:06:08 PST

Attachment 48680 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/262247

Dmitry Titov

Comment 33 2010-02-12 17:46:29 PST

Created attachment 48684 [details] Updated patch EWS is awesome.

David Levin

Comment 34 2010-02-17 01:01:26 PST

Comment on attachment 48684 [details] Updated patch r=me (as long as there are ok's from the other two folks involved for qt and chromium specific changes). Two minor nits below. > diff --git a/WebCore/html/HTMLFrameOwnerElement.h b/WebCore/html/HTMLFrameOwnerElement.h > + virtual void setName() {} Would be nice to add a space inside of the {}. > diff --git a/WebCore/loader/EmptyClients.h b/WebCore/loader/EmptyClients.h > + virtual void didTransferChildFrameToNewDocument() { return; } "return;" isn't needed here. In other words, s/{ return; }/{ }/

Eric Seidel (no email)

Comment 35 2010-02-17 14:23:07 PST

Comment on attachment 48547 [details] Same patch, with style fix. Cleared David Levin's review+ from obsolete attachment 48547 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Eric Seidel (no email)

Comment 36 2010-02-17 14:24:10 PST

Attachment 48684 [details] was posted by a committer and has review+, assigning to Dmitry Titov for commit.

Dmitry Titov

Comment 37 2010-02-18 00:25:58 PST

Landed: http://trac.webkit.org/changeset/54938

Dmitry Titov

Comment 38 2010-02-19 10:51:52 PST

Comment on attachment 48684 [details] Updated patch cleaned flags

Kent Hansen

Comment 39 2010-03-22 09:42:40 PDT

(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!

Note You need to log in before you can comment on or make changes to this bug.