Bug 97173

Summary: [EFL][WK2] Refactoring initialization and shutdown codes of EFL libraries.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, gyuyoung.kim, kangil.han, kenneth, kling, laszlo.gombos, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99681    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Rebased patch for landing
none
Up for review again... kenneth: review+

Description Eunmi Lee 2012-09-19 22:57:59 PDT
There are duplicated initializing codes in the ewk_main.cpp and RunLoopEfl.cpp.
We have WebContext in the WebKit2, so we can initialize ecore and evas through RunLoopEfl when creating WebContext.
We don't have to add ewk_{init, shutdown}() like WebKit1 and let applications call them.
Comment 1 Eunmi Lee 2012-09-26 06:50:04 PDT
(In reply to comment #0)
> There are duplicated initializing codes in the ewk_main.cpp and RunLoopEfl.cpp.
> We have WebContext in the WebKit2, so we can initialize ecore and evas through RunLoopEfl when creating WebContext.
> We don't have to add ewk_{init, shutdown}() like WebKit1 and let applications call them.

I will make patch to move initialization and shutdown codes of EFL libraries to the ewk_main.cpp for ui process and WebProcessMainEfl.cpp for web-engine process.
RunLoopEfl.cpp will not be used to initialize and shutdown the EFL libraries anymore.

Additionally,
ewk_{init,shutdown} will be called when ewk_context is created and deleted,
so application don't have to call ewk_{init,shutdown} and they will be changed to internal function from API.
Comment 2 Eunmi Lee 2012-09-26 07:10:33 PDT
Created attachment 165794 [details]
Patch
Comment 3 Chris Dumez 2012-09-27 03:31:38 PDT
Comment on attachment 165794 [details]
Patch

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

Looks sane.

> Source/WebCore/ChangeLog:9
> +        RunLooEfl.cpp. Initialization and shutdown will be done in the

"RunLoopEfl"

> Source/WebCore/ChangeLog:10
> +        ewk_main.cpp for ui process and WebProcessMainEfl.cpp for web-engine

"Web process" ?
Comment 4 Eunmi Lee 2012-09-27 07:43:55 PDT
(In reply to comment #3)
> (From update of attachment 165794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165794&action=review
> 
> Looks sane.
> 
> > Source/WebCore/ChangeLog:9
> > +        RunLooEfl.cpp. Initialization and shutdown will be done in the
> 
> "RunLoopEfl"
> 
> > Source/WebCore/ChangeLog:10
> > +        ewk_main.cpp for ui process and WebProcessMainEfl.cpp for web-engine
> 
> "Web process" ?

Thanks for review, I will fix them.
Comment 5 Eunmi Lee 2012-09-27 07:49:26 PDT
Created attachment 165999 [details]
Patch
Comment 6 Gyuyoung Kim 2012-09-27 23:17:07 PDT
Comment on attachment 165999 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:42
> + * @internal

It would be good to add a new line @internal below,

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:107
> + * @internal

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:109
> + * If the reference count reaches 0 WebKit's instance is destroyed.

reaches 0 -> reaches 0, ?
Comment 7 Eunmi Lee 2012-09-27 23:38:13 PDT
Created attachment 166151 [details]
Patch
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-09-28 00:55:36 PDT
Comment on attachment 166151 [details]
Patch

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

Looks mostly fine, as long as you are sure calling code that does not require a context before creating one does not cause any problems.

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:77
> +    if (!edje_init()) {

Don't you need to shut Edje down as well now?
Comment 9 Eunmi Lee 2012-09-28 01:05:14 PDT
(In reply to comment #8)
> (From update of attachment 166151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166151&action=review
> 
> Looks mostly fine, as long as you are sure calling code that does not require a context before creating one does not cause any problems.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:77
> > +    if (!edje_init()) {
> 
> Don't you need to shut Edje down as well now?

Thanks for your comment. it is my mistake. edje_shutdown() should be there.
Comment 10 Eunmi Lee 2012-09-28 01:20:31 PDT
Created attachment 166169 [details]
Patch
Comment 11 WebKit Review Bot 2012-09-28 01:56:40 PDT
Comment on attachment 166169 [details]
Patch

Clearing flags on attachment: 166169

Committed r129863: <http://trac.webkit.org/changeset/129863>
Comment 12 WebKit Review Bot 2012-09-28 01:56:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Gyuyoung Kim 2012-09-28 03:49:16 PDT
Reverted r129863 for reason:

Broken debug WK2 layout test

Committed r129872: <http://trac.webkit.org/changeset/129872>
Comment 14 Eunmi Lee 2012-10-15 04:16:25 PDT
Created attachment 168674 [details]
Patch
Comment 15 Eunmi Lee 2012-10-15 04:19:02 PDT
(In reply to comment #14)
> Created an attachment (id=168674) [details]
> Patch

The ecore_pipe is created in the Source/WTF/wtf/efl/MainThreadEfl.cpp's initializeMainThreadPlatform(),
initializeMainThreadPlatform() is called when we use the WKString and the WKString is used in the WebKitTestRunner before creating WebContext.
so, I could not remove the ewk_{init, shutdown} API and I've made new patch without that.
Comment 16 Gyuyoung Kim 2012-10-15 04:21:22 PDT
Comment on attachment 168674 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Missing patch description.
Comment 17 Gyuyoung Kim 2012-10-15 04:27:24 PDT
(In reply to comment #15)

> initializeMainThreadPlatform() is called when we use the WKString and the WKString is used in the WebKitTestRunner before creating WebContext.

Isn't this bug ? Or, can't we change this according to your patch ?
Comment 18 Eunmi Lee 2012-10-15 04:46:42 PDT
(In reply to comment #16)
> (From update of attachment 168674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168674&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> Missing patch description.

Oops, I have to add that :)
Comment 19 Eunmi Lee 2012-10-15 04:56:49 PDT
(In reply to comment #17)
> (In reply to comment #15)
> 
> > initializeMainThreadPlatform() is called when we use the WKString and the WKString is used in the WebKitTestRunner before creating WebContext.
> 
> Isn't this bug ? Or, can't we change this according to your patch ?

I think that is not a bug and it is normal operation of WK.
so, I've just done refactoring for init, shutdown functions in this bug.
Comment 20 Eunmi Lee 2012-10-15 04:58:43 PDT
Created attachment 168681 [details]
Patch
Comment 21 WebKit Review Bot 2012-10-15 13:36:26 PDT
Comment on attachment 168681 [details]
Patch

Clearing flags on attachment: 168681

Committed r131349: <http://trac.webkit.org/changeset/131349>
Comment 22 WebKit Review Bot 2012-10-15 13:36:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Gyuyoung Kim 2012-10-15 22:17:05 PDT
Reverted r131349 for reason:

Revert

Committed r131412: <http://trac.webkit.org/changeset/131412>
Comment 24 Eunmi Lee 2012-10-22 04:59:58 PDT
Created attachment 169878 [details]
Patch
Comment 25 Eunmi Lee 2012-10-22 05:01:54 PDT
(In reply to comment #24)
> Created an attachment (id=169878) [details]
> Patch

I've re-based and checked patch after Bug 99681 was resolved.
Comment 26 Gyuyoung Kim 2012-10-29 09:09:55 PDT
Comment on attachment 169878 [details]
Patch

Land after checking API test and layout test both on release and debug.
Comment 27 Gyuyoung Kim 2013-01-06 17:16:02 PST
Eunmi, is this patch valid now ? Could you check this ?
Comment 28 Raphael Kubo da Costa (:rakuco) 2013-01-28 07:28:42 PST
Bump. It'd be nice to get this in as soon as possible; right now we're not properly shutting things down in WK2 since RunLoop's destructor is not called when we think it should be (it's a global static).
Comment 29 Raphael Kubo da Costa (:rakuco) 2013-02-05 03:10:07 PST
Created attachment 186592 [details]
Rebased patch for landing
Comment 30 Raphael Kubo da Costa (:rakuco) 2013-02-05 03:11:02 PST
(In reply to comment #29)
> Created an attachment (id=186592) [details]
> Rebased patch for landing

EunMi said it was OK for me to rebase the patch, so I've done that and cleaned up the shutdown in ewk_main a little. I'll land and watch the bots after EWS goes green.
Comment 31 Kenneth Rohde Christiansen 2013-02-05 03:11:36 PST
This needs to be signed off by a WK2 owner
Comment 32 Raphael Kubo da Costa (:rakuco) 2013-02-05 03:29:51 PST
Aaaaargh.
Comment 33 Gyuyoung Kim 2013-02-05 03:52:03 PST
Please remove my name in reviewer field. WebKit2 owners don't want that WK2 patch is landed by other reviewer.
Comment 34 Raphael Kubo da Costa (:rakuco) 2013-02-05 09:10:13 PST
Created attachment 186639 [details]
Up for review again...
Comment 35 Benjamin Poulain 2013-02-06 14:53:04 PST
Comment on attachment 186639 [details]
Up for review again...

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

No way to avoid the duplicated code?

I sign off on this for WebKit2. Feel free to land it after a proper review of the EFL bits.

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:89
> +        CRITICAL("could not init edje.");

could -> Could?
Comment 36 Kenneth Rohde Christiansen 2013-02-06 15:36:22 PST
Comment on attachment 186639 [details]
Up for review again...

r=me as Benjamin signed off, but let's fix the spelling before landing.
Comment 37 Raphael Kubo da Costa (:rakuco) 2013-02-07 03:04:02 PST
Committed r142087: <http://trac.webkit.org/changeset/142087>
Comment 38 Raphael Kubo da Costa (:rakuco) 2013-02-07 04:29:51 PST
(In reply to comment #35)
> (From update of attachment 186639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186639&action=review
> 
> No way to avoid the duplicated code?

Probably, but I'd rather do it separately; I chose to modify the original patch as little as possible here.