Bug 89186

Summary:

[EFL][WK2] Add APIs to create, delete and get ewk_context.

Product:

WebKit

Reporter:

Eunmi Lee <enmi.lee>

Component:

WebKit EFL

Assignee:

Eunmi Lee <enmi.lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dglazkov, gyuyoung.kim, gyuyoung.kim, js45.yang, kenneth, lucas.de.marchi, rakuco, ryuan.choi, sw0524.lee, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

61838

Attachments:

Description	Flags
patch for ewk_context APIs	none
patch for ewk_context APIs	none
patch for ewk_context APIs	none
patch for ewk_context APIs	webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-07	none
patch for ewk_context APIs	none
patch for ewk_context APIs	none
patch for ewk_context API.	none
patch for ewk_context APIs	none
[WIP] Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Eunmi Lee

Reported 2012-06-15 01:25:23 PDT

Provide APIs to create ewk_context with or without injected bundle path. Additionally, the ewk_view can be created with ewk_context and we can get ewk_context from ewk_view.

Attachments
patch for ewk_context APIs (5.51 KB, patch) 2012-06-15 23:20 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context APIs (5.55 KB, patch) 2012-06-15 23:23 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context APIs (6.07 KB, patch) 2012-06-28 03:08 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context APIs (8.35 KB, patch) 2012-07-13 00:11 PDT, Eunmi Lee	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-07 (329.67 KB, application/zip) 2012-07-13 00:44 PDT, WebKit Review Bot	no flags	Details
patch for ewk_context APIs (7.98 KB, patch) 2012-07-13 01:26 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context APIs (7.98 KB, patch) 2012-07-13 03:06 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context API. (8.16 KB, patch) 2012-07-14 09:30 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
patch for ewk_context APIs (8.38 KB, patch) 2012-07-15 22:39 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
[WIP] Patch (9.16 KB, patch) 2012-09-07 00:00 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (13.78 KB, patch) 2012-09-12 22:25 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (16.05 KB, patch) 2012-09-13 00:12 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (22.01 KB, patch) 2012-09-13 00:52 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (22.75 KB, patch) 2012-09-13 05:46 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (23.08 KB, patch) 2012-09-18 01:29 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (23.73 KB, patch) 2012-09-19 04:35 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (23.61 KB, patch) 2012-09-19 19:41 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Patch (22.83 KB, patch) 2012-09-20 06:00 PDT, Eunmi Lee	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Eunmi Lee

Comment 1 2012-06-15 23:20:29 PDT

Created attachment 147952 [details] patch for ewk_context APIs

Eunmi Lee

Comment 2 2012-06-15 23:23:13 PDT

Created attachment 147953 [details] patch for ewk_context APIs Add "Reviewed by NOBODY (OOPS!)." to the Changelog.

Ryuan Choi

Comment 3 2012-06-24 22:41:22 PDT

Comment on attachment 147953 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=147953&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:70 > +void ewk_context_delete(Ewk_Context* ewkContext) > +{ > + EINA_SAFETY_ON_NULL_RETURN(ewkContext); > + EINA_SAFETY_ON_TRUE_RETURN(ewkContext == ewk_context_default_get()); > + > + delete ewkContext; > +} Can we delete context which have live ewk_view?

Chris Dumez

Comment 4 2012-06-25 05:01:29 PDT

Comment on attachment 147953 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=147953&action=review I looked at GTK port. They don't provide constructors for the context. However, they provide a way to get the default context. Should we consider adding a way to get the default context as well? > Source/WebKit2/UIProcess/API/efl/ewk_context.h:75 > +EAPI void ewk_context_delete(Ewk_Context *context); We usually use *_free() not *_delete().

Eunmi Lee

Comment 5 2012-06-27 02:22:21 PDT

