Bug 34382 - When a live iframe element is moved between pages, it still depends on the old page.
: When a live iframe element is moved between pages, it still depends on the ol...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 31552
  Show dependency treegraph
 
Reported: 2010-01-30 18:26 PST by
Modified: 2010-03-22 09:42 PST (History)


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


Note

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


Description From 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.
------- Comment #1 From 2010-01-30 18:45:39 PST -------
Created an attachment (id=47780) [details]
Patch.
------- Comment #2 From 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.
------- Comment #3 From 2010-01-30 18:52:18 PST -------
Created an attachment (id=47781) [details]
Updated patch

Fixed style.
------- Comment #4 From 2010-02-01 16:56:56 PST -------
(From update of attachment 47781 [details])
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();
> +}
> +
------- Comment #5 From 2010-02-01 17:56:46 PST -------
Created an attachment (id=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!
------- Comment #6 From 2010-02-01 20:08:19 PST -------
This appears to have been committed in changeset 54194 <http://trac.webkit.org/changeset/54194>.
------- Comment #7 From 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>.
------- Comment #8 From 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.
------- Comment #9 From 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....
------- Comment #10 From 2010-02-01 22:54:39 PST -------
Reverted in http://trac.webkit.org/changeset/54207
------- Comment #11 From 2010-02-08 15:13:25 PST -------
(From update of attachment 47781 [details])
Cleared David Levin's review+ from obsolete attachment 47781 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #12 From 2010-02-08 15:13:33 PST -------
(From update of attachment 47891 [details])
Cleared David Levin's review+ from obsolete attachment 47891 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #13 From 2010-02-10 19:31:10 PST -------
Created an attachment (id=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.
------- Comment #14 From 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.
------- Comment #15 From 2010-02-10 22:20:57 PST -------
Created an attachment (id=48547) [details]
Same patch, with style fix.

Fixed style issue.
------- Comment #16 From 2010-02-11 12:04:31 PST -------
(From update of attachment 48547 [details])

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 #17 From 2010-02-11 12:40:17 PST -------
(From update of attachment 48547 [details])

>  /*!
> +  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.
------- Comment #18 From 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?
------- Comment #19 From 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.
------- Comment #20 From 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.
------- Comment #21 From 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?
------- Comment #22 From 2010-02-11 18:10:48 PST -------
Created an attachment (id=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.
------- Comment #23 From 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.
------- Comment #24 From 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
------- Comment #25 From 2010-02-11 18:56:26 PST -------
Created an attachment (id=48605) [details]
Updated patch.

Ugh. not the right patch. Now hopefully the right one.
------- Comment #26 From 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.
------- Comment #27 From 2010-02-12 00:13:28 PST -------
(From update of attachment 48605 [details])
> +++ 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
------- Comment #28 From 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?
------- Comment #29 From 2010-02-12 04:30:07 PST -------
(From update of attachment 48605 [details])
> 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! :)
------- Comment #30 From 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?
------- Comment #31 From 2010-02-12 16:47:16 PST -------
Created an attachment (id=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.
------- Comment #32 From 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
------- Comment #33 From 2010-02-12 17:46:29 PST -------
Created an attachment (id=48684) [details]
Updated patch

EWS is awesome.
------- Comment #34 From 2010-02-17 01:01:26 PST -------
(From update of attachment 48684 [details])
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; }/{ }/
------- Comment #35 From 2010-02-17 14:23:07 PST -------
(From update of attachment 48547 [details])
Cleared David Levin's review+ from obsolete attachment 48547 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #36 From 2010-02-17 14:24:10 PST -------
Attachment 48684 [details] was posted by a committer and has review+, assigning to Dmitry Titov for commit.
------- Comment #37 From 2010-02-18 00:25:58 PST -------
Landed: http://trac.webkit.org/changeset/54938
------- Comment #38 From 2010-02-19 10:51:52 PST -------
(From update of attachment 48684 [details])
cleaned flags
------- Comment #39 From 2010-03-22 09:42:40 PST -------
(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!