Bug 34382 - When a live iframe element is moved between pages, it still depends on the old page.
Summary: When a live iframe element is moved between pages, it still depends on the ol...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dmitry Titov
URL:
Keywords:
Depends on:
Blocks: 31552
  Show dependency treegraph
 
Reported: 2010-01-30 18:26 PST by Dmitry Titov
Modified: 2010-03-22 09:42 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 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 Dmitry Titov 2010-01-30 18:45:39 PST
Created attachment 47780 [details]
Patch.
Comment 2 WebKit Review Bot 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 Dmitry Titov 2010-01-30 18:52:18 PST
Created attachment 47781 [details]
Updated patch

Fixed style.
Comment 4 David Levin 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();
> +}
> +
Comment 5 Dmitry Titov 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!
Comment 6 Daniel Bates 2010-02-01 20:08:19 PST
This appears to have been committed in changeset 54194 <http://trac.webkit.org/changeset/54194>.
Comment 7 Daniel Bates 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 Nick Young 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 Dmitry Titov 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 Dmitry Titov 2010-02-01 22:54:39 PST
Reverted in http://trac.webkit.org/changeset/54207
Comment 11 Eric Seidel (no email) 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.
Comment 12 Eric Seidel (no email) 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.
Comment 13 Dmitry Titov 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.
Comment 14 WebKit Review Bot 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 Dmitry Titov 2010-02-10 22:20:57 PST
Created attachment 48547 [details]
Same patch, with style fix.

Fixed style issue.
Comment 16 David Levin 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.
Comment 17 Kenneth Rohde Christiansen 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.
Comment 18 Kenneth Rohde Christiansen 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 Dmitry Titov 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 Darin Fisher (:fishd, Google) 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 Darin Fisher (:fishd, Google) 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 Dmitry Titov 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.
Comment 23 WebKit Review Bot 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 WebKit Review Bot 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 Dmitry Titov 2010-02-11 18:56:26 PST
Created attachment 48605 [details]
Updated patch.

Ugh. not the right patch. Now hopefully the right one.
Comment 26 Dmitry Titov 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 Darin Fisher (:fishd, Google) 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
Comment 28 Dmitry Titov 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 Simon Hausmann 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! :)
Comment 30 Dmitry Titov 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 Dmitry Titov 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.
Comment 32 WebKit Review Bot 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 Dmitry Titov 2010-02-12 17:46:29 PST
Created attachment 48684 [details]
Updated patch

EWS is awesome.
Comment 34 David Levin 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; }/{ }/
Comment 35 Eric Seidel (no email) 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.
Comment 36 Eric Seidel (no email) 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 Dmitry Titov 2010-02-18 00:25:58 PST
Landed: http://trac.webkit.org/changeset/54938
Comment 38 Dmitry Titov 2010-02-19 10:51:52 PST
Comment on attachment 48684 [details]
Updated patch

cleaned flags
Comment 39 Kent Hansen 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!