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.
(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.
Created attachment 165794 [details] Patch
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" ?
(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.
Created attachment 165999 [details] Patch
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, ?
Created attachment 166151 [details] Patch
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?
(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.
Created attachment 166169 [details] Patch
Comment on attachment 166169 [details] Patch Clearing flags on attachment: 166169 Committed r129863: <http://trac.webkit.org/changeset/129863>
All reviewed patches have been landed. Closing bug.
Reverted r129863 for reason: Broken debug WK2 layout test Committed r129872: <http://trac.webkit.org/changeset/129872>
Created attachment 168674 [details] Patch
(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 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.
(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 ?
(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 :)
(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.
Created attachment 168681 [details] Patch
Comment on attachment 168681 [details] Patch Clearing flags on attachment: 168681 Committed r131349: <http://trac.webkit.org/changeset/131349>
Reverted r131349 for reason: Revert Committed r131412: <http://trac.webkit.org/changeset/131412>
Created attachment 169878 [details] Patch
(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 on attachment 169878 [details] Patch Land after checking API test and layout test both on release and debug.
Eunmi, is this patch valid now ? Could you check this ?
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).
Created attachment 186592 [details] Rebased patch for landing
(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.
This needs to be signed off by a WK2 owner
Aaaaargh.
Please remove my name in reviewer field. WebKit2 owners don't want that WK2 patch is landed by other reviewer.
Created attachment 186639 [details] Up for review again...
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 on attachment 186639 [details] Up for review again... r=me as Benjamin signed off, but let's fix the spelling before landing.
Committed r142087: <http://trac.webkit.org/changeset/142087>
(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.