Today, Frame::create() calls Page::setMainFrame() when creating the main Frame for a Page. This means that there's a period between Page construction and Frame construction where Page::m_mainFrame is null. My goal here is to make Page::mainFrame() guaranteed non-null to disambiguate the code and remove a myriad of null checks. I will be needing some help from port maintainers to get main Frame construction right since it involves setting up the initial FrameLoaderClient in port-specific ways.
Created attachment 209015 [details] First stab for EWS Here's an initial attempt at this. - The Page ctor now takes a "FrameLoaderClient* loaderClientForMainFrame" argument. - The Page ctor now creates the mainFrame(). - The WebKit layer is modified to create its WebFrame classes wrapping an already instantiated WebCore::Frame.
Created attachment 209016 [details] First stab for EWS + SVGImage bugfix
Comment on attachment 209016 [details] First stab for EWS + SVGImage bugfix Attachment 209016 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1493209
Comment on attachment 209016 [details] First stab for EWS + SVGImage bugfix Attachment 209016 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1501200
Comment on attachment 209016 [details] First stab for EWS + SVGImage bugfix Attachment 209016 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1439221
Comment on attachment 209016 [details] First stab for EWS + SVGImage bugfix Attachment 209016 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1394258
Created attachment 209025 [details] Second stab for EWS Removed the extra Page constructor argument and added a PageClients::loaderClientForMainFrame pointer instead.
Created attachment 209043 [details] Patch
(In reply to comment #8) > Created an attachment (id=209043) [details] > Patch I can take it for Qt port, If nobody is involved :)
Comment on attachment 209043 [details] Patch This patch is missing the changes needed to build under Windows. Once you land the patch, I can commit a build correction that adds this for Windows.
Created attachment 209093 [details] GTK-WK1 changes Here are the changes required for the GTK port. Thanks for waiting for these.
Comment on attachment 209093 [details] GTK-WK1 changes View in context: https://bugs.webkit.org/attachment.cgi?id=209093&action=review > Source/WebKit/gtk/webkit/webkitwebframe.cpp:556 > - WebKit::FrameLoaderClient* client = new WebKit::FrameLoaderClient(frame); > - priv->coreFrame = Frame::create(viewPriv->corePage, 0, client).get(); > + priv->coreFrame = viewPriv->corePage->mainFrame(); > + static_cast<WebKit::FrameLoaderClient*>(viewPriv->corePage->mainFrame()->loader().client())->setWebFrame(frame); This looks wrong to me, I think you'll end up with all WebKitWebFrames referring to the main frame (and will change the WebKitWebFrame the main frame's LoaderClient uses whenever a new one is created).
Comment on attachment 209093 [details] GTK-WK1 changes View in context: https://bugs.webkit.org/attachment.cgi?id=209093&action=review >> Source/WebKit/gtk/webkit/webkitwebframe.cpp:556 >> + static_cast<WebKit::FrameLoaderClient*>(viewPriv->corePage->mainFrame()->loader().client())->setWebFrame(frame); > > This looks wrong to me, I think you'll end up with all WebKitWebFrames referring to the main frame (and will change the WebKitWebFrame the main frame's LoaderClient uses whenever a new one is created). OK, I discussed this with Zan on IRC, we will further refactor this in a separate bug to clear this confusion up, I'm good with landing it as is.
(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=209043) [details] [details] > > Patch > > I can take it for Qt port, If nobody is involved :) Guys, Qt side patch lives in https://gist.github.com/ararunprasad/6271826.
I'm having some trouble getting this working under Windows, mainly because the Windows WebFrame is subclassed from WebFrameLoaderClient. I think this might be easier to land if I could do an initial patch that separated the WebFrameLoaderClient logic from WebFrame (have WebFrame contain a WebFrameLoaderClient member). Once that is done, then this change would become much easier.
I added a blocking bug (Bug 120092) to refactor the Windows code to make these changes easier.
Committed r154616: <http://trac.webkit.org/changeset/154616>
(In reply to comment #14) > (In reply to comment #9) > > (In reply to comment #8) > > > Created an attachment (id=209043) [details] [details] [details] > > > Patch > > > > I can take it for Qt port, If nobody is involved :) > > Guys, Qt side patch lives in https://gist.github.com/ararunprasad/6271826. Could you land that change? I'm not sure I can do it since it hasn't been through the patch upload license agreement here on bugzilla.
(In reply to comment #14) > (In reply to comment #9) > > (In reply to comment #8) > > > Created an attachment (id=209043) [details] [details] [details] > > > Patch > > > > I can take it for Qt port, If nobody is involved :) > > Guys, Qt side patch lives in https://gist.github.com/ararunprasad/6271826. Otherwise it doesn't build at all.
I missed to attach the build log: Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp:96:87: error: invalid static_cast from type 'WebCore::FrameLoaderClient' to type 'WebCore::FrameLoaderClientQt*'
(In reply to comment #19) > (In reply to comment #14) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > Created an attachment (id=209043) [details] [details] [details] [details] > > > > Patch > > > > > > I can take it for Qt port, If nobody is involved :) > > > > Guys, Qt side patch lives in https://gist.github.com/ararunprasad/6271826. > > Otherwise it doesn't build at all. A Qt person needs to upload a patch through our interface (thereby asserting that we are allowed to use it).
cc Qt developers
(In reply to comment #18) > (In reply to comment #14) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > Created an attachment (id=209043) [details] [details] [details] [details] > > > > Patch > > > > > > I can take it for Qt port, If nobody is involved :) > > > > Guys, Qt side patch lives in https://gist.github.com/ararunprasad/6271826. > > Could you land that change? I'm not sure I can do it since it hasn't been through the patch upload license agreement here on bugzilla. I thought of uploading after getting green flag from Qt guys :) Sorry for the License, it has same license as QWebFrameAdapter.cpp.
Created attachment 209717 [details] Qt-Patch-For-119964.patch Qt Side Patch (ChangeLog is missing, will upload a proper one soon)
(In reply to comment #24) > Created an attachment (id=209717) [details] > Qt-Patch-For-119964.patch > > Qt Side Patch (ChangeLog is missing, will upload a proper one soon) I got an error with the latest patch: WebKit/Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp:95:39: error: no match for 'operator=' in '((QWebFrameData*)this)->QWebFrameData::frame = parentPage->WebCore::Page::mainFrame()'
Comment on attachment 209717 [details] Qt-Patch-For-119964.patch r-, because it doesn't build. Otherwise it would be great to have a new bug report on this regression instead of fixing something in a closed bug.
(In reply to comment #25) > (In reply to comment #24) > > Created an attachment (id=209717) [details] [details] > > Qt-Patch-For-119964.patch > > > > Qt Side Patch (ChangeLog is missing, will upload a proper one soon) > > I got an error with the latest patch: > > WebKit/Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp:95:39: error: no match for 'operator=' in '((QWebFrameData*)this)->QWebFrameData::frame = parentPage->WebCore::Page::mainFrame()' It was against some old revisions, now mainFrame() returns Frame& it seems. @ line #95 change to, frame = &parentPage->mainFrame();
EFL WK1 is still broken because of this change. before: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/1529 after: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/1480 Qt already fixed in https://bugs.webkit.org/show_bug.cgi?id=120349
(In reply to comment #28) > EFL WK1 is still broken because of this change. > > before: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/1529 > after: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/1480 > > Qt already fixed in https://bugs.webkit.org/show_bug.cgi?id=120349 I created bug for EFL WK1. https://bugs.webkit.org/show_bug.cgi?id=120360