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

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

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 147952 [details]
patch for ewk_context APIs

[Eunmi Lee]

Created attachment 147953 [details]
patch for ewk_context APIs

Add "Reviewed by NOBODY (OOPS!)." to the Changelog.

[Ryuan Choi]

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 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]

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

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

Created attachment 149910 [details]
patch for ewk_context APIs

[Eunmi Lee]

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

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

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

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

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 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 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]

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]

Created attachment 152183 [details]
patch for ewk_context APIs

I modified code for Christophe's comment.

[Chris Dumez]

Comment on attachment 152183 [details]
patch for ewk_context APIs

Ah, I like it now :) LGTM.

[Gyuyoung Kim]

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]

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

Created attachment 152203 [details]
patch for ewk_context APIs

Fixed for Gyuyoung's comment.

[Gyuyoung Kim]

Comment on attachment 152203 [details]
patch for ewk_context APIs

LGTM now.

[Eunmi Lee]

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 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]

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

Created attachment 152477 [details]
patch for ewk_context APIs

Modified for Christophe's comment.

[Chris Dumez]

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]

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

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

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

What prevents this patch from landing?

[Gyuyoung Kim]

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]

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

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

Created attachment 162686 [details]
[WIP] Patch

[Eunmi Lee]

Created attachment 163775 [details]
Patch

[Gyuyoung Kim]

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 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 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 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]

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

[Eunmi Lee]

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

Created attachment 163796 [details]
Patch

[Chris Dumez]

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

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

Created attachment 163804 [details]
Patch

[Kenneth Rohde Christiansen]

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 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]

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

Created attachment 163850 [details]
Patch

[Eunmi Lee]

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

> > 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 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]

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

Created attachment 164512 [details]
Patch

[Kenneth Rohde Christiansen]

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]

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

Looks fine on my side.

[Raphael Kubo da Costa (:rakuco)]

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

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

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

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

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

Created attachment 164711 [details]
Patch

[Jinwoo Song]

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]

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

Created attachment 164826 [details]
Patch

[Kenneth Rohde Christiansen]

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

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

Created attachment 164902 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164902 [details]
Patch

Clearing flags on attachment: 164902

Committed r129134: <http://trac.webkit.org/changeset/129134>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.