Bug 119964 - WebCore: Let Page create the main Frame.
Summary: WebCore: Let Page create the main Frame.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 120092
Blocks: 120323 120349 120360
  Show dependency treegraph
 
Reported: 2013-08-17 16:36 PDT by Andreas Kling
Modified: 2013-08-28 05:20 PDT (History)
34 users (show)

See Also:


Attachments
First stab for EWS (14.99 KB, patch)
2013-08-17 16:42 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
First stab for EWS + SVGImage bugfix (15.26 KB, patch)
2013-08-17 16:46 PDT, Andreas Kling
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Second stab for EWS (15.61 KB, patch)
2013-08-18 04:37 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (21.48 KB, patch)
2013-08-18 15:35 PDT, Andreas Kling
andersca: review+
Details | Formatted Diff | Diff
GTK-WK1 changes (3.22 KB, patch)
2013-08-19 09:34 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Qt-Patch-For-119964.patch (1.66 KB, patch)
2013-08-26 23:43 PDT, Arunprasad Rajkumar
ossy: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-08-17 16:36:33 PDT
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.
Comment 1 Andreas Kling 2013-08-17 16:42:35 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.
Comment 2 Andreas Kling 2013-08-17 16:46:05 PDT
Created attachment 209016 [details]
First stab for EWS + SVGImage bugfix
Comment 3 Early Warning System Bot 2013-08-17 16:56:59 PDT
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 4 Early Warning System Bot 2013-08-17 16:59:33 PDT
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 5 kov's GTK+ EWS bot 2013-08-17 17:02:34 PDT
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 6 EFL EWS Bot 2013-08-17 17:21:40 PDT
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
Comment 7 Andreas Kling 2013-08-18 04:37:10 PDT
Created attachment 209025 [details]
Second stab for EWS

Removed the extra Page constructor argument and added a PageClients::loaderClientForMainFrame pointer instead.
Comment 8 Andreas Kling 2013-08-18 15:35:51 PDT
Created attachment 209043 [details]
Patch
Comment 9 Arunprasad Rajkumar 2013-08-18 23:02:46 PDT
(In reply to comment #8)
> Created an attachment (id=209043) [details]
> Patch

I can take it for Qt port, If nobody is involved :)
Comment 10 Brent Fulgham 2013-08-19 08:39:59 PDT
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.
Comment 11 Zan Dobersek 2013-08-19 09:34:03 PDT
Created attachment 209093 [details]
GTK-WK1 changes

Here are the changes required for the GTK port.
Thanks for waiting for these.
Comment 12 Gustavo Noronha (kov) 2013-08-19 09:55:51 PDT
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 13 Gustavo Noronha (kov) 2013-08-19 10:20:58 PDT
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.
Comment 14 Arunprasad Rajkumar 2013-08-19 10:36:02 PDT
(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.
Comment 15 Brent Fulgham 2013-08-20 14:28:02 PDT
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.
Comment 16 Brent Fulgham 2013-08-20 14:51:12 PDT
I added a blocking bug (Bug 120092) to refactor the Windows code to make these changes easier.
Comment 17 Andreas Kling 2013-08-26 10:53:44 PDT
Committed r154616: <http://trac.webkit.org/changeset/154616>
Comment 18 Andreas Kling 2013-08-26 14:33:59 PDT
(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.
Comment 19 Csaba Osztrogonác 2013-08-26 15:12:02 PDT
(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.
Comment 20 Csaba Osztrogonác 2013-08-26 15:12:28 PDT
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*'
Comment 21 Brent Fulgham 2013-08-26 15:13:51 PDT
(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).
Comment 22 Csaba Osztrogonác 2013-08-26 15:25:06 PDT
cc Qt developers
Comment 23 Arunprasad Rajkumar 2013-08-26 23:41:59 PDT
(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.
Comment 24 Arunprasad Rajkumar 2013-08-26 23:43:50 PDT
Created attachment 209717 [details]
Qt-Patch-For-119964.patch

Qt Side Patch (ChangeLog is missing, will upload a proper one soon)
Comment 25 Zoltan Arvai 2013-08-27 02:09:29 PDT
(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 26 Csaba Osztrogonác 2013-08-27 02:12:08 PDT
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.
Comment 27 Arunprasad Rajkumar 2013-08-27 02:14:50 PDT
(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();