Bug 120360 - [EFL] Let Page create the main Frame
Summary: [EFL] Let Page create the main Frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 119964
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-27 06:58 PDT by Ryuan Choi
Modified: 2013-08-28 17:30 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.49 KB, patch)
2013-08-27 07:24 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (454.18 KB, application/zip)
2013-08-27 08:04 PDT, Build Bot
no flags Details
Follow kubo's comment (14.98 KB, patch)
2013-08-27 17:16 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (623.59 KB, application/zip)
2013-08-27 18:01 PDT, Build Bot
no flags Details
Patch (15.02 KB, patch)
2013-08-27 18:39 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2013-08-27 06:58:46 PDT
All layout tests an EWebLauncher was crashed because page create main frame since r154616.

Patch will be added.
Comment 1 Ryuan Choi 2013-08-27 07:24:27 PDT
Created attachment 209767 [details]
Patch
Comment 2 Build Bot 2013-08-27 08:04:29 PDT
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
Comment 3 Build Bot 2013-08-27 08:04:32 PDT
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
Comment 4 Raphael Kubo da Costa (:rakuco) 2013-08-27 10:15:00 PDT
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?
Comment 5 Raphael Kubo da Costa (:rakuco) 2013-08-27 13:57:33 PDT
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.
Comment 6 Ryuan Choi 2013-08-27 17:16:12 PDT
Created attachment 209823 [details]
Follow kubo's comment
Comment 7 Ryuan Choi 2013-08-27 17:19:02 PDT
(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().
Comment 8 Build Bot 2013-08-27 18:00:59 PDT
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
Comment 9 Build Bot 2013-08-27 18:01:03 PDT
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
Comment 10 Gyuyoung Kim 2013-08-27 18:17:57 PDT
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.
Comment 11 Ryuan Choi 2013-08-27 18:39:29 PDT
Created attachment 209833 [details]
Patch
Comment 12 Ryuan Choi 2013-08-27 18:42:24 PDT
(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.
Comment 13 Gyuyoung Kim 2013-08-28 17:06:54 PDT
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.
Comment 14 WebKit Commit Bot 2013-08-28 17:30:18 PDT
Comment on attachment 209833 [details]
Patch

Clearing flags on attachment: 209833

Committed r154798: <http://trac.webkit.org/changeset/154798>
Comment 15 WebKit Commit Bot 2013-08-28 17:30:22 PDT
All reviewed patches have been landed.  Closing bug.