Summary: | WebCore: Let Page create the main Frame. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Andreas Kling <kling> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abecsi, abrhm, allan.jensen, andersca, ararunprasad, arurajku, bfulgham, commit-queue, darin, d-r, eflews.bot, fmalita, gtk-ews, gustavo, gyuyoung.kim, gyuyoung.kim, hausmann, japhet, jturcotte, kadam, kling, koivisto, mrobinson, nick.diego, ossy, pdr, rakuco, ryuan.choi, schenney, sergio, webkit-ews, xan.lopez, zan, zarvai | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 120092 | ||||||||||||||||
Bug Blocks: | 120323, 120349, 120360 | ||||||||||||||||
Attachments: |
|
Description
Andreas Kling
2013-08-17 16:36:33 PDT
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 |