Bug 119964 – WebCore: Let Page create the main Frame.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andreas Kling]

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.

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 209016 [details]
First stab for EWS + SVGImage bugfix

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[kov's GTK+ EWS bot]

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

[EFL EWS Bot]

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

[Andreas Kling]

Created attachment 209025 [details]
Second stab for EWS

Removed the extra Page constructor argument and added a PageClients::loaderClientForMainFrame pointer instead.

[Andreas Kling]

Created attachment 209043 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #8)
> Created an attachment (id=209043) [details]
> Patch

I can take it for Qt port, If nobody is involved :)

[Brent Fulgham]

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.

[Zan Dobersek]

Created attachment 209093 [details]
GTK-WK1 changes

Here are the changes required for the GTK port.
Thanks for waiting for these.

[Gustavo Noronha (kov)]

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).

[Gustavo Noronha (kov)]

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.

[Arunprasad Rajkumar]

(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.

[Brent Fulgham]

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.

[Brent Fulgham]

I added a blocking bug (Bug 120092) to refactor the Windows code to make these changes easier.

[Andreas Kling]

Committed r154616: <http://trac.webkit.org/changeset/154616>

[Andreas Kling]

(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.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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*'

[Brent Fulgham]

(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).

[Csaba Osztrogonác]

cc Qt developers

[Arunprasad Rajkumar]

(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.

[Arunprasad Rajkumar]

Created attachment 209717 [details]
Qt-Patch-For-119964.patch

Qt Side Patch (ChangeLog is missing, will upload a proper one soon)

[Zoltan Arvai]

(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()'

[Csaba Osztrogonác]

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.

[Arunprasad Rajkumar]

(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();

[Csaba Osztrogonác]

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

[Ryuan Choi]

(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