Bug 97173 – [EFL][WK2] Refactoring initialization and shutdown codes of EFL libraries.

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

[Eunmi Lee]

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.

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 165794 [details]
Patch

[Chris Dumez]

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" ?

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 165999 [details]
Patch

[Gyuyoung Kim]

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, ?

[Eunmi Lee]

Created attachment 166151 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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?

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 166169 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166169 [details]
Patch

Clearing flags on attachment: 166169

Committed r129863: <http://trac.webkit.org/changeset/129863>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

Reverted r129863 for reason:

Broken debug WK2 layout test

Committed r129872: <http://trac.webkit.org/changeset/129872>

[Eunmi Lee]

Created attachment 168674 [details]
Patch

[Eunmi Lee]

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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

[Eunmi Lee]

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

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 168681 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168681 [details]
Patch

Clearing flags on attachment: 168681

Committed r131349: <http://trac.webkit.org/changeset/131349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

Reverted r131349 for reason:

Revert

Committed r131412: <http://trac.webkit.org/changeset/131412>

[Eunmi Lee]

Created attachment 169878 [details]
Patch

[Eunmi Lee]

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

I've re-based and checked patch after Bug 99681 was resolved.

[Gyuyoung Kim]

Comment on attachment 169878 [details]
Patch

Land after checking API test and layout test both on release and debug.

[Gyuyoung Kim]

Eunmi, is this patch valid now ? Could you check this ?

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 186592 [details]
Rebased patch for landing

[Raphael Kubo da Costa (:rakuco)]

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

[Kenneth Rohde Christiansen]

This needs to be signed off by a WK2 owner

[Raphael Kubo da Costa (:rakuco)]

Aaaaargh.

[Gyuyoung Kim]

Please remove my name in reviewer field. WebKit2 owners don't want that WK2 patch is landed by other reviewer.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 186639 [details]
Up for review again...

[Benjamin Poulain]

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?

[Kenneth Rohde Christiansen]

Comment on attachment 186639 [details]
Up for review again...

r=me as Benjamin signed off, but let's fix the spelling before landing.

[Raphael Kubo da Costa (:rakuco)]

Committed r142087: <http://trac.webkit.org/changeset/142087>

[Raphael Kubo da Costa (:rakuco)]

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