Bug 120360 – [EFL] Let Page create the main Frame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

All layout tests an EWebLauncher was crashed because page create main frame since r154616.

Patch will be added.

[Ryuan Choi]

Created attachment 209767 [details]
Patch

[Build Bot]

Comment on attachment 209767 [details]
Patch

Attachment 209767 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1579316

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209774 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Raphael Kubo da Costa (:rakuco)]

I didn't have time to look at the whole patch, but ewk_frame_coreFrame_set looks like a weird function name. Why not add setCoreFrame to EWKPrivate (just like we already have coreFrame() there) and call it instead?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 209767 [details]
Patch

Overall, I found the new location of the calls more confusing than before. Can't we get our behavior more inline with GTK+'s? It basically has most of the code present in ewk_view_frame_create() in FrameLoaderClientGtk::createFrame() itself.

[Ryuan Choi]

Created attachment 209823 [details]
Follow kubo's comment

[Ryuan Choi]

(In reply to comment #4)
> I didn't have time to look at the whole patch, but ewk_frame_coreFrame_set looks like a weird function name. Why not add setCoreFrame to EWKPrivate (just like we already have coreFrame() there) and call it instead?

Fixed.

(In reply to comment #5)
> (From update of attachment 209767 [details])
> Overall, I found the new location of the calls more confusing than before. Can't we get our behavior more inline with GTK+'s? It basically has most of the code present in ewk_view_frame_create() in FrameLoaderClientGtk::createFrame() itself.

Thank you for good suggestion. I thought that ewk_view_frame_create is bad way.
I moved most of the code to FrameLoaderClientEfl::createFrame().

[Build Bot]

Comment on attachment 209823 [details]
Follow kubo's comment

Attachment 209823 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1624087

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209828 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Gyuyoung Kim]

Comment on attachment 209823 [details]
Follow kubo's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=209823&action=review

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Crash after r154616

It looks this patch has meaningful change current title is unable to represent. It would move this title to description. Then, please change title more meaningful thing.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:380
> +    if (!coreSubFrame->page()) {

EWKPrivate::coreFrame may return 0. Don't you need to check if coreSubFrame is null ?


WebCore::Frame* coreFrame(const Evas_Object* ewkFrame)
{
    EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, 0);
    return smartData->frame;
}

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:381
> +        evas_object_del(subFrame);

It would be nice to add a message.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:391
> +        evas_object_del(subFrame);

ditto.

[Ryuan Choi]

Created attachment 209833 [details]
Patch

[Ryuan Choi]

(In reply to comment #10)
> (From update of attachment 209823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209823&action=review
> 
> > Source/WebKit/efl/ChangeLog:3
> > +        [EFL] Crash after r154616
> 
> It looks this patch has meaningful change current title is unable to represent. It would move this title to description. Then, please change title more meaningful thing.
> 
OK, I changed like other ports did.

> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:380
> > +    if (!coreSubFrame->page()) {
> 
> EWKPrivate::coreFrame may return 0. Don't you need to check if coreSubFrame is null ?
> 
> 
> WebCore::Frame* coreFrame(const Evas_Object* ewkFrame)
> {
>     EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, smartData, 0);
>     return smartData->frame;
> }
> 

Added ASSERT.
I think that it can't be NULL logically.

> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:381
> > +        evas_object_del(subFrame);
> 
> It would be nice to add a message.
> 
> > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:391
> > +        evas_object_del(subFrame);
> 
> ditto.

I think that this is normal case.
So we don't need to print error message.

[Gyuyoung Kim]

Comment on attachment 209833 [details]
Patch

Darin set r+ instead of me. It looks there is no critical problem now. I land this patch in order to fix EFL WK1 buildbot as soon as possible.

[WebKit Commit Bot]

Comment on attachment 209833 [details]
Patch

Clearing flags on attachment: 209833

Committed r154798: <http://trac.webkit.org/changeset/154798>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.