Bug 90054 – [EFL][WK2] Make ewk_view inheritable in the WebKit2.

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

[Eunmi Lee]

The current ewk_view can not be inherited in the WebKit2 because the smart_class_set() API is not opened.
This bug will make ewk_view inheritable.

[Eunmi Lee]

Created attachment 149705 [details]
patch for inheritance

[Gyuyoung Kim]

Comment on attachment 149705 [details]
patch for inheritance

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:522
> +    EWK_VIEW_SD_GET(ewkView, smartData);

Why not use EWK_VIEW_SD_GET_OR_RETURN() instead of EWK_VIEW_SD_GET() ? Do you need to free ewkView object when failing to get smart data ?

[Ryuan Choi]

Comment on attachment 149705 [details]
patch for inheritance

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

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:40
> +Evas_Object* ewk_view_base_add(Evas*, Ewk_Context*, Ewk_Page_Group*);

Doesn't we need to expose this?

[Eunmi Lee]

(In reply to comment #2)
> (From update of attachment 149705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149705&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:522
> > +    EWK_VIEW_SD_GET(ewkView, smartData);
> 
> Why not use EWK_VIEW_SD_GET_OR_RETURN() instead of EWK_VIEW_SD_GET() ? Do you need to free ewkView object when failing to get smart data ?

Yes, I think it is better to free the ewkView and return 0 when we fail to create SmartData and Private Data because the ewkView does not work correctly without them.

[Eunmi Lee]

(In reply to comment #3)
> (From update of attachment 149705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149705&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:40
> > +Evas_Object* ewk_view_base_add(Evas*, Ewk_Context*, Ewk_Page_Group*);
> 
> Doesn't we need to expose this?

I think we don't have to expose now because we can not create Ewk_Page_Group yet.
We can expose that when API to create Ewk_Page_Group is added.

[Chris Dumez]

Comment on attachment 149705 [details]
patch for inheritance

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

> Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:40
> +    if (!ewkPageGroup)

Shouldn't we use EINA_SAFETY_ON_NULL_RETURN_VAL(ewkPageGroup, 0); ? Passing NULL is not valid API use IMHO.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:39
> +Evas_Object* ewk_view_add_with_WKContext_and_WKPageGroup(Evas*, WKContextRef, WKPageGroupRef);

Can't we make this function name a bit shorter? :)

[Eunmi Lee]

(In reply to comment #6)

Thanks for your comments :)

> (From update of attachment 149705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149705&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_page_group.cpp:40
> > +    if (!ewkPageGroup)
> 
> Shouldn't we use EINA_SAFETY_ON_NULL_RETURN_VAL(ewkPageGroup, 0); ? Passing NULL is not valid API use IMHO.

Yes, right. It's better to use EINA_SAFETY in this case. I will change it.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:39
> > +Evas_Object* ewk_view_add_with_WKContext_and_WKPageGroup(Evas*, WKContextRef, WKPageGroupRef);
> 
> Can't we make this function name a bit shorter? :)

Yes, the name is long. so how about ewk_view_add_with_WKType()?

[Chris Dumez]

Comment on attachment 149705 [details]
patch for inheritance

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:39
>>> +Evas_Object* ewk_view_add_with_WKContext_and_WKPageGroup(Evas*, WKContextRef, WKPageGroupRef);
>> 
>> Can't we make this function name a bit shorter? :)
> 
> Yes, the name is long. so how about ewk_view_add_with_WKType()?

Yes, ewk_view_add_with_WKType() is better for me. Let's see if Gyuyoung/Ryuan agree :)

[Ryuan Choi]

(In reply to comment #8)
> (From update of attachment 149705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149705&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:39
> >>> +Evas_Object* ewk_view_add_with_WKContext_and_WKPageGroup(Evas*, WKContextRef, WKPageGroupRef);
> >> 
> >> Can't we make this function name a bit shorter? :)
> > 
> > Yes, the name is long. so how about ewk_view_add_with_WKType()?
> 
> Yes, ewk_view_add_with_WKType() is better for me. Let's see if Gyuyoung/Ryuan agree :)

+1

[Gyuyoung Kim]

(In reply to comment #8)

> Yes, ewk_view_add_with_WKType() is better for me. Let's see if Gyuyoung/Ryuan agree :)

Looks good to me.

[Eunmi Lee]

Created attachment 150107 [details]
patch for inheritance

I've applied review comments to this new patch.

[Kenneth Rohde Christiansen]

Comment on attachment 150107 [details]
patch for inheritance

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

> Source/WebKit2/ChangeLog:10
> +        Make the ewk_view inheritable by adding ewk_view_smart_class_set() API
> +        to the ewk_view.h.
> +        Additionally, the initialize() API is added to the SmartClass and it

What does that mean (sorry didnt look at the patch yet)? You will be able to reuse all the API? Are you sure that is a good idea? bringing over legacy?

[Eunmi Lee]

(In reply to comment #12)
> (From update of attachment 150107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150107&action=review
> 
> > Source/WebKit2/ChangeLog:10
> > +        Make the ewk_view inheritable by adding ewk_view_smart_class_set() API
> > +        to the ewk_view.h.
> > +        Additionally, the initialize() API is added to the SmartClass and it
> 
> What does that mean (sorry didnt look at the patch yet)? You will be able to reuse all the API? Are you sure that is a good idea? bringing over legacy?

I want to make ewk_view inheritable like WebKit1's ewk_view,
because I want to make ewk_view_touchscreen which inherits the ewk_view.
You can see the Bug 88631.
Actually I can make ewk_view_touchscreen without inheritance, but I think I have to do duplicated works such as drawing contents without inheritance.

If you think my answer is not enough, please feel free to ask me again :)

[Kenneth Rohde Christiansen]

> > What does that mean (sorry didnt look at the patch yet)? You will be able to reuse all the API? Are you sure that is a good idea? bringing over legacy?
> 
> I want to make ewk_view inheritable like WebKit1's ewk_view,
> because I want to make ewk_view_touchscreen which inherits the ewk_view.
> You can see the Bug 88631.
> Actually I can make ewk_view_touchscreen without inheritance, but I think I have to do duplicated works such as drawing contents without inheritance.
> 
> If you think my answer is not enough, please feel free to ask me again :)

Look at where the industry is going. Every browser will support touch screens and might even do so as well as supporting mouse (think windows 8). I think you should forget about current desktop use cases (legacy) and concentrate on creating a top-notch mobile web view, because the mobile is moving to the desktop and not the other way around.

[Eunmi Lee]

(In reply to comment #14)
> > > What does that mean (sorry didnt look at the patch yet)? You will be able to reuse all the API? Are you sure that is a good idea? bringing over legacy?
> > 
> > I want to make ewk_view inheritable like WebKit1's ewk_view,
> > because I want to make ewk_view_touchscreen which inherits the ewk_view.
> > You can see the Bug 88631.
> > Actually I can make ewk_view_touchscreen without inheritance, but I think I have to do duplicated works such as drawing contents without inheritance.
> > 
> > If you think my answer is not enough, please feel free to ask me again :)
> 
> Look at where the industry is going. Every browser will support touch screens and might even do so as well as supporting mouse (think windows 8). I think you should forget about current desktop use cases (legacy) and concentrate on creating a top-notch mobile web view, because the mobile is moving to the desktop and not the other way around.

Kenneth, thank you for your comments.

Yes, right. the mobile is getting important these days,
and I also considered how to support touch screens as well as supporting mouse.
but, in my conclusion, I can not support both touch screen and mouse at the same time now in the EFL port.
Because, Evas can not discriminate the touchscreen's input and mouse's input.
That means, Evas generates "Mouse Event" when we touch the touchscreen and it also generates "Mouse Event" for Mouse device's operation.
I want to do scrolling for "Mouse down, move, up" on the touchscreen, but I want to do dragging for that with the mouse device,
so, I can not support touch screen and mouse at the same time.

Additionally, I think I can write the code to support touchscreen using "if else" statement in the all mouse event callback to separate the touchscreen and mouse in the ewk_view.cpp.
But, IMO, it is better to inherit and override the mouse event instead of using "if else" statement.

I really want to listen your thought about that. :)

BTW, This patch is for making ewk_view inheritable.
the ewk_view_touchscreen is just a one example of inheritance,
and I think it will be useful for applications to extend ewk_view functions if ewk_view is inheritable.
So, would you review this patch for that?
The discussion for ewk_view_touchscreen, will be applied in the Bug 88631.

[Kenneth Rohde Christiansen]

> Kenneth, thank you for your comments.
> 
> Yes, right. the mobile is getting important these days,
> and I also considered how to support touch screens as well as supporting mouse.
> but, in my conclusion, I can not support both touch screen and mouse at the same time now in the EFL port.
> Because, Evas can not discriminate the touchscreen's input and mouse's input.
> That means, Evas generates "Mouse Event" when we touch the touchscreen and it also generates "Mouse Event" for Mouse device's operation.
> I want to do scrolling for "Mouse down, move, up" on the touchscreen, but I want to do dragging for that with the mouse device,
> so, I can not support touch screen and mouse at the same time.

Qt has had the same issues but it is almost fixed now ;-) EFL developers should look at how we are fixing this. What you can do while this is not working is adding a method like "ewk_enable_mouse_events() / ewk_enable_touch_events()" to ignore one or the other. 

> Additionally, I think I can write the code to support touchscreen using "if else" statement in the all mouse event callback to separate the touchscreen and mouse in the ewk_view.cpp.
> But, IMO, it is better to inherit and override the mouse event instead of using "if else" statement.

Then you will have a problem in the future when your view supports both. Think about the future :-)

> 
> I really want to listen your thought about that. :)
> 
> BTW, This patch is for making ewk_view inheritable.
> the ewk_view_touchscreen is just a one example of inheritance,
> and I think it will be useful for applications to extend ewk_view functions if ewk_view is inheritable.

It can also easily become a can or worms (talking from experience). What is it that you need from the view that can only be solved by inheritance? why are free functions not good enough?

[Eunmi Lee]

(In reply to comment #16)
> > Kenneth, thank you for your comments.
> > 
> > Yes, right. the mobile is getting important these days,
> > and I also considered how to support touch screens as well as supporting mouse.
> > but, in my conclusion, I can not support both touch screen and mouse at the same time now in the EFL port.
> > Because, Evas can not discriminate the touchscreen's input and mouse's input.
> > That means, Evas generates "Mouse Event" when we touch the touchscreen and it also generates "Mouse Event" for Mouse device's operation.
> > I want to do scrolling for "Mouse down, move, up" on the touchscreen, but I want to do dragging for that with the mouse device,
> > so, I can not support touch screen and mouse at the same time.
> 
> Qt has had the same issues but it is almost fixed now ;-) EFL developers should look at how we are fixing this. What you can do while this is not working is adding a method like "ewk_enable_mouse_events() / ewk_enable_touch_events()" to ignore one or the other. 

Thanks for your advice.I hope EFL becomes good like Qt and I have to study Qt port. :)
and, I will modify Bug 88631 to use API such as ewk_enable_touch_events() to distinguish touch and mouse event.

> 
> > Additionally, I think I can write the code to support touchscreen using "if else" statement in the all mouse event callback to separate the touchscreen and mouse in the ewk_view.cpp.
> > But, IMO, it is better to inherit and override the mouse event instead of using "if else" statement.
> 
> Then you will have a problem in the future when your view supports both. Think about the future :-)

Yes, right. If I add ewk_view_touchscreen, I have to merge codes of ewk_view_touchscreen to the ewk_view when view supports both mouse and touch.
I didn't consider about that. and I think it is better to use "if else" statement in this case. :)

> 
> > 
> > I really want to listen your thought about that. :)
> > 
> > BTW, This patch is for making ewk_view inheritable.
> > the ewk_view_touchscreen is just a one example of inheritance,
> > and I think it will be useful for applications to extend ewk_view functions if ewk_view is inheritable.
> 
> It can also easily become a can or worms (talking from experience). What is it that you need from the view that can only be solved by inheritance? why are free functions not good enough?

There is 8 Smart Class functions in the ewk_view such as mouse_down, key_down, focus_in and so on, and the ewk_view offers the default behavior of them.
I think application can want to do different operations for those Smart Class functions.
If we do not allow the inheritance, the application (or extending library) can not override Smart Class functions.
I don't want to restrict applications' possibility. so I want to make the ewk_view inheritable.

[Eunmi Lee]

Created attachment 152426 [details]
patch to make ewk_view inheritable

I've rebased patch.

Kenneth, I'm waiting for your review. :)

[Eunmi Lee]

I've separated the ewk_view_group related codes to the Bug 91468.
I will make patch for this Bug again, and add dependency for Bug 91468.

[Eunmi Lee]

Created attachment 152760 [details]
[WIP] patch for inheritance

This patch depends on Bug 91468, so I will update patch again after Bug 91468 is reviewed.

[Eunmi Lee]

Created attachment 154873 [details]
Patch

[Eunmi Lee]

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

I've uploaded new patch for inheritance.

I made patch to add the initialize API to the SmartClass before, but I've changed my mind, because I think it is not clear to call initialize API in the object which inherits the ewk_view.

In the new patch, I make ewk_view inheritable by exposing the ewk_view_smart_class_set() and getting Ewk_View_Smart_Class_Data as a second parameter.

In order to make the ewk_view inheritable, I have to create PageClientImpl in the not ewk_view_base_add() but _smart_add().
but there is a problem because the _smart_add() can not pass the data to create PageClientImpl.
(Unlike WK1 EFL, we have to pass the data which is used to create WebPage in the WK2 such as Context and PageGroup.)
so, I have to consider how to pass the data to the ewk_view's constructor, and I've decided to use Smart_Class's data.

The Smart_Class's data was used only for type before, but I've made Ewk_View_Smart_Class_Data to store Context and PageGroup as well as type.

[Ryuan Choi]

Comment on attachment 154873 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:127
> +        Ewk_View_Smart_Class_Data* data = reinterpret_cast<Ewk_View_Smart_Class_Data*>(evas_smart_data_get(_tmp_s)); \
> +        if (EINA_UNLIKELY(data->type != EWK_VIEW_TYPE_STR)) {                                                        \

It's difficult for me to find what you change.
Although I know why you did, we'd better not to touch others if possible.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:551
> -    api->sc.data = EWK_VIEW_TYPE_STR; // It is used by type checking.
> +    api->sc.data = data;

If I am right, Smart class is shared for all ewkView object so that we will have only one ewk_context.

[Eunmi Lee]

(In reply to comment #23)
> (From update of attachment 154873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154873&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:127
> > +        Ewk_View_Smart_Class_Data* data = reinterpret_cast<Ewk_View_Smart_Class_Data*>(evas_smart_data_get(_tmp_s)); \
> > +        if (EINA_UNLIKELY(data->type != EWK_VIEW_TYPE_STR)) {                                                        \
> 
> It's difficult for me to find what you change.
> Although I know why you did, we'd better not to touch others if possible.
> 

I've changed the data of smart class, so I think it is reasonable to modify above codes.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:551
> > -    api->sc.data = EWK_VIEW_TYPE_STR; // It is used by type checking.
> > +    api->sc.data = data;
> 
> If I am right, Smart class is shared for all ewkView object so that we will have only one ewk_context.

The smart class is not shared for all ewkView object,
and each ewkView has its own smart class.
so, would you explain your thought again? I can not catch your point.

[Ryuan Choi]

Comment on attachment 154873 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:127
>>> +        if (EINA_UNLIKELY(data->type != EWK_VIEW_TYPE_STR)) {                                                        \
>> 
>> It's difficult for me to find what you change.
>> Although I know why you did, we'd better not to touch others if possible.
> 
> I've changed the data of smart class, so I think it is reasonable to modify above codes.

IMO, You'd better to change above code only although it break comment style.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:579
>      static Ewk_View_Smart_Class api = EWK_VIEW_SMART_CLASS_INIT_NAME_VERSION("Ewk_View");
>      static Evas_Smart* smart = 0;
>  
>      if (EINA_UNLIKELY(!smart)) {
> -        ewk_view_smart_class_init(&api);
> +        ewk_view_smart_class_set(&api, data);

Although you call ewk_view_smart_class_new with different data, api and smart will be initialized only one time.

[Eunmi Lee]

(In reply to comment #25)
> (From update of attachment 154873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154873&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:127
> >>> +        if (EINA_UNLIKELY(data->type != EWK_VIEW_TYPE_STR)) {                                                        \
> >> 
> >> It's difficult for me to find what you change.
> >> Although I know why you did, we'd better not to touch others if possible.
> > 
> > I've changed the data of smart class, so I think it is reasonable to modify above codes.
> 
> IMO, You'd better to change above code only although it break comment style.
> 

Now, I've finally got what you mean.
You mean I don't have to move "\" in the 96~113 line.
If it is OK to break the style, I will modify patch to not move the "\".

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:579
> >      static Ewk_View_Smart_Class api = EWK_VIEW_SMART_CLASS_INIT_NAME_VERSION("Ewk_View");
> >      static Evas_Smart* smart = 0;
> >  
> >      if (EINA_UNLIKELY(!smart)) {
> > -        ewk_view_smart_class_init(&api);
> > +        ewk_view_smart_class_set(&api, data);
> 
> Although you call ewk_view_smart_class_new with different data, api and smart will be initialized only one time.

I didn't catch the Ewk_View_Smart_Class is "static".
I will remove "static" to set the data for each ewk_view.
Thanks for your comment.

[Eunmi Lee]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 154873 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154873&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:127
> > >>> +        if (EINA_UNLIKELY(data->type != EWK_VIEW_TYPE_STR)) {                                                        \
> > >> 
> > >> It's difficult for me to find what you change.
> > >> Although I know why you did, we'd better not to touch others if possible.
> > > 
> > > I've changed the data of smart class, so I think it is reasonable to modify above codes.
> > 
> > IMO, You'd better to change above code only although it break comment style.
> > 
> 
> Now, I've finally got what you mean.
> You mean I don't have to move "\" in the 96~113 line.
> If it is OK to break the style, I will modify patch to not move the "\".
> 
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:579
> > >      static Ewk_View_Smart_Class api = EWK_VIEW_SMART_CLASS_INIT_NAME_VERSION("Ewk_View");
> > >      static Evas_Smart* smart = 0;
> > >  
> > >      if (EINA_UNLIKELY(!smart)) {
> > > -        ewk_view_smart_class_init(&api);
> > > +        ewk_view_smart_class_set(&api, data);
> > 
> > Although you call ewk_view_smart_class_new with different data, api and smart will be initialized only one time.
> 
> I didn't catch the Ewk_View_Smart_Class is "static".
> I will remove "static" to set the data for each ewk_view.
> Thanks for your comment.

I've try to remove static and allocate memory for Ewk_View_Smart_Class,
but I can't find the way to free the allocated Smart_Class! :(
because returned smart class from evas_smart_class_get() is "const" type.

I have to find another way for making ewk_view inheritable with specific data.

[Eunmi Lee]

Created attachment 156556 [details]
patch to make ewk_view inheritable

I've got an advice from EFL community to make Evas_Object inheritable with specific value.
http://sourceforge.net/mailarchive/forum.php?thread_name=CAOtL8OpdAnDSRWv2Vdn1F%2B1b_Hm7vhKVs%3DwHLcB1rSDO3KvdEw%40mail.gmail.com&forum_name=enlightenment-devel

I've made patch by referencing elementary's elm_widget.
The elm_widget provides elm_widget_add() which accepts parent as a parameter,
and the widget which inherits elm_widget creates its Evas_Object using elm_widget_add() instead of evas_object_smart_add().

In this patch, the ewk_view provides ewk_view_smart_add() API to create Evas_Object for WebKit2 EFL,
so, the object which inherits ewk_view can use that API to create Evas_Object and the ewk_view can get Ewk_Context using the ewk_view_smart_add()'s parameter.

I hope this patch can be the final patch for inheritance :)

[Ryuan Choi]

Comment on attachment 156556 [details]
patch to make ewk_view inheritable

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

Thank you.

Looks better than before, I have only below nit from this patch.

And I want to know whether this has any regression to run WTR/Efl.
Previously, WTR/Efl did not use Ewk_Context.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:703
> +    Evas_Object* ewkView = _ewk_view_base_add(canvas, _ewk_view_smart_class_new());
> +    if (!ewkView)
> +        return 0;
> +
> +    _ewk_view_initialize(ewkView, ewk_context_new_with_WKContext(contextRef), pageGroupRef);

This code is duplicated with below.

[Ryuan Choi]

(In reply to comment #29)
> (From update of attachment 156556 [details])
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:703
> > +    Evas_Object* ewkView = _ewk_view_base_add(canvas, _ewk_view_smart_class_new());
> > +    if (!ewkView)
> > +        return 0;
> > +
> > +    _ewk_view_initialize(ewkView, ewk_context_new_with_WKContext(contextRef), pageGroupRef);
> 
> This code is duplicated with below.

Sorry, please ignore this, I missed that second argument of _ewk_view_initialize is different.

Looks good to me if we don't have regression for WTR/Efl.

Please check WTR/Efl with/without this patch.

[Kenneth Rohde Christiansen]

Comment on attachment 156556 [details]
patch to make ewk_view inheritable

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:123
> +Ewk_Context* ewk_context_new_with_WKContext(WKContextRef contextRef)

from_WKContext?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:653
> +    if (priv->pageClient)
> +        return;

newline after return

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:695
> + * Constructs a ewk_view Evas_Object with WKType parameters.
> + */
> +Evas_Object* ewk_view_add_with_WKType(Evas* canvas, WKContextRef contextRef, WKPageGroupRef pageGroupRef)
> +{

I think this with_WKType is confusing. What is a WKType? Plus you have multiple arguments...  why not just like ewk_view_add_internal or so

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:73
> -Evas_Object* ewk_view_base_add(Evas* canvas, WKContextRef, WKPageGroupRef);
> +Evas_Object* ewk_view_add_with_WKType(Evas*, WKContextRef, WKPageGroupRef);

the old name seems better

[Eunmi Lee]

(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 156556 [details] [details])
> > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:703
> > > +    Evas_Object* ewkView = _ewk_view_base_add(canvas, _ewk_view_smart_class_new());
> > > +    if (!ewkView)
> > > +        return 0;
> > > +
> > > +    _ewk_view_initialize(ewkView, ewk_context_new_with_WKContext(contextRef), pageGroupRef);
> > 
> > This code is duplicated with below.
> 
> Sorry, please ignore this, I missed that second argument of _ewk_view_initialize is different.
> 
> Looks good to me if we don't have regression for WTR/Efl.
> 
> Please check WTR/Efl with/without this patch.

Thanks for your comment :)
I've checked with WTR/Efl and there is no regression with this patch.

[Eunmi Lee]

Comment on attachment 156556 [details]
patch to make ewk_view inheritable

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

Thanks for your comments :)

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:123
>> +Ewk_Context* ewk_context_new_with_WKContext(WKContextRef contextRef)
> 
> from_WKContext?

Thanks, that is better :)

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:653
>> +        return;
> 
> newline after return

OK, I will fix it.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:695
>> +{
> 
> I think this with_WKType is confusing. What is a WKType? Plus you have multiple arguments...  why not just like ewk_view_add_internal or so

OK, I will use old name as your below comment.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:73
>> +Evas_Object* ewk_view_add_with_WKType(Evas*, WKContextRef, WKPageGroupRef);
> 
> the old name seems better

Yes, I understand the ewk_view_add_with_WKType is confusing.
I think it is better to use old name.

[Eunmi Lee]

Created attachment 157172 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 157172 [details]
Patch

I am no EFL expert but this looks OK to me

[Eunmi Lee]

Created attachment 157194 [details]
Patch

[Ryuan Choi]

Comment on attachment 157194 [details]
Patch

Thank you.

[WebKit Review Bot]

Comment on attachment 157194 [details]
Patch

Clearing flags on attachment: 157194

Committed r125030: <http://trac.webkit.org/changeset/125030>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.