(In reply to comment #3) > (From update of attachment 147953 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147953&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:70 > > +void ewk_context_delete(Ewk_Context* ewkContext) > > +{ > > + EINA_SAFETY_ON_NULL_RETURN(ewkContext); > > + EINA_SAFETY_ON_TRUE_RETURN(ewkContext == ewk_context_default_get()); > > + > > + delete ewkContext; > > +} > > Can we delete context which have live ewk_view? Good question :) I think application should not call ewk_context_delete() for context which have live ewk_view. so, I will add codes to check whether context has ewk_view or not.

Eunmi Lee

Comment 6 2012-06-27 02:23:52 PDT

(In reply to comment #4) > (From update of attachment 147953 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147953&action=review > > I looked at GTK port. They don't provide constructors for the context. However, they provide a way to get the default context. Should we consider adding a way to get the default context as well? > Yes, the application can use default context or context which is created by them. > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:75 > > +EAPI void ewk_context_delete(Ewk_Context *context); > > We usually use *_free() not *_delete(). OK, I will change to *_delete().

Eunmi Lee

Comment 7 2012-06-28 03:08:25 PDT

Created attachment 149910 [details] patch for ewk_context APIs

Eunmi Lee

Comment 8 2012-06-28 03:10:03 PDT

(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 147953 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147953&action=review > > > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:70 > > > +void ewk_context_delete(Ewk_Context* ewkContext) > > > +{ > > > + EINA_SAFETY_ON_NULL_RETURN(ewkContext); > > > + EINA_SAFETY_ON_TRUE_RETURN(ewkContext == ewk_context_default_get()); > > > + > > > + delete ewkContext; > > > +} > > > > Can we delete context which have live ewk_view? > > Good question :) > I think application should not call ewk_context_delete() for context which have live ewk_view. > so, I will add codes to check whether context has ewk_view or not. Ryuan, I added code to check WKContext is referred by someome using refCount(). Please review new patch again :)

Ryuan Choi

Comment 9 2012-06-28 23:10:50 PDT

(In reply to comment #8) > (In reply to comment #5) > > (In reply to comment #3) > > > Can we delete context which have live ewk_view? > > > > Good question :) > > I think application should not call ewk_context_delete() for context which have live ewk_view. > > so, I will add codes to check whether context has ewk_view or not. > > Ryuan, > > I added code to check WKContext is referred by someome using refCount(). > Please review new patch again :) Looks fine to me.

Chris Dumez

Comment 10 2012-07-12 01:04:54 PDT

Comment on attachment 149910 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=149910&action=review This patch probably needs updating because the Ewk Context now contains the BatteryProvider. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:35 > + _Ewk_Context(WKRetainPtr<WKContextRef> contextRef) Why this change? > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:54 > + return new Ewk_Context(adoptWK(WKContextCreate())); return new Ewk_Context(WKContextCreate()); ? > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:65 > +Eina_Bool ewk_context_free(Ewk_Context* ewkContext) Our *_free() functions usually don't return a boolean I believe. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:69 > + if (toImpl(ewkContext->context.get())->refCount() > 1) This check does not look right. You need to free the ewkContext no matter what, the WKContext ref count does not matter here. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:524 > + return priv->context; Blank line before return?

Eunmi Lee

Comment 11 2012-07-12 20:19:24 PDT

(In reply to comment #10) > (From update of attachment 149910 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149910&action=review > > This patch probably needs updating because the Ewk Context now contains the BatteryProvider. OK, I will rebase the patch, thanks for your information. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:35 > > + _Ewk_Context(WKRetainPtr<WKContextRef> contextRef) > > Why this change? > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:54 > > + return new Ewk_Context(adoptWK(WKContextCreate())); > > return new Ewk_Context(WKContextCreate()); ? We have to use adoptWK() here because WKContextCreate() returns leakPtr(), and we have to get that using WKRetainPtr in the line 35 to prevent memory leak. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:65 > > +Eina_Bool ewk_context_free(Ewk_Context* ewkContext) > > Our *_free() functions usually don't return a boolean I believe. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:69 > > + if (toImpl(ewkContext->context.get())->refCount() > 1) > > This check does not look right. You need to free the ewkContext no matter what, the WKContext ref count does not matter here. > Actually, there is a matter for ewk_view when we free ewk_context directly even though there is no matter for WKContext. Suppose that there is a ewk_view which has a reference of ewk_context, it will have the freed memory's reference if we free the ewk_context whenever ewk_context_free() is called. So, I want to check that there is a reference of ewk_context or not by using WKContext's refCount because ewk_context and WKContext have the same reference count. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:524 > > + return priv->context; > > Blank line before return? OK, I will add blank line when I rebase the patch :)

Chris Dumez

Comment 12 2012-07-12 22:30:40 PDT

Comment on attachment 149910 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=149910&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:35 >>> + _Ewk_Context(WKRetainPtr<WKContextRef> contextRef) >> >> Why this change? > > We have to use adoptWK() here because WKContextCreate() returns leakPtr(), > and we have to get that using WKRetainPtr in the line 35 to prevent memory leak. It is already assigned to context which is a WKRetainPtr<WKContextRef>, so I did not think there was a leak here. Are you sure? >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:65 >>> +Eina_Bool ewk_context_free(Ewk_Context* ewkContext) >> >> Our *_free() functions usually don't return a boolean I believe. > > Actually, there is a matter for ewk_view when we free ewk_context directly even though there is no matter for WKContext. > Suppose that there is a ewk_view which has a reference of ewk_context, > it will have the freed memory's reference if we free the ewk_context whenever ewk_context_free() is called. > So, I want to check that there is a reference of ewk_context or not by using WKContext's refCount because ewk_context and WKContext have the same reference count. So I would prefer if we had "ewk_context_ref() / ewk_context_unref()" function and if we had a reference counter in the struct, like we do already for several other Ewk types.

Eunmi Lee

Comment 13 2012-07-12 22:55:58 PDT

(In reply to comment #12) > (From update of attachment 149910 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149910&action=review > > >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:35 > >>> + _Ewk_Context(WKRetainPtr<WKContextRef> contextRef) > >> > >> Why this change? > > > > We have to use adoptWK() here because WKContextCreate() returns leakPtr(), > > and we have to get that using WKRetainPtr in the line 35 to prevent memory leak. > > It is already assigned to context which is a WKRetainPtr<WKContextRef>, so I did not think there was a leak here. Are you sure? > I think my sentence caused the confusion. I can explain again. I use the adoptWK() to prevent leak because WKContextCreate() returns leakPtr(), so the leak problem is fixed here. and, I change the _Ewk_Context()'s parameter to WKRetainPtr because adoptWK() returns WKRetainPtr type. but I can use WKContextRef if I return the adoptWK(...).get() istead of adoptWK(...). So, do you want that? > >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:65 > >>> +Eina_Bool ewk_context_free(Ewk_Context* ewkContext) > >> > >> Our *_free() functions usually don't return a boolean I believe. > > > > Actually, there is a matter for ewk_view when we free ewk_context directly even though there is no matter for WKContext. > > Suppose that there is a ewk_view which has a reference of ewk_context, > > it will have the freed memory's reference if we free the ewk_context whenever ewk_context_free() is called. > > So, I want to check that there is a reference of ewk_context or not by using WKContext's refCount because ewk_context and WKContext have the same reference count. > > So I would prefer if we had "ewk_context_ref() / ewk_context_unref()" function and if we had a reference counter in the struct, like we do already for several other Ewk types. Yes, I use the WKContext's refCount for my convenience but I think ewk_context_ref()/unref() is better now. that is more explicit. :)

Chris Dumez

Comment 14 2012-07-12 23:03:43 PDT

Comment on attachment 149910 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=149910&action=review >>>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:35 >>>>> + _Ewk_Context(WKRetainPtr<WKContextRef> contextRef) >>>> >>>> Why this change? >>> >>> We have to use adoptWK() here because WKContextCreate() returns leakPtr(), >>> and we have to get that using WKRetainPtr in the line 35 to prevent memory leak. >> >> It is already assigned to context which is a WKRetainPtr<WKContextRef>, so I did not think there was a leak here. Are you sure? > > I think my sentence caused the confusion. I can explain again. > I use the adoptWK() to prevent leak because WKContextCreate() returns leakPtr(), so the leak problem is fixed here. > and, I change the _Ewk_Context()'s parameter to WKRetainPtr because adoptWK() returns WKRetainPtr type. but I can use WKContextRef if I return the adoptWK(...).get() istead of adoptWK(...). > So, do you want that? No, it is fine as it is. I understand now. Thanks for the explanation. the adoptWK() is indeed needed when you call a *Create() function.

Eunmi Lee

Comment 15 2012-07-13 00:11:38 PDT

Created attachment 152166 [details] patch for ewk_context APIs I add reference count for ewk_context to check that there is any ewk_view which has reference of ewk_context or not when ewk_context is freed.

Chris Dumez

Comment 16 2012-07-13 00:19:21 PDT

Comment on attachment 152166 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=152166&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:41 > + : __ref(0) __ref should be initialized to 1 > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:61 > + ewkContext->__ref--; You never call delete. Should look like: if (--ewkContext->__ref) return; delete ewkContext; > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:68 > + ewkContext->__ref++; Please use pre-incrementation. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:75 > + ewk_context_ref(ewkContext); Should not be called after a new. Not needed if __ref is initialized to 1. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:100 > +Eina_Bool ewk_context_free(Ewk_Context* ewkContext) There should be no _free() function since we have ref() / unref() now. > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:28 > +void ewk_context_unref(Ewk_Context*); I believe ref() / unref() should be public.

WebKit Review Bot

Comment 17 2012-07-13 00:44:35 PDT

Comment on attachment 152166 [details] patch for ewk_context APIs Attachment 152166 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13221544 New failing tests: http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html

WebKit Review Bot

Comment 18 2012-07-13 00:44:40 PDT

Created attachment 152173 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Eunmi Lee

Comment 19 2012-07-13 01:26:00 PDT

Created attachment 152183 [details] patch for ewk_context APIs I modified code for Christophe's comment.

Chris Dumez

Comment 20 2012-07-13 01:30:54 PDT

Comment on attachment 152183 [details] patch for ewk_context APIs Ah, I like it now :) LGTM.

Gyuyoung Kim

Comment 21 2012-07-13 02:29:28 PDT

Comment on attachment 152183 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=152183&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:522 > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, ewkView); When ewkView can't get smart data, why do you return ewkView ? If ewkView can't get smart data, I think this view is something wrong. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:523 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, ewkView); ditto.

Eunmi Lee

Comment 22 2012-07-13 02:47:57 PDT

(In reply to comment #21) > (From update of attachment 152183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152183&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:522 > > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, ewkView); > > When ewkView can't get smart data, why do you return ewkView ? If ewkView can't get smart data, I think this view is something wrong. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:523 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, ewkView); > > ditto. The smart data and priv are already checked in the ewk_view_base_add(). So they should not be null in the line 522, 523. I will change them without returning.

Eunmi Lee

Comment 23 2012-07-13 03:06:41 PDT

Created attachment 152203 [details] patch for ewk_context APIs Fixed for Gyuyoung's comment.

Gyuyoung Kim

Comment 24 2012-07-13 03:09:22 PDT

Comment on attachment 152203 [details] patch for ewk_context APIs LGTM now.

Eunmi Lee

Comment 25 2012-07-14 09:30:11 PDT

Created attachment 152425 [details] patch for ewk_context API. I've added code to initialize context in the Ewk_View_Priv_Data's constructor. I've modified comments to use NULL instead of 0.

Chris Dumez

Comment 26 2012-07-14 09:40:12 PDT

Comment on attachment 152425 [details] patch for ewk_context API. View in context: https://bugs.webkit.org/attachment.cgi?id=152425&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:76 > Ewk_Context* ewkContext = new Ewk_Context(wkContext); Useless assignment now, we could return directly: return Ewk_Context(WKContextGetSharedProcessContext()); > Source/WebKit2/UIProcess/API/efl/ewk_context.h:73 > +EAPI Ewk_Context *ewk_context_new(); I think we use ewk_context_new(void) for public C APIs.

Eunmi Lee

Comment 27 2012-07-15 22:29:03 PDT

(In reply to comment #26) > (From update of attachment 152425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152425&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:76 > > Ewk_Context* ewkContext = new Ewk_Context(wkContext); > > Useless assignment now, we could return directly: > return Ewk_Context(WKContextGetSharedProcessContext()); > Thanks, I will change that. > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:73 > > +EAPI Ewk_Context *ewk_context_new(); > > I think we use ewk_context_new(void) for public C APIs. Yes, right. I will add "void". and, I think it is better to add this rule to the Efl webkit's coding style guide wiki page. I will edit. https://trac.webkit.org/wiki/EFLWebKitCodingStyle

Eunmi Lee

Comment 28 2012-07-15 22:39:17 PDT

Created attachment 152477 [details] patch for ewk_context APIs Modified for Christophe's comment.

Chris Dumez

Comment 29 2012-07-17 11:35:14 PDT

Could you explain why we need this API? What's the problem if we keep using only one default context? The GTK port seems to be doing fine this way. I just want to make sure we think about the use cases before making the API more complex.

Kenneth Rohde Christiansen

Comment 30 2012-07-17 20:09:10 PDT

(In reply to comment #29) > Could you explain why we need this API? What's the problem if we keep using only one default context? The GTK port seems to be doing fine this way. > > I just want to make sure we think about the use cases before making the API more complex. For Qt we kind of decided to not expose the context for now unless we really need to. It is also one of those API's which might change when webkit2 at some point supports multiple processes etc and we don't want to make wrong API decisions that we have to live with.

Eunmi Lee

Comment 31 2012-07-17 20:17:53 PDT

(In reply to comment #30) > (In reply to comment #29) > > Could you explain why we need this API? What's the problem if we keep using only one default context? The GTK port seems to be doing fine this way. > > > > I just want to make sure we think about the use cases before making the API more complex. > > For Qt we kind of decided to not expose the context for now unless we really need to. It is also one of those API's which might change when webkit2 at some point supports multiple processes etc and we don't want to make wrong API decisions that we have to live with. The reason of that I expose the context is that we need context which is created with injected bundle path. We are using WK2 EFL port with injected bundle in the Tizen. So, I need this patch :)

Kenneth Rohde Christiansen

Comment 32 2012-07-17 20:21:03 PDT

(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > Could you explain why we need this API? What's the problem if we keep using only one default context? The GTK port seems to be doing fine this way. > > > > > > I just want to make sure we think about the use cases before making the API more complex. > > > > For Qt we kind of decided to not expose the context for now unless we really need to. It is also one of those API's which might change when webkit2 at some point supports multiple processes etc and we don't want to make wrong API decisions that we have to live with. > > The reason of that I expose the context is that we need context which is created with injected bundle path. > We are using WK2 EFL port with injected bundle in the Tizen. > So, I need this patch :) Qt handles that fine with private API. This is what some also call SPI (system programming interface). The inject bundle can be used for a lot of things. For Qt we took out some use-cases and made nice API's for those. Such as evaluateJavaScript, postMessage/receiveMessage (UI <-> Web process IPC). Also I can suggest using a local web server for exposing additional API's.

Chris Dumez

Comment 33 2012-08-23 07:46:01 PDT

What prevents this patch from landing?

Gyuyoung Kim

Comment 34 2012-09-02 22:36:34 PDT

Comment on attachment 152477 [details] patch for ewk_context APIs View in context: https://bugs.webkit.org/attachment.cgi?id=152477&action=review Please add unit test for public APIs as well. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:49 > + * Decreases the reference count of the given object, possibly freeing it. Add a new line. We have added a line to distinguish main description from detail. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:57 > * Gets default Ewk_Context instance. ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:66 > + * Creates a new Ewk_Context. ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:76 > + * Creates a new Ewk_Context. ditto.

Eunmi Lee

Comment 35 2012-09-06 22:46:18 PDT

(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > (In reply to comment #29) > > > > Could you explain why we need this API? What's the problem if we keep using only one default context? The GTK port seems to be doing fine this way. > > > > > > > > I just want to make sure we think about the use cases before making the API more complex. > > > > > > For Qt we kind of decided to not expose the context for now unless we really need to. It is also one of those API's which might change when webkit2 at some point supports multiple processes etc and we don't want to make wrong API decisions that we have to live with. > > > > The reason of that I expose the context is that we need context which is created with injected bundle path. > > We are using WK2 EFL port with injected bundle in the Tizen. > > So, I need this patch :) > > Qt handles that fine with private API. This is what some also call SPI (system programming interface). > > The inject bundle can be used for a lot of things. For Qt we took out some use-cases and made nice API's for those. Such as evaluateJavaScript, postMessage/receiveMessage (UI <-> Web process IPC). > > Also I can suggest using a local web server for exposing additional API's. I also have a plan to add APIs like Qt such as evaluateJavaScript and postMessage/receiveMessage. Additionally, we need to do other things using injected bundle. We need to process something in the WebProcess side such WKBundlePageResourceClient, WKBundlePageLoaderClient and so on. and the application has to add path of library used for injected bundle for that. so, I've added ewk_context_new_with_injected_bundle_path() API.

Eunmi Lee

Comment 36 2012-09-06 22:47:11 PDT

(In reply to comment #34) > (From update of attachment 152477 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152477&action=review > > Please add unit test for public APIs as well. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:49 > > + * Decreases the reference count of the given object, possibly freeing it. > > Add a new line. We have added a line to distinguish main description from detail. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:57 > > * Gets default Ewk_Context instance. > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:66 > > + * Creates a new Ewk_Context. > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:76 > > + * Creates a new Ewk_Context. > > ditto. Thanks for your comments. I will rebase and update the patch as your comments.

Eunmi Lee

Comment 37 2012-09-07 00:00:19 PDT

Created attachment 162686 [details] [WIP] Patch

Eunmi Lee

Comment 38 2012-09-12 22:25:17 PDT

Created attachment 163775 [details] Patch

Gyuyoung Kim

Comment 39 2012-09-12 22:34:58 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review > Source/WebKit2/UIProcess/API/efl/tests/InjectedBundle/injected_bundle_sample.cpp:1 > +#include <WebKit2/WKBundleInitialize.h> Missing boilerplate. > Source/WebKit2/UIProcess/API/efl/tests/InjectedBundle/injected_bundle_sample.cpp:4 > +{ No test ?

Kangil Han

Comment 40 2012-09-12 22:44:37 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:61 > + unsigned int __ref; /**< the reference count of the object */ unsigned int -> unsigned?

Chris Dumez

Comment 41 2012-09-12 22:57:05 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review With this patch, PageClientImpl::handleDownloadRequest() needs to be fixed so that you retrieve the context using ewk_view_context_get(m_viewWidget) instead of calling ewk_context_default_get(). > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:247 > + Shouldn't we at least check that the given path exists?

Eunmi Lee

Comment 42 2012-09-12 23:24:50 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:61 >> + unsigned int __ref; /**< the reference count of the object */ > > unsigned int -> unsigned? Let's change that with other "unsigned int" in the other files when we decide to use "unsigned". :) >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:247 >> + > > Shouldn't we at least check that the given path exists? Yes, you have a point. It's better to check path here. I will add code for that. >> Source/WebKit2/UIProcess/API/efl/tests/InjectedBundle/injected_bundle_sample.cpp:1 >> +#include <WebKit2/WKBundleInitialize.h> > > Missing boilerplate. oops, I will add boilerplate. >> Source/WebKit2/UIProcess/API/efl/tests/InjectedBundle/injected_bundle_sample.cpp:4 >> +{ > > No test ? There is nothing to do here because I just need codes to make injected bundle shared library.

Gyuyoung Kim

Comment 43 2012-09-12 23:28:46 PDT

Eunmi, you need to check Bug 96604. It modifies reference count mechanism.

Eunmi Lee

Comment 44 2012-09-12 23:46:23 PDT

(In reply to comment #43) > Eunmi, you need to check Bug 96604. It modifies reference count mechanism. Thanks for your comment. That's convenient to use :) I will modify like that.

Eunmi Lee

Comment 45 2012-09-13 00:12:53 PDT

Created attachment 163796 [details] Patch

Chris Dumez

Comment 46 2012-09-13 00:31:55 PDT

(In reply to comment #45) > Created an attachment (id=163796) [details] > Patch I insist :) With this patch, PageClientImpl::handleDownloadRequest() needs to be fixed so that you retrieve the context using ewk_view_context_get(m_viewWidget) instead of calling ewk_context_default_get().

Eunmi Lee

Comment 47 2012-09-13 00:37:29 PDT

(In reply to comment #46) > (In reply to comment #45) > > Created an attachment (id=163796) [details] [details] > > Patch > > I insist :) > > With this patch, PageClientImpl::handleDownloadRequest() needs to be fixed so that you retrieve the context using ewk_view_context_get(m_viewWidget) instead of calling ewk_context_default_get(). Yes, right! I will replace ewk_context_default_get() to ewk_view_context_get(). :)

Eunmi Lee

Comment 48 2012-09-13 00:52:11 PDT

Created attachment 163804 [details] Patch

Kenneth Rohde Christiansen

Comment 49 2012-09-13 00:54:52 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review I guess you should already think about how to make this api work with multiple processes now that webkit2 even supports it. You can upstream the API in pieces, but I would like to have a look at how you intent the whole API to function >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:61 >>> + unsigned int __ref; /**< the reference count of the object */ >> >> unsigned int -> unsigned? > > Let's change that with other "unsigned int" in the other files when we decide to use "unsigned". :) WebKit style guide says to use "unsigned" and not "unsigned int" >>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:247 >>> + >> >> Shouldn't we at least check that the given path exists? > > Yes, you have a point. It's better to check path here. > I will add code for that. Good idea, better use realpath as well? like the below pseudo code char* realPath = ecore_file_realpath(path); // "" in case of error. WKRetainPtr<WKStringRef> pathRef if (ecore_file_is_dir(realPath)) pathRef = WKRetainPtr<WKStringRef>(AdoptWK, WKStringCreateWithUTF8CString(path)); free(realPath); return new Ewk_Context(adoptWK(WKContextCreateWithInjectedBundlePath(pathRef.get()))); > Source/WebKit2/UIProcess/API/efl/ewk_context.h:92 > + * The returned Ewk_Context object @b should be unrefed after use. unref'ed or unref-ed as unrefed is pronounced with the first e as being [i:]

Eunmi Lee

Comment 50 2012-09-13 03:26:27 PDT

Comment on attachment 163775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review >>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:61 >>>> + unsigned int __ref; /**< the reference count of the object */ >>> >>> unsigned int -> unsigned? >> >> Let's change that with other "unsigned int" in the other files when we decide to use "unsigned". :) > > WebKit style guide says to use "unsigned" and not "unsigned int" I will change about this after rule is decided. >>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:247 >>>> + >>> >>> Shouldn't we at least check that the given path exists? >> >> Yes, you have a point. It's better to check path here. >> I will add code for that. > > Good idea, better use realpath as well? like the below pseudo code > > char* realPath = ecore_file_realpath(path); // "" in case of error. > > WKRetainPtr<WKStringRef> pathRef > > if (ecore_file_is_dir(realPath)) > pathRef = WKRetainPtr<WKStringRef>(AdoptWK, WKStringCreateWithUTF8CString(path)); > free(realPath); > > return new Ewk_Context(adoptWK(WKContextCreateWithInjectedBundlePath(pathRef.get()))); I've prepared patch using WebCore/FileSystem.h but I think it is better to use ecore API in the EFL port. additionally, the path is for injected bundle library's path and it is not a directory, so we don't have to check path is directory or not. I will update patch to use ecore_file_realpath(). >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:92 >> + * The returned Ewk_Context object @b should be unrefed after use. > > unref'ed or unref-ed as unrefed is pronounced with the first e as being [i:] Thanks, I will change the code to use "unref'ed".

Eunmi Lee

Comment 51 2012-09-13 03:30:48 PDT

(In reply to comment #49) > (From update of attachment 163775 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review > > I guess you should already think about how to make this api work with multiple processes now that webkit2 even supports it. You can upstream the API in pieces, but I would like to have a look at how you intent the whole API to function > I'm sorry but I couldn't catch your point in the above comment. Do you mean that you need an example to use APIs in this patch?

Eunmi Lee

Comment 52 2012-09-13 05:46:26 PDT

Created attachment 163850 [details] Patch

Eunmi Lee

Comment 53 2012-09-13 05:47:49 PDT

(In reply to comment #51) > (In reply to comment #49) > > (From update of attachment 163775 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=163775&action=review > > > > I guess you should already think about how to make this api work with multiple processes now that webkit2 even supports it. You can upstream the API in pieces, but I would like to have a look at how you intent the whole API to function > > > > I'm sorry but I couldn't catch your point in the above comment. > Do you mean that you need an example to use APIs in this patch? I've added below comment to the ewk_context.h file :) "The new WebProcess will be created whenever creating new ewk_context, so we have to recognize that when we create new ewk_context. The ewk_views within the ewk_context share one WebProcess. It is because that default process model is the Shared Secondary Process Model and there is no API to change the process model yet. If we can set the process model to the Multiple Secondary Processes, we can have WebProcesses per each ewk_view."

Raphael Kubo da Costa (:rakuco)

Comment 54 2012-09-17 05:21:24 PDT

Comment on attachment 163850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163850&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:255 > + char* realPath = ecore_file_realpath(path); > + if (!strlen(realPath)) { > + free(realPath); > + return 0; > + } > + free(realPath); Isn't ecore_file_exists() enough here? By the way, ecore_file_{init,shutdown}() do not seem to be called anywhere, nor do you seem to be linking against ECORE_FILE_LIBRARIES and add ECORE_FILE_INCLUDE_DIRS to the include paths. > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:48 > + return TEST_RESOURCES_DIR"/libewk2UnitTestInjectedBundleSample.so"; Nitpick: please separate the macro from the string in order to be C++11-compliant.

Kenneth Rohde Christiansen

Comment 55 2012-09-17 06:15:07 PDT

> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:255 > > + char* realPath = ecore_file_realpath(path); > > + if (!strlen(realPath)) { > > + free(realPath); > > + return 0; > > + } > > + free(realPath); No need to iterate the whole strign with strlen. If it is not valid it will be "", strcmp(realPath, "") should be enough

Eunmi Lee

Comment 56 2012-09-18 00:29:03 PDT

Comment on attachment 163850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163850&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:255 >> + free(realPath); > > Isn't ecore_file_exists() enough here? > > By the way, ecore_file_{init,shutdown}() do not seem to be called anywhere, nor do you seem to be linking against ECORE_FILE_LIBRARIES and add ECORE_FILE_INCLUDE_DIRS to the include paths. Yes, ecore_file_exists() is enough here. I will modify code to use that. and, Thanks for your comment. I have to add ecore_file_{init,shutdown}(), ECORE_FILE_LIBRARIES and ECORE_FILE_INCLUDE_DIRS. One question, is it OK to call ecore_file_init() before ecore_file_exists() and call ecore_file_shutdown() after using ecore_file_exists()? >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:48 >> + return TEST_RESOURCES_DIR"/libewk2UnitTestInjectedBundleSample.so"; > > Nitpick: please separate the macro from the string in order to be C++11-compliant. ok, thanks.

Eunmi Lee

Comment 57 2012-09-18 00:30:50 PDT

(In reply to comment #55) > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:255 > > > + char* realPath = ecore_file_realpath(path); > > > + if (!strlen(realPath)) { > > > + free(realPath); > > > + return 0; > > > + } > > > + free(realPath); > > No need to iterate the whole strign with strlen. If it is not valid it will be "", strcmp(realPath, "") should be enough I'm going to use ecore_file_exists() instead of ecore_file_realpath, so strlen is not needed anymore. I will use strcmp instead of strlen when I need to compare with "" later.

Eunmi Lee

Comment 58 2012-09-18 01:29:56 PDT

Created attachment 164512 [details] Patch

Kenneth Rohde Christiansen

Comment 59 2012-09-18 04:08:02 PDT

Comment on attachment 164512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164512&action=review Looks more or less good to me, others? > Source/WebKit2/UIProcess/API/efl/ewk_context.h:28 > + * The new WebProcess will be created whenever creating new ewk_context, The new web-engine process will... Btw the English isnt so good here

Gyuyoung Kim

Comment 60 2012-09-18 04:24:40 PDT

Raphael Kubo da Costa (:rakuco)

Comment 61 2012-09-18 05:35:32 PDT

(In reply to comment #60) > (In reply to comment #59) > > (From update of attachment 164512 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=164512&action=review > > > > Looks more or less good to me, others? > > Looks fine on my side. The way ecore_file_{init,shutdown} are being called is weird. One should call init when the code is being initialized, and shutdown when, well, shutting down.

Kenneth Rohde Christiansen

Comment 62 2012-09-18 07:56:30 PDT

(In reply to comment #61) > (In reply to comment #60) > > (In reply to comment #59) > > > (From update of attachment 164512 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=164512&action=review > > > > > > Looks more or less good to me, others? > > > > Looks fine on my side. > > The way ecore_file_{init,shutdown} are being called is weird. One should call init when the code is being initialized, and shutdown when, well, shutting down. Is that call even needed by that method?

Raphael Kubo da Costa (:rakuco)

Comment 63 2012-09-18 08:03:04 PDT

(In reply to comment #62) > > The way ecore_file_{init,shutdown} are being called is weird. One should call init when the code is being initialized, and shutdown when, well, shutting down. > > Is that call even needed by that method? Do you mean init and shutdown? If so, one is expected to init each "subsystem" before using it.

Eunmi Lee

Comment 64 2012-09-19 00:27:47 PDT

(In reply to comment #59) > (From update of attachment 164512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164512&action=review > > Looks more or less good to me, others? > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:28 > > + * The new WebProcess will be created whenever creating new ewk_context, > > The new web-engine process will... > > Btw the English isnt so good here Thanks, I will try to modify sentences.

Eunmi Lee

Comment 65 2012-09-19 00:41:14 PDT

(In reply to comment #63) > (In reply to comment #62) > > > The way ecore_file_{init,shutdown} are being called is weird. One should call init when the code is being initialized, and shutdown when, well, shutting down. > > > > Is that call even needed by that method? > > Do you mean init and shutdown? If so, one is expected to init each "subsystem" before using it. Actually, ecore_file_{init,shutdown} are called when WebContext is created and deleted in the WebCore/platform/efl/RunLoopEfl.cpp. But in this patch, I have to use ecore_file_exists() before creating WebContext. so, I could not find good place to call ecore_file_{init,shutdown}. How about using WebCore/FileSystem's fileExists() function in this case? Do I have to use only ecore_file_* APIs for efl port?

Eunmi Lee

Comment 66 2012-09-19 04:35:37 PDT

Created attachment 164711 [details] Patch

Jinwoo Song

Comment 67 2012-09-19 05:07:39 PDT

Comment on attachment 164711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164711&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.h:42 > + * How about to simplify the comments like this? * - Shared Secondary Process Model: A web-engine process is created per each context. * - Multiple Secondary Processes Model: A web-engine process is created per each page. * The default process model is Shared Secondary Process Model and WebKit2/EFL port * does not support the Multiple Secondary Processes Model yet.

Eunmi Lee

Comment 68 2012-09-19 19:35:29 PDT

(In reply to comment #67) > (From update of attachment 164711 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164711&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:42 > > + * > > How about to simplify the comments like this? > * - Shared Secondary Process Model: A web-engine process is created per each context. > * - Multiple Secondary Processes Model: A web-engine process is created per each page. > * The default process model is Shared Secondary Process Model and WebKit2/EFL port > * does not support the Multiple Secondary Processes Model yet. OK, I will modify like that :)

Eunmi Lee

Comment 69 2012-09-19 19:41:58 PDT

Created attachment 164826 [details] Patch

Kenneth Rohde Christiansen

Comment 70 2012-09-20 03:21:23 PDT

Comment on attachment 164826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164826&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.h:32 > + * Applications can create multiple pages that belong to one context and > + * all pages in the same context share the same visited link set, local storage > + * set, preferences and so on. If application wants to make page which has > + * different preferences than other pages, it can create new context and > + * creates page that belongs to the newly created context. If application does > + * not create any context and creates only pages, the default context will be > + * created and all created pages will belong to the default context. Applications have the option of creating a context different than the default one and use it for a group of pages. All pages in the same context share the same preferences, visited link set, local storage, etc. A process model can be specified per context. The default one is the shared model where the web-engine process is shared among the pages in the context. The second model allows each page to use a separate web-engine process. This latter model is currently not supported by WebKit2/EFL.

Eunmi Lee

Comment 71 2012-09-20 03:32:38 PDT

Comment on attachment 164826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164826&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:32 >> + * created and all created pages will belong to the default context. > > Applications have the option of creating a context different than the default one > and use it for a group of pages. All pages in the same context share the same > preferences, visited link set, local storage, etc. > > A process model can be specified per context. The default one is the shared model > where the web-engine process is shared among the pages in the context. The second > model allows each page to use a separate web-engine process. This latter model is > currently not supported by WebKit2/EFL. wow, I'm really grateful for your elaboration and review! :)

Raphael Kubo da Costa (:rakuco)

Comment 72 2012-09-20 04:43:58 PDT

(In reply to comment #65) > How about using WebCore/FileSystem's fileExists() function in this case? > Do I have to use only ecore_file_* APIs for efl port? OK, skipping all the conversation we had about ewk_{init,shutdown} on IRC and attaining myself only to this question -- using fileExists() would work. We do not have established rules about when to use a call from WebCore or from the EFL. The latter seems more common across ports, though.

Eunmi Lee

Comment 73 2012-09-20 06:00:15 PDT

Created attachment 164902 [details] Patch

WebKit Review Bot

Comment 74 2012-09-20 07:39:01 PDT

Comment on attachment 164902 [details] Patch Clearing flags on attachment: 164902 Committed r129134: <http://trac.webkit.org/changeset/129134>

WebKit Review Bot

Comment 75 2012-09-20 07:39:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